-
Notifications
You must be signed in to change notification settings - Fork 81
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
Introduce test plan for CUDA backend interop #207
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Quite interesting starting point!
How can we handle various implementation details?
Or do you assume that all the CUDA backends will have the same underlying mapping of CUDA to SYCL?
is available and resolves to an alias of `CUevent`. | ||
* Check that the `sycl::backend_input_t<sycl::backend::cuda, sycl::buffer>` | ||
is available and resolves to an alias of | ||
`struct { CUdeviceptr ptr; size_t size; }`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks very much like an implementation detail for a given implementation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This has actually changed now in the CUDA backend specification, interop in this direction was removed as it was decided that it was too complicated to represent the complexities of the buffer
class as an input for interop.
* Check that the `sycl::backend_return_t<sycl::backend::cuda, sycl::event>` | ||
is available and resolves to an alias of `CUevent`. | ||
* Check that the `sycl::backend_return_t<sycl::backend::cuda, sycl::buffer>` | ||
is available and resolves to an alias of `CUdeviceptr`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At least it is interesting to see this as a Rosetta Stone for SYCL-CUDA translation. :-)
d33caac
to
309840d
Compare
I've updated the test plan to reflect changes in the CUDA backend spec and the tests in #336. |
* `unsigned long int` | ||
* `long long int` | ||
* `unsigned long long int` | ||
* `float` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some NVIDIA GPUs support double
data types. Can we add double
to this list?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great contribution!
* `vec<T, int dim>` where `T` is each of the scalar types listed above except | ||
for `bool` and `dim` is `1`, `2`, `3`, `4`, `8`, and `16`. | ||
* `marray<T, size_t dim>` where `T` is each of the scalar types listed above | ||
and `dim` is `2`, `5`, and `10`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Testing with 1
here too as a special array case.
LGTM after offline changes. |
@AerialMantis, I've applied code review suggestion in 3c3a06c. If you are okay with these changes, I suggest we merge this PR. @psalz, could you take a look at check-clang-format job, please? |
No idea what's going on there, looks like a GitHub issue: I'd suggest to wait a few hours and try again. |
@bader thanks for applying those changes, this looks good to me, I think we can merge. |
Introduce the first draft of a test plan for CUDA backend interoperability as defined in the CUDA backend specification.
A pull request for introducing the CUDA backend specification to the SYCL 2020 specification is here - KhronosGroup/SYCL-Docs#197