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

[SYCL][CUDA][HIP] Deprecate context interop for CUDA and HIP #10975

Merged
merged 8 commits into from
Oct 23, 2023

Conversation

hdelan
Copy link
Contributor

@hdelan hdelan commented Aug 25, 2023

The sycl::context does not map clearly to a native context for CUDA and HIP backends. This is especially true now that we are adding support for multi device context #10737 . It would be good to start this deprecation process. PRs to oneMKL and oneDNN to follow

@hdelan hdelan requested a review from a team as a code owner August 25, 2023 16:43
@hdelan hdelan requested a review from cperkinsintel August 25, 2023 16:43
@hdelan hdelan force-pushed the deprecate-cuda-hip-context-interop branch from 2dde128 to 36fbb6e Compare August 30, 2023 15:23
@hdelan
Copy link
Contributor Author

hdelan commented Sep 4, 2023

Some further description:

The get_native method for a sycl::context currently maps to a single native CUcontext. The CUDA backend spec KhronosGroup/SYCL-Docs#420 for SYCL specifies that a sycl::context should map to a vector of native contexts. However this PR is still unmerged meaning this mapping cannot be integrated into the core API. For this reason we have two separate versions of get_native<backend::ext_oneapi_cuda, sycl::context>. One is the standard core SYCL spec version and the other can be found here https://github.com/intel/llvm/blob/sycl/sycl/include/sycl/ext/oneapi/experimental/backend/cuda.hpp#L38 which can be used if the macro SYCL_EXT_ONEAPI_BACKEND_CUDA_EXPERIMENTAL is defined.

This dual impl has been harmless up until now since the context has been constrained to a single device. However #10737 introduces behaviour that if we have two devices in a single platform (ie two CUDA devices) then the default context will contain them both. Therefore if users are using the experimental interface for get_native which returns a vec of CUcontexts, the correct behaviour will be guaranteed. Whereas if users are using the standard interface which returns a single CUcontext this get_native will return the native context of the first device in the sycl::context. This could lead to tricky bugs in user code as well as cases where only the first context in a sycl::context is usable, making the interop interface unintuitive and cumbersome.

For this reason I think it is better to deprecate the get_native interface for sycl::context for CUDA and HIP backends, and instead encourage users to get the native context by calling [cu|hip]DevicePrimaryCtxRetain with a native device.

@hdelan hdelan temporarily deployed to WindowsCILock September 28, 2023 09:46 — with GitHub Actions Inactive
@hdelan hdelan temporarily deployed to WindowsCILock September 28, 2023 10:12 — with GitHub Actions Inactive
@hdelan hdelan temporarily deployed to WindowsCILock September 28, 2023 13:19 — with GitHub Actions Inactive
Copy link
Contributor

@npmiller npmiller left a comment

Choose a reason for hiding this comment

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

LGTM

@hdelan
Copy link
Contributor Author

hdelan commented Oct 2, 2023

Ping @intel/llvm-gatekeepers can we merge this please?

@hdelan hdelan temporarily deployed to WindowsCILock October 2, 2023 16:16 — with GitHub Actions Inactive
@aelovikov-intel
Copy link
Contributor

Ping @intel/llvm-gatekeepers can we merge this please?

check-sycl has failed.

@hdelan
Copy link
Contributor Author

hdelan commented Oct 2, 2023

Ping @intel/llvm-gatekeepers can we merge this please?

check-sycl has failed.

It seems that interop-cuda.cpp is being run for the AMD triple, even though REQUIRES: cuda is being used. I am not sure what's happening here but will restart the CI.

@hdelan hdelan closed this Oct 2, 2023
@hdelan hdelan reopened this Oct 2, 2023
@hdelan hdelan temporarily deployed to WindowsCILock October 2, 2023 20:10 — with GitHub Actions Inactive
@hdelan hdelan temporarily deployed to WindowsCILock October 2, 2023 20:35 — with GitHub Actions Inactive
@hdelan
Copy link
Contributor Author

hdelan commented Oct 2, 2023

@aelovikov-intel test should be fixed now, but still surprising that this is running for the AMD triple

@hdelan hdelan temporarily deployed to WindowsCILock October 2, 2023 21:53 — with GitHub Actions Inactive
@aelovikov-intel
Copy link
Contributor

aelovikov-intel commented Oct 2, 2023

@aelovikov-intel test should be fixed now, but still surprising that this is running for the AMD triple

I think the semantics of LIT features might be different between sycl/test and sycl/test-e2e. #10635 could also be related somehow.

I'm not driver expert, but might it be that we specify some options to be used when targeting AMD, but then don't actually target it? IMO, that would be consistent with the "argument unused during compilation" warning we saw in that test run.

@hdelan hdelan temporarily deployed to WindowsCILock October 2, 2023 22:19 — with GitHub Actions Inactive
@hdelan hdelan closed this Oct 16, 2023
@hdelan hdelan reopened this Oct 16, 2023
@hdelan hdelan temporarily deployed to WindowsCILock October 16, 2023 16:56 — with GitHub Actions Inactive
@hdelan hdelan temporarily deployed to WindowsCILock October 16, 2023 18:03 — with GitHub Actions Inactive
@hdelan hdelan closed this Oct 23, 2023
@hdelan hdelan reopened this Oct 23, 2023
@hdelan hdelan temporarily deployed to WindowsCILock October 23, 2023 13:27 — with GitHub Actions Inactive
@hdelan hdelan temporarily deployed to WindowsCILock October 23, 2023 13:40 — with GitHub Actions Inactive
@hdelan hdelan temporarily deployed to WindowsCILock October 23, 2023 14:05 — with GitHub Actions Inactive
@againull againull merged commit e213fe2 into intel:sycl Oct 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.

6 participants