Skip to content

Comments

Add provisional command-buffer extension#182

Merged
bashbaug merged 3 commits intoKhronosGroup:masterfrom
EwanC:cl_khr_command_buffer
Nov 17, 2021
Merged

Add provisional command-buffer extension#182
bashbaug merged 3 commits intoKhronosGroup:masterfrom
EwanC:cl_khr_command_buffer

Conversation

@EwanC
Copy link
Contributor

@EwanC EwanC commented Nov 12, 2021

Run gen_headers.py from #161 on XML file containing KhronosGroup/OpenCL-Docs#711 new spec extensions.

The only XML changes are for the cl_khr_command_buffer extension, which declarations get generated at the top of cl_ext.h by the script. So I have taken the generated cl_khr_command_buffer code and applied it to the same location in master branch cl_ext.h.

Run `gen_headers.py` from KhronosGroup#161 on XML file
containing KhronosGroup/OpenCL-Docs#711 new spec extensions.

The only XML changes are for the `cl_khr_command_buffer` extension, which declarations gets
generated at the top of `cl_ext.h` by the script. So I have taken the generated
`cl_khr_command_buffer` code and applied it to the same location in master branch `cl_ext.h`.
Copy link
Contributor

@bashbaug bashbaug left a comment

Choose a reason for hiding this comment

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

LGTM, though we should probably hold off merging this until the spec PR is merged, just in case there are some last-minute changes.

My one minor comment is that this PR includes two "features" from the codegen scripts that I think are good but we haven't formally agreed to:

  • It includes a define for the extension name CL_KHR_COMMAND_BUFFER_EXTENSION_NAME.
  • It include the CL_NO_PROTOTYPES guard around the function prototypes.

I'm fine including these now, though if we want to stay consistent with the other extensions in this header at least until the codegen changes are merged we could remove them for now.

Thanks!

Copy link
Contributor

@bashbaug bashbaug left a comment

Choose a reason for hiding this comment

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

Needs to be updated to remove "INFO" from the names of the command buffer query enums, see: KhronosGroup/OpenCL-Docs#711 (review)

bashbaug
bashbaug previously approved these changes Nov 15, 2021
1. Change type of `cl_command_buffer_properties_khr` to `cl_properties`
2. Add a new `cl_command_buffer_flags_khr` type.
3. Rename `CL_COMMAND_BUFFER_PROPERTIES_KHR` to
`CL_COMMAND_BUFFER_FLAGS_KHR`.
  3a. Change the expected type of this to `cl_command_buffer_flags_khr`.
4. Change type of `cl_ndrange_kernel_command_properties_khr` to
`cl_properties`
Copy link
Contributor

@alycm alycm left a comment

Choose a reason for hiding this comment

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

declarations get generated at the top of cl_ext.h by the script. So I have taken the generated cl_khr_command_buffer code and applied it to the same location

It looks like the generation script goes alphabetically. No functional difference, so I don't mind where it goes.

My one minor comment is that this PR includes two "features" from the codegen scripts that I think are good but we haven't formally agreed to:

These seem harmless.

@bashbaug
Copy link
Contributor

Spec changes are merged so I think we can merge this now, too. Thanks!

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