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

Update clUpdateMutableCommandsKHR to match new API #501

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

EwanC
Copy link
Contributor

@EwanC EwanC commented Jul 19, 2024

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

  • Read and follow the project Code of Conduct.
  • Make sure the project builds successfully with your changes.
  • Run relevant testing locally to avoid regressions.
  • Run clang-format-17 on all modified code.

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
bashbaug pushed a commit to KhronosGroup/OpenCL-CTS that referenced this pull request Sep 6, 2024
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
codeplaysoftware/oneapi-construction-kit#501
@EwanC EwanC changed the title WIP: Update clUpdateMutableCommandsKHR to match new API Update clUpdateMutableCommandsKHR to match new API Sep 16, 2024
@EwanC EwanC marked this pull request as ready for review September 16, 2024 12:31
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{
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?


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;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Originally, config was a reference, it should probably remain a reference?

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

Successfully merging this pull request may close these issues.

3 participants