Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
kenba opened this issue Oct 9, 2021 · 6 comments
Closed

Comments

@kenba
Copy link
Owner

kenba commented Oct 9, 2021

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

kenba added a commit that referenced this issue Oct 10, 2021
@vmx
Copy link
Contributor

vmx commented Oct 12, 2021

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
Copy link
Owner Author

kenba commented Oct 12, 2021

@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
Copy link
Contributor

vmx commented Oct 13, 2021

@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 added a commit that referenced this issue Oct 15, 2021
@kenba
Copy link
Owner Author

kenba commented Oct 16, 2021

@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
Copy link
Contributor

vmx commented Oct 17, 2021

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
Copy link
Owner Author

kenba commented Oct 17, 2021

@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.

@kenba kenba closed this as completed Oct 23, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants