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][hip] Exception for unsupported get_native<sycl::context> #14476

Open
wants to merge 14 commits into
base: sycl
Choose a base branch
from

Conversation

JackAKirk
Copy link
Contributor

@JackAKirk JackAKirk commented Jul 8, 2024

Add exception for unsupported get_nativesycl::context specialization for HIP backend.
This was previously marked deprecated. We keep the specialization in order to give an error message to users.

Signed-off-by: JackAKirk <[email protected]>
@JackAKirk JackAKirk requested a review from a team as a code owner July 11, 2024 11:00
@JackAKirk JackAKirk changed the title [DO NOT MERGE] Test UR pr. [sycl][hip] Exception for unsupported get_native<sycl::context> Jul 11, 2024
@JackAKirk JackAKirk marked this pull request as draft July 12, 2024 10:09
@JackAKirk
Copy link
Contributor Author

JackAKirk commented Sep 20, 2024

I'll update the FetchUnifiedRuntime to resolve conflicts once approvals are given and oneapi-src/unified-runtime#1830 has been merged.

@JackAKirk JackAKirk marked this pull request as ready for review September 20, 2024 14:04
@JackAKirk
Copy link
Contributor Author

Hi @uditagarwal97
this is ready for review now, thanks.

@@ -51,7 +51,7 @@ int main() {
// backend-defined and specified in the backend specification.

hip_device = get_native<backend::ext_oneapi_hip>(Device);
// expected-warning@+1{{'get_native<sycl::backend::ext_oneapi_hip, sycl::context>' is deprecated: Context interop is deprecated for HIP. If a native context is required, use hipDevicePrimaryCtxRetain with a native device}}
// expected-no-diagnostics
hip_context = get_native<backend::ext_oneapi_hip>(Context);
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm curious about why don't we run this LIT test? Since call to get_native<backend::ext_oneapi_hip>(Context) should now be throwing an exception instead of a depreciation warning and to check the exception in LIT test, don't we have to run the test?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think that tests in sycl/test/basic_tests are never ran; all runtime tests are in sycl/test-e2e AFAIK. Good point though, I've updated the corresponding test-e2e test to check for the exception. I've also changed the errc from runtime to feature_not_supported.

Thanks.

Also made exception use feature_not_supported

Signed-off-by: JackAKirk <[email protected]>
Copy link
Contributor

@uditagarwal97 uditagarwal97 left a comment

Choose a reason for hiding this comment

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

Thanks for making the changes!
SYCL RT changes 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.

4 participants