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

command_buffer: Add clCommandSVMMemcpy and clCommandSVMMemFill #915

Merged
merged 5 commits into from Oct 3, 2023

Conversation

pjaaskel
Copy link
Contributor

This extends the command buffer extension with two SVM commands.

Copy link
Contributor

@EwanC EwanC left a comment

Choose a reason for hiding this comment

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

Have some minor editorial feedback, but don't have a problem adding these optional SVM entry-points. Although we don't support SVM, so can't provide in depth implementation coverage.

ext/cl_khr_command_buffer.asciidoc Outdated Show resolved Hide resolved
ext/cl_khr_command_buffer.asciidoc Outdated Show resolved Hide resolved
ext/cl_khr_command_buffer.asciidoc Outdated Show resolved Hide resolved
ext/cl_khr_command_buffer.asciidoc Outdated Show resolved Hide resolved
@EwanC EwanC added the cl_khr_command_buffer Relating to the command-buffer family of extension label Apr 27, 2023
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.

These should be pretty easy additions to the command buffer emulation layer if we decide to do this.

I would be interested in adding support for USM as well, perhaps as a layered extension.

edit: USM, not UVM...

ext/cl_khr_command_buffer.asciidoc Outdated Show resolved Hide resolved
ext/cl_khr_command_buffer.asciidoc Outdated Show resolved Hide resolved
@EwanC
Copy link
Contributor

EwanC commented May 4, 2023

I would be interested in adding support for UVM as well, perhaps as a layered extension.

I also thought about USM support when I saw this PR, I think support could be useful in the context of SYCL-Graphs where you can capture SYCL USM memcpy/memfill nodes, and want to propagate that down to commands in the underlying command-buffer representation.

A layered vendor extension for this makes sense to me if there are dependencies on both a vendor & KHR extension. We also have an existing ticket for the interaction of USM with cl_khr_command_buffer_mutable_dispatch, which a new layered extension could solve.

Also updated the date of the revision.
@pjaaskel
Copy link
Contributor Author

Thanks! I added the mention of System SVM as suggested by Ben.

@franz
Copy link
Contributor

franz commented Sep 26, 2023

related OpenCL-CTS tests are here

Comment on lines 386 to 387
include::{generated}/api/protos/clCommandSVMMemcpyKHR.txt[]
include::{generated}/api/protos/clCommandSVMMemFillKHR.txt[]
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm getting an asciidoctor warning building the specs that I think is caused by these two lines:

asciidoctor: WARNING: generated/api/protos/clCommandSVMMemcpyKHR.txt: line 4: id assigned to block already in use: clCommandSVMMemcpyKHR
asciidoctor: WARNING: generated/api/protos/clCommandSVMMemFillKHR.txt: line 4: id assigned to block already in use: clCommandSVMMemFillKHR

Basically, these generated snippets include a "label" for the API entry points to this means that they can only be included once. If we want to support including them multiple times - here, for example, and in the description of each API - then we'll need to do something to the spec tooling to allow this.

For now I would recommend just describing each API "manually", as is done above.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

@@ -378,6 +380,11 @@ cl_int clGetCommandBufferInfoKHR(
size_t* param_value_size_ret);
----

The following SVM entry points are included only from version 2.0, adding selected commands
Copy link
Contributor

Choose a reason for hiding this comment

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

Is "from version 2.0" the right description? Or, should this be "from version 0.9.4"?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Clarified. It now mentions both the min OpenCL version as well as the minimum ext version.

Comment on lines +1596 to +1597
successfully. Otherwise, it returns the errors defined by
{clEnqueueSVMMemcpy} except:
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need to say anything about an OpenCL 3.0 device that does not support SVM here, or is this one of the errors that was inherited by clEnqueueSVMMemcpy?

For reference, I believe the relevant error is:

CL_INVALID_OPERATION if the device associated with command queue does not support SVM.

Same comment also applies to the new SVM fill API.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I believe we don't need to explicitly state it, but it should be inherited from the 3.0 spec error description.

Copy-pasted the function fingerprints instead of including the generated
finger print twice to avoid an asciidoctor warning produced due to the
duplicated label.

Clarified that the SVM entry points are supported only starting OpenCL
2.0 and the ext 0.9.4 onwards.
@bashbaug
Copy link
Contributor

bashbaug commented Oct 3, 2023

Merging as discussed in the October 3rd teleconference.

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 needs-cts-coverage The CTS needs to be extended
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants