Skip to content

[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

Merged
merged 9 commits into from
Sep 29, 2023

Conversation

JackAKirk
Copy link
Contributor

@JackAKirk JackAKirk commented Sep 25, 2023

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.

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>
@JackAKirk JackAKirk temporarily deployed to WindowsCILock September 25, 2023 11:01 — with GitHub Actions Inactive
@jinz2014
Copy link
Contributor

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 ?
Could "IDevice" become "InteropDevice" ?

Thanks

@JackAKirk
Copy link
Contributor Author

JackAKirk commented Sep 26, 2023

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 ? Could "IDevice" become "InteropDevice" ?

Thanks

  • The reason it is SYCL_EXT_ONEAPI_BACKEND_CUDA_EXPERIMENTAL is because the cuda backend spec is in an experimental state. I don't think it is very important, but the EXPERIMENTAL in this macro is the most accurate naming IMO.
  • Initially I using the naming InteropDevice, then switched to IDevice because the point is that this sycl device is created from a native device; "InteropDevice" didn't exactly sound like a correct naming since it referred to a sycl device. However I didn't think of a better name so I simply shortened it. If you prefer I will use InteropDevice etc instead.

Also tried to fix llvm testing instructions.

Signed-off-by: JackAKirk <jack.kirk@codeplay.com>
@JackAKirk JackAKirk temporarily deployed to WindowsCILock September 26, 2023 12:42 — with GitHub Actions Inactive
Signed-off-by: JackAKirk <jack.kirk@codeplay.com>
@JackAKirk JackAKirk temporarily deployed to WindowsCILock September 26, 2023 13:51 — with GitHub Actions Inactive
@jinz2014
Copy link
Contributor

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>
@JackAKirk JackAKirk temporarily deployed to WindowsCILock September 26, 2023 14:56 — with GitHub Actions Inactive
I don't think it is possible to support these backends in the same test
file.

Signed-off-by: JackAKirk <jack.kirk@codeplay.com>
@JackAKirk JackAKirk temporarily deployed to WindowsCILock September 26, 2023 16:36 — with GitHub Actions Inactive
@JackAKirk JackAKirk temporarily deployed to WindowsCILock September 26, 2023 16:51 — with GitHub Actions Inactive
Signed-off-by: JackAKirk <jack.kirk@codeplay.com>
@JackAKirk JackAKirk temporarily deployed to WindowsCILock September 26, 2023 19:25 — with GitHub Actions Inactive
@JackAKirk JackAKirk temporarily deployed to WindowsCILock September 26, 2023 19:37 — with GitHub Actions Inactive
@JackAKirk JackAKirk marked this pull request as ready for review September 27, 2023 10:07
@JackAKirk JackAKirk requested a review from a team as a code owner September 27, 2023 10:07
int main() {

device Device;
auto NativeDevice = get_native<BACKEND>(Device);
Copy link
Contributor

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.

Copy link
Contributor Author

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?

Copy link
Contributor

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.

Copy link
Contributor Author

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>
@JackAKirk JackAKirk temporarily deployed to WindowsCILock September 28, 2023 12:25 — with GitHub Actions Inactive
@JackAKirk JackAKirk temporarily deployed to WindowsCILock September 28, 2023 12:40 — with GitHub Actions Inactive
Copy link
Contributor

@steffenlarsen steffenlarsen left a comment

Choose a reason for hiding this comment

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

LGTM!

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