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

cl_khr_tensor extension #2

Closed
wants to merge 18 commits into from
Closed

Conversation

linehill
Copy link

No description provided.

Copy link
Owner

@pjaaskel pjaaskel left a comment

Choose a reason for hiding this comment

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

This is great!


=== Sample Codes

Helper functions used in the follow up tensor code samples:
Copy link
Owner

Choose a reason for hiding this comment

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

Let's first define a couple of helper functions which are used in the tensor usage examples:

// might not use some of the tensor.
assert(clEnqueueReadTensor(..., t0, ...) == CL_INVALID_OPERATION);
----

Copy link
Owner

Choose a reason for hiding this comment

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

The OpenCL C builtins for acessing the tensor metadata needs to be defined (and after that the SPIR-V extensions).

Copy link
Author

Choose a reason for hiding this comment

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

Could the OpenCL C support for tensors be in a separate extension? I think, there is a lot to be covered.

Built-in kernels should be able to define kernels tensor arguments without the OpenCL C tensor extension.

Copy link
Owner

Choose a reason for hiding this comment

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

Yes. Since this will be useful already for the BiKs, it doesn't require the OpenCL C or SPIR-V counterparts, you're right.

[[cl_khr_tensor]]
== Tensor Data Type

Purpose of this extension is to provide ...
Copy link
Owner

Choose a reason for hiding this comment

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

"This extension provides a new opaque OpenCL datatype called cl_tensor. It is used for storing N-dimensional tensor data in implementation-optimized way. The datatype is designed to be efficiently used within the cl_khr_command_buffers extension to capture task graphs which can utilize tensors as input, output and temporary storage." [etc etc]

Copy link
Author

Choose a reason for hiding this comment

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

Added this and tweaked it a little.

Copy link

@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.

Interesting extension! There are some parts of it that we might want to consider refactoring out, since they could be applicable to non-tensor objects also:

  1. The ability to bind a description of an object to an existing buffer. We already have this slightly, albeit in the other direction, in the form of the "2d image from buffer" extensions.
  2. The ability to attach "temporary" (or "virtual") objects to a command buffer, that are owned by a command buffer but that do not persist beyond command buffer execution.


[source,c]
----
cl_int clEnqueueReadTensor(

Choose a reason for hiding this comment

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

Presumably these read and write functions are invalid for CL_TENSOR_COMMAND_BUFFER_TEMPORARY tensors?

Copy link
Author

Choose a reason for hiding this comment

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

Yes and the functions should return CL_INVALID_OPERATION. This is attempted to be stated in CL_MEM_COMMAND_BUFFER_TEMPORARY property description (perhaps it's not clear enough?).

linehill and others added 12 commits November 2, 2023 14:16
Co-authored-by: Ben Ashbaugh <ben.ashbaugh@intel.com>
Co-authored-by: Pekka Jääskeläinen <pekka.jaaskelainen@tuni.fi>
* cl_khr_tensor -> cl_exp_tensor.

* Remove cl_khr_command_buffer requirement.
* Fix signed -> unsigned.

* Single line cl{Retain,Release}TensorObject declaration.
…Tensor.

* Clarify in clEnqueue{TranslateFrom,TranslateTo}Tensor that data read
  from / written to the tensor in opaque manner.
@linehill
Copy link
Author

linehill commented Nov 2, 2023

@bashbaug, temporary command buffer object property has been refactored out to be a buffer object property. Would this look good to you?

@bashbaug
Copy link

bashbaug commented Nov 7, 2023

Would this look good to you?

This seems like a good direction, yes.

A few other things we could consider:

  1. Perhaps the "temporary command buffer object" could be created from and have its object lifetime attached to a specific command buffer? This concept of a "virtual" allocation is something that I really liked from OpenVX (see e.g. vxCreateVirtualImage).
  2. Eventually we may want to refactor the property into its own extension so it could apply to ordinary buffers and images as well, though I think it's fine to keep it as part of this extension for now while proving its usefulness.

Thanks!

which can utilize tensors as input, output and temporary storage.

=== General information

Copy link
Owner

Choose a reason for hiding this comment

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

Let's write a motivational text here which is interfacing to the DBKs (refer to the WiP spec) on the backend, and to better interface to higher-level models for neural networks and other libraries/formats dealing with tensors.

* new section for tensor data type

* add origin, region and pitch parameters for clEnqueueTranslate*Tensor.

* Update code samples.

* Add take on accessing tensors in OpenCL C.
Copy link
Owner

@pjaaskel pjaaskel left a comment

Choose a reason for hiding this comment

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

Kiitos. Looks good. Please fix these and then submit the PR to the Khronos repo to hopefully get further feedback.


// TODO: add clEnqueueCopyTensor

// TODO: add clEnqueueFillTensor?

// TODO: add command buffer variants for clEnqueue{copy,read,write}Tensor.

TODO: add command buffer variants for clEnqueue*Tensor.
Copy link
Owner

Choose a reason for hiding this comment

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

Perhaps do this still before sending the Khronos/OpenCL-Docs pull request?

linehill and others added 2 commits November 15, 2023 14:00
Co-authored-by: Pekka Jääskeläinen <pekka.jaaskelainen@tuni.fi>
* Add error codes for tensor translation commands.

* Tweaked mem_pitch semantics.
@@ -418,11 +421,131 @@ an abstract function that accesses a tensor element in its storage at
given coordinate. The method how the coordinates translate to tensor
storage addresses is unspecified.

*clEnqueueTranslateFsomTensor* and *clEnqueueTranslateToTensor*
Copy link
Owner

Choose a reason for hiding this comment

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

Typo Fsom

Copy link
Owner

Choose a reason for hiding this comment

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

Maybe import/export are even better word here instead of 'translate'?

Copy link
Author

Choose a reason for hiding this comment

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

Fixed type and renamed.

// TODO: add clEnqueueCopyTensor

// TODO: add clEnqueueFillTensor?

TODO: add command buffer variants for clEnqueue*Tensor.
If *cl_khr_command_buffer* is is supported, then the following command
Copy link
Owner

Choose a reason for hiding this comment

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

is is

Copy link
Author

Choose a reason for hiding this comment

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

Fixed.

@linehill
Copy link
Author

Closing. This PR has moved to KhronosGroup#1006.

@linehill linehill closed this Nov 23, 2023
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