-
Notifications
You must be signed in to change notification settings - Fork 787
[SYCL] Added test-e2e for queue, device, and event interop. #11292
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
Conversation
All backends currently support these three types of interop, but a complete e2e test did not previously exist. Signed-off-by: JackAKirk <jack.kirk@codeplay.com>
Signed-off-by: JackAKirk <jack.kirk@codeplay.com>
Thank you for adding the test for my better understanding of interop. I have some questions about the program. Could SYCL_EXT_ONEAPI_BACKEND_CUDA_EXPERIMENTAL become SYCL_EXT_ONEAPI_BACKEND_CUDA ? Thanks |
|
Also tried to fix llvm testing instructions. Signed-off-by: JackAKirk <jack.kirk@codeplay.com>
Signed-off-by: JackAKirk <jack.kirk@codeplay.com>
I just read your reply. SyclDevice may be an alternative name, but your pushed your changes. Thanks. |
Signed-off-by: JackAKirk <jack.kirk@codeplay.com>
I don't think it is possible to support these backends in the same test file. Signed-off-by: JackAKirk <jack.kirk@codeplay.com>
Signed-off-by: JackAKirk <jack.kirk@codeplay.com>
int main() { | ||
|
||
device Device; | ||
auto NativeDevice = get_native<BACKEND>(Device); |
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.
Instead of using auto, could we consider making backend-dependent type aliases for these? I believe the backend specifications should be making some requirement for what they are, so it would be another level of checks we would do.
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.
Sure,
Do you mean having backend dependent alias' for e.g.
TYPE NativeDevice
such that TYPE == CUdevice
for cuda etc?
Or do you mean using
backend_traits<backend::ext_oneapi_cuda>::return_type<device> NativeDevice
where backend_traits<backend::ext_oneapi_cuda>::return_type<device> ==CUdevice
?
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.
I was thinking the former, but we could do both while we are at it, just to make sure that the types adhere to both the backend traits and that the backend traits adhere to the expected type.
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.
Sounds good, how is the last commit for this?
Signed-off-by: JackAKirk <jack.kirk@codeplay.com>
Signed-off-by: JackAKirk <jack.kirk@codeplay.com>
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.
LGTM!
All CI backends (cuda, hip, l0, opencl) currently support these three types of interop, that are probably the most important cases, but a complete e2e test did not previously exist. opencl is also the only backend with an existing test-e2e that calls make_* for these three sycl objects.
This adds a short test-e2e corresponding to e.g. #10526.