Update clUpdateMutableCommandsKHR to match new API #501
Conversation
Implementation change to prototype specification changes proposed in KhronosGroup/OpenCL-Docs#1041 Uses OpenCL changes from * KhronosGroup/OpenCL-Docs#1045 * KhronosGroup/OpenCL-Headers#245 * KhronosGroup/OpenCL-CTS#1984
EwanC
force-pushed
the
update_using_array
branch
from
0db99fe to
a91f218
Compare
CTS changes to reflect the spec changes merged in KhronosGroup/OpenCL-Docs#1045 and requires header updates from KhronosGroup/OpenCL-Headers#245 Tested using OCK implementation from uxlfoundation/oneapi-construction-kit#501
EwanC
force-pushed
the
update_using_array
branch
from
32ede68 to
33ad6a6
Compare
EwanC
changed the title
| CL_STRUCTURE_TYPE_MUTABLE_DISPATCH_CONFIG_KHR, | ||
| nullptr, | ||
| cl_mutable_dispatch_arg_khr args[] = {arg_0, arg_1, arg_2}; | ||
| cl_mutable_dispatch_config_khr dispatch_config{ |
There was a problem hiding this comment.
With the clang-tidy reports on the earlier version for variables that could be const, I am surprised that none are raised here. Could you check whether this really needs to be non-const, or whether the source file was skipped in the check?
There was a problem hiding this comment.
clang-tidy was being run (I manually run the tidy-clMutableDispathcKHR target to check) but the tool didn't seem to mandate const on these, but I went through and added const anyway
|
|
||
| for (const auto config_ptr : mutable_dispatch_configs) { | ||
| OCL_CHECK(config_ptr == nullptr, return CL_INVALID_VALUE); | ||
| const cl_mutable_dispatch_config_khr config = *config_ptr; |
There was a problem hiding this comment.
Originally, config was a reference, it should probably remain a reference?
There was a problem hiding this comment.
I've made this config variable a reference and the loop iteration variable config_ptr
EwanC
force-pushed
the
update_using_array
branch
from
935a451 to
c14327a
Compare
source/cl/examples/CMakeLists.txt
Outdated
| @@ -88,7 +88,7 @@ endif() | |||
| if(${OCL_EXTENSION_cl_khr_command_buffer_mutable_dispatch}) | |||
| # Intentionally misnamed. Windows guesses that executables with "patch" | |||
| # in the file name are installers and need admin privileges. | |||
| add_ca_cl_example(clMutableDispathcKHR | |||
| add_ca_cl_example(clMutableDispatchKHR | |||
There was a problem hiding this comment.
This was not a typo, the comment explains why it had that name.
There was a problem hiding this comment.
Reverted this, thanks
| } | ||
|
|
||
| const cl_mutable_dispatch_config_khr **casted_configs = | ||
| reinterpret_cast<const cl_mutable_dispatch_config_khr **>(configs); |
There was a problem hiding this comment.
I don't think this (when dereferencing the result) is well-defined in standard C++, and I do not recall whether current compilers optimize on the assumption that we do not do this. Do you know?
There was a problem hiding this comment.
So we're looking at the same thing, do you have a reference to where this is UB? I'm looking at https://en.cppreference.com/w/cpp/language/reinterpret_cast and guessing it's point 5) then "Type Accessibility" doesn't seem to cover this case as well defined.
I'm not sure how type based alias analysis works with void** that we're only reading from after the cast, could put this through compiler explorer to double check what some popular compilers are doing. An alternative could be to use a C-style case, but looking at https://en.cppreference.com/w/cpp/language/explicit_cast it seems like this will fallback to reinterpret_cast since static_cast isn't valid. The most conservative code is maybe defining a separate variable and memcpying to it.
There was a problem hiding this comment.
Yes, it's the type accessibility thing. There is no need to memcpy, we can use cargo::array_view<const void *> and use a static_cast on its elements instead.
EwanC
force-pushed
the
update_using_array
branch
from
c14327a to
84be0bf
Compare
EwanC
force-pushed
the
update_using_array
branch
from
67bd9ee to
abac7ad
Compare
Overview
Update cl_khr_command_buffer_mutable_dispatch implementation to reflect specification changes merged in KhronosGroup/OpenCL-Docs#1041
Reflects changes from upstream in:
Description of change
Updates our implementation from spec revision 0.9.1 of cl_khr_command_buffer_mutable_dispatch to revision 0.9.2 which made a breaking API change to
clUpdateMutableCommandsKHR().Checklist