kenba/opencl3

Remove cl3 `Info` enums to support new OpenCL versions and extensions.

Closed this issue · 6 comments

kenba commented

Change interface to cl3 for issue kenba/cl3#13.

vmx commented

Is the idea to remove the info enums also from opencl3, or to move the enums from cl3 to the higher level opencl3?

In case they are removed: I see the point, though it would remove type information, and to me one of rust strength is its type system. So I prefer if higher level APIs make use of types. Though perhaps there could even be yet another layer that is higher then opencl3, which uses even more Rust idioms.

kenba commented

@vmx my intention is to remove all the Info types from both cl3 and opencl3.

I agree with you that it's nice to have the type information that the Info types provided. My original intention was to use the Info types in the get_*_info functions match statements for exhaustive matching. However, as I've incorporated OpenCL extensions and changes to the API I now see that these types as effectively "open ended", with new values being added as the spec evolves.

Since data for new values can always be returned as a Vec<u8>, it makes sense to me to allow users to call the get_*_info functions with cl_device_info, cl_platform_info, etc. types using whatever value they want, because those values may be valid for an OpenCL device in the future. It also makes it simpler to maintain the library, since it is no longer necessary to update the the Info types as well whenever a new value is added.

I've tried to provide methods in the opencl3 structs (e.g.: Device, Platform, etc.) to access all of the information that can be obtained using the Info types in the correct form, e.g. Device::vendor_id returns the vendor_id as a cl_uint, etc. I think that I should also add get_data methods to all of the structs to simply call the cl3 get_*_data function with a given info value and return a Result<Vec<u8>> to make the opencl3 API more future proof.

I've implemented the Info types change in the develop branches of cl3 and opencl3. I'd welcome your opinion on the changes Volker.

vmx commented

@kenba I had another look (especially at 02d5f68). What I hadn't in mind anymore (sorry for not having a closer look before commenting) was that you provide methods in opencl3. This means that (hopefully) the usual code path wouldn't even need to make info calls directly. Given then I think it makes sense to make those changes as it really makes things simpler and more flexible.

kenba commented

@vmx Thank you Volker. I have implemented the changes in both libraries and updated them both to version 0.6.0 in crates.io.

BTW I've also implemented UUID and LUID types (see issue kenba/cl3#14), which I'm now having second thoughts about. Because if the driver returns a Vec<u8> of the wrong size, the cl3 library will now panic. I'm considering reverting this change back to just returning a Vec<u8>. What do you think?

vmx commented

BTW I've also implemented UUID and LUID types (see issue kenba/cl3#14), which I'm now having second thoughts about. Because if the driver returns a Vec<u8> of the wrong size, the cl3 library will now panic. I'm considering reverting this change back to just returning a Vec<u8>. What do you think?

I like this change. The sizes of the IDs is specified in the OpenCL header files: https://github.com/KhronosGroup/OpenCL-Headers/blob/1aa1139b58a515877a923cce6b254e09d1b2fb2c/CL/cl_ext.h#L687-L688 Hence I don't see a reason why one shouldn't just depend on that information.

kenba commented

@vmx I agree with you that the OpenCL header files specify the sizes of the UUIDs and LUIDs so we should be able to depend on that information. However, we've both encountered OpenCL devices that do not to strictly conform to the OpenCL specification, see kenba/cl3#2 and #8 that you raised Volker.

I have implemented a fix for the panic issue in the cl3 library , see kenba/cl3#15. Since you like the new UUID and LUID types, the fix keeps them while eliminating the potential panic. That library has been published on crates.io as issue 0.6.1 and is picked up by opencl3 issue 0.6.0 after a cargo update.

BTW I have no more changes planned for the library, so this version (0.6.0) should be stable.