remove extension defines #148
Conversation
This isn't perfect! Because the extension function pointers are stored in static variables these tests have state and need to execute in a specific order.
bashbaug
force-pushed
the
no-extension-defines
branch
from
898d7db to
46775f8
Compare
|
I've found a few issues with this PR that I need to fix:
I'll mark this PR as ready for review when I have solved these issues. |
The helper functions to get the platform version are used in so many places they cannot be protected by ifdefs for specific target OpenCL versions or minimum OpenCL versions.
|
i think this is ready for review again. I've been using this version for a week or so and it seems to be working OK, though I admittedly haven't tested the DX interop very well. One note: We have some functions to get the platform version that used to be protected by ifdefs in some cases. They're used a lot more now, though, so much that it's not practical to protect them by ifdefs anymore. I think this is OK, but it's one area to pay attention to, if anyone remember why they were protected by ifdefs to begin with. Thanks! |
| if (devices) { | ||
| devices->resize(ids.size()); | ||
|
|
||
| // Assign to param, constructing with retain behaviour | ||
| // to correctly capture each underlying CL object | ||
| for (size_type i = 0; i < ids.size(); i++) { | ||
| // We do not need to retain because this device is being created | ||
| // by the runtime | ||
| (*devices)[i] = Device(ids[i], false); | ||
| } | ||
| } |
There was a problem hiding this comment.
IMHO this function is unfortunate in multiple ways, some of which are breaking so I won't list. However:
- if the user doesn't supply a vector, then the entire function is meaningless and we should shortcircuit the entire thing and not leak subdevice handles.
- the SDK utility library introduced new error codes for cases when the wrapper itself is misused. This function really has no meaning when a
nullptris provided by the user and we should likely signal that. Whether we repurpose an error code fromclCreateSubDevicesor start making our own, it's up to us. Silently leaking resources seems wrong.
I'm fine deferring this into a separate issue.
| devices->resize(ids.size()); | ||
|
|
||
| // Assign to param, constructing with retain behaviour | ||
| // to correctly capture each underlying CL object | ||
| for (size_type i = 0; i < ids.size(); i++) { | ||
| // We do not need to retain because this device is being created | ||
| // by the runtime | ||
| (*devices)[i] = Device(ids[i], false); | ||
| } |
There was a problem hiding this comment.
In the spirit of no raw loops. Mild bikeshedding, feel free to discard if more trouble than use.
| devices->resize(ids.size()); | |
| // Assign to param, constructing with retain behaviour | |
| // to correctly capture each underlying CL object | |
| for (size_type i = 0; i < ids.size(); i++) { | |
| // We do not need to retain because this device is being created | |
| // by the runtime | |
| (*devices)[i] = Device(ids[i], false); | |
| } | |
| devices->reserve(ids.size()); | |
| // Assign to param, constructing with retain behaviour | |
| // to correctly capture each underlying CL object | |
| std::transform(ids.cbegin(), ids.cend(), std::back_inserter(devices), [](const cl_device_id& id) { | |
| // We do not need to retain because this device is being created | |
| // by the runtime | |
| return Device(id, false); | |
| }); |
There was a problem hiding this comment.
Mind you, this requires algorithm. iterator is already included.
Fixes #146
This PR removes many of the extension defines from the C++ bindings and enables support for extensions based on defines in the OpenCL headers instead.
Specifically, the following extensions defines are removed:
CL_HPP_USE_CL_DEVICE_FISSION(forcl_ext_device_fission)CL_HPP_USE_CL_SUB_GROUPS_KHR(forcl_khr_subgroups)CL_HPP_USE_IL_KHR(forcl_khr_il_program)CL_HPP_USE_CL_IMAGE2D_FROM_BUFFER_KHR(forcl_khr_image2d_from_buffer)The extension define for
CL_HPP_USE_DX_INTEROP(forcl_khr_d3d10_sharing) has not been removed since the DX interop APIs require including the DX-specific header and hence should be enabled conditionally.Some implementation notes:
cl_ext_device_fissionsub-device creation based on the function overload, sincecl_device_partition_propertyis a different type thancl_device_partition_property_ext(intptr_tvs.cl_ulong).cl_khr_image2d_from_bufferuses the sameclCreateImageAPI for both the core and extension version there is no need to try both. Additionally, there is no runtime check, and the API call will either succeed or return an error.I think all of this is reasonble and correct but please double-check my reasoning!
A note about testing:
cl_khr_subgroupsandcl_khr_il_program, but this admittedly isn't perfect: because the C++ bindings store the extension API function pointers in static variables they are "stateful" and therefore must run in a specific order. We should rethink how extensions are handled to improve robustness, especially because the C++ bindings currently will not work for many extension APIs from multiple platforms.