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

Use array for clUpdateMutableCommandsKHR. #1045

Merged
merged 1 commit into from
Jul 16, 2024

Conversation

EwanC
Copy link
Contributor

@EwanC EwanC commented Jan 16, 2024

Proposal to pass the update configs to clUpdateMutableCommandsKHR as an array, rather than pointer chained linked list.

See #1041 for motivation.

EwanC added a commit to EwanC/oneapi-construction-kit that referenced this pull request Jan 17, 2024
Implementation change to prototype specification changes proposed
in KhronosGroup/OpenCL-Docs#1041

Uses OpenCL-Docs and OpenCL-Header changes from
* KhronosGroup/OpenCL-Docs#1045
* KhronosGroup/OpenCL-Headers#245
EwanC added a commit to EwanC/OpenCL-Docs that referenced this pull request Feb 21, 2024
Draft of `cl_khr_command_buffer_mutable_memory_commands` based
ontop of KhronosGroup#1045
which updates `clUpdateMutableCommandsKHR` to pass configs
by an array rather than linked list.

The goal of this extension is to be able to update the parameters to memory
operation commands in a command-buffer after the command-buffer has been
finalized using the `clUpdateMutableCommandsKHR` entry-point defined by
`cl_khr_command_buffer_mutable_dispatch`.
EwanC added a commit to EwanC/OpenCL-Docs that referenced this pull request Feb 21, 2024
Draft of `cl_khr_command_buffer_mutable_memory_commands` based
ontop of KhronosGroup#1045
which updates `clUpdateMutableCommandsKHR` to pass configs
by an array rather than linked list.

The goal of this extension is to be able to update the parameters to memory
operation commands in a command-buffer after the command-buffer has been
finalized using the `clUpdateMutableCommandsKHR` entry-point defined by
`cl_khr_command_buffer_mutable_dispatch`.
@EwanC EwanC marked this pull request as ready for review March 18, 2024 09:50
@EwanC
Copy link
Contributor Author

EwanC commented Mar 22, 2024

I've rebased this change after #950 merged, however I'm still getting familiar with how the spec looks now. So changing this PR back to draft until I double check I'm happy with how everything looks.

EDIT - taken this out of draft again, as I'm good with how it looks now.

@EwanC EwanC marked this pull request as draft March 22, 2024 14:27
@EwanC EwanC marked this pull request as ready for review March 25, 2024 14:34
@EwanC EwanC force-pushed the array_command_update branch 2 times, most recently from 62e7b7d to 2673582 Compare March 28, 2024 18:11
EwanC added a commit to EwanC/OpenCL-Docs that referenced this pull request Mar 28, 2024
Draft of `cl_khr_command_buffer_mutable_memory_commands` based
ontop of KhronosGroup#1045
which updates `clUpdateMutableCommandsKHR` to pass configs
by an array rather than linked list.

The goal of this extension is to be able to update the parameters to memory
operation commands in a command-buffer after the command-buffer has been
finalized using the `clUpdateMutableCommandsKHR` entry-point defined by
`cl_khr_command_buffer_mutable_dispatch`.
EwanC added a commit to EwanC/OpenCL-Docs that referenced this pull request Mar 29, 2024
Draft of `cl_khr_command_buffer_mutable_memory_commands` based
ontop of KhronosGroup#1045
which updates `clUpdateMutableCommandsKHR` to pass configs
by an array rather than linked list.

The goal of this extension is to be able to update the parameters to memory
operation commands in a command-buffer after the command-buffer has been
finalized using the `clUpdateMutableCommandsKHR` entry-point defined by
`cl_khr_command_buffer_mutable_dispatch`.
Proposal to pass the update configs to `clUpdateMutableCommandsKHR` as
an array, rather than pointer changed linked list.

See KhronosGroup#1041 for
motivation.
EwanC added a commit to EwanC/OpenCL-CTS that referenced this pull request Jun 19, 2024
CTS change to reflect proposed change from KhronosGroup/OpenCL-Docs#1041
Taking advantage of the extension version macros to avoid
breaking existing implementations immediately.

This reflects PRs:
* KhronosGroup/OpenCL-Docs#1045
* KhronosGroup/OpenCL-Headers#245
EwanC added a commit to EwanC/OpenCL-CTS that referenced this pull request Jun 19, 2024
CTS change to reflect proposed change from KhronosGroup/OpenCL-Docs#1041
Taking advantage of the extension version macros to avoid
breaking existing implementations immediately.

This reflects PRs:
* KhronosGroup/OpenCL-Docs#1045
* KhronosGroup/OpenCL-Headers#245
EwanC added a commit to EwanC/oneapi-construction-kit that referenced this pull request Jun 19, 2024
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 bashbaug linked an issue Jun 21, 2024 that may be closed by this pull request
@bashbaug bashbaug self-requested a review July 9, 2024 16:51
@kpet kpet self-requested a review July 9, 2024 16:51
@bashbaug
Copy link
Contributor

bashbaug commented Jul 9, 2024

Discussed in the July 9th teleconference. We set a deadline for two weeks to merge (July 23rd).

Copy link
Contributor

@kpet kpet left a comment

Choose a reason for hiding this comment

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

Thanks for proposing this change! Happy with the direction. I have not turned every stone and so might have missed points of detail but I suggest we prioritise aligning all components to this proposal to enable forward progress and fix any minor issues on top.

Comment on lines +15730 to +15734
If _configs_ is non-`NULL`, then for any {cl_mutable_dispatch_config_khr_TYPE}
element of the array the errors defined by {clEnqueueNDRangeKernel},
{clSetKernelExecInfo}, {clSetKernelArg}, and {clSetKernelArgSVMPointer} are
returned by {clUpdateMutableCommandsKHR} if any of the struct elements are set
to an invalid value. Additionally, the following errors are returned if any
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should try to improve how this is described so we make the list of errors that may be returned a bit clearer. This is not something that this PR changes so I'm prefectly fine with capturing an issue that we'd look at separately.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Creating an issue makes sense, is there any particular way you imagine the improvement looking? e.g. Being more explicit about the errors rather than referencing core API entry-points, or just making the layout of the existing information more readable.

Copy link
Contributor

@kpet kpet Jul 10, 2024

Choose a reason for hiding this comment

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

The first idea that came to mind was to categorise errors in the description of those commands such that we could refer to the categories here but I have not though enough about this to really have good input or opinions.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Created issue #1209 to track this

@bashbaug
Copy link
Contributor

Merging as discussed in the July 16th teleconference.

@bashbaug bashbaug merged commit c6cceb1 into KhronosGroup:main Jul 16, 2024
2 checks passed
EwanC added a commit to EwanC/OpenCL-Headers that referenced this pull request Jul 19, 2024
Proposal to pass the update configs to `clUpdateMutableCommandsKHR` as
an array, rather than pointer changed linked list.

See KhronosGroup/OpenCL-Docs#1041 for
motivation and KhronosGroup/OpenCL-Docs#1045
for OpenCL-Docs PR.
EwanC added a commit to EwanC/oneapi-construction-kit that referenced this pull request Jul 19, 2024
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 added a commit to EwanC/oneapi-construction-kit that referenced this pull request Jul 19, 2024
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 added a commit to EwanC/oneapi-construction-kit that referenced this pull request Jul 19, 2024
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 added a commit to EwanC/OpenCL-CLHPP that referenced this pull request Jul 31, 2024
In OpenCL-Docs PR KhronosGroup/OpenCL-Docs#1045
the API for `cl_khr_command_buffer_mutable_dispatch` API
`clUpdateMutableCommandsKHR` changed in a breaking way.

When the headers update OpenCL-Headers PR KhronosGroup/OpenCL-Headers#245
the bindings will break if they are not updated to reflect this
change.

Bindings updated in this PR to pass to arrays of templated length to
the C++ method. The underlying `.data()` pointers of these parameters can be
passed through to the OpenCL API.
EwanC added a commit to EwanC/OpenCL-CLHPP that referenced this pull request Aug 1, 2024
In OpenCL-Docs PR KhronosGroup/OpenCL-Docs#1045
the API for `cl_khr_command_buffer_mutable_dispatch` API
`clUpdateMutableCommandsKHR` changed in a breaking way.

When the headers update OpenCL-Headers PR KhronosGroup/OpenCL-Headers#245
the bindings will break if they are not updated to reflect this
change.

Bindings updated in this PR to pass to arrays of templated length to
the C++ method. The underlying `.data()` pointers of these parameters can be
passed through to the OpenCL API.
EwanC added a commit to EwanC/OpenCL-CTS that referenced this pull request Sep 5, 2024
CTS change to reflect proposed change from KhronosGroup/OpenCL-Docs#1041
Taking advantage of the extension version macros to avoid
breaking existing implementations immediately.

This reflects PRs:
* KhronosGroup/OpenCL-Docs#1045
* KhronosGroup/OpenCL-Headers#245
bashbaug pushed a commit to KhronosGroup/OpenCL-Headers that referenced this pull request Sep 6, 2024
Proposal to pass the update configs to `clUpdateMutableCommandsKHR` as
an array, rather than pointer changed linked list.

See KhronosGroup/OpenCL-Docs#1041 for
motivation and KhronosGroup/OpenCL-Docs#1045
for OpenCL-Docs PR.
bashbaug pushed a commit to KhronosGroup/OpenCL-CLHPP that referenced this pull request Sep 6, 2024
In OpenCL-Docs PR KhronosGroup/OpenCL-Docs#1045
the API for `cl_khr_command_buffer_mutable_dispatch` API
`clUpdateMutableCommandsKHR` changed in a breaking way.

When the headers update OpenCL-Headers PR KhronosGroup/OpenCL-Headers#245
the bindings will break if they are not updated to reflect this
change.

Bindings updated in this PR to pass to arrays of templated length to
the C++ method. The underlying `.data()` pointers of these parameters can be
passed through to the OpenCL API.
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cl_khr_command_buffer Relating to the command-buffer family of extension
Development

Successfully merging this pull request may close these issues.

Reconsider mutable command-buffer structure pointer chaining
4 participants