-
Notifications
You must be signed in to change notification settings - Fork 730
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
base: sycl
Are you sure you want to change the base?
Conversation
Signed-off-by: JackAKirk <[email protected]>
Signed-off-by: JackAKirk <[email protected]>
Signed-off-by: JackAKirk <[email protected]>
Signed-off-by: JackAKirk <[email protected]>
…kAKirk/llvm into test-ur-hip-context-refactor
I'll update the FetchUnifiedRuntime to resolve conflicts once approvals are given and oneapi-src/unified-runtime#1830 has been merged. |
Hi @uditagarwal97 |
@@ -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); |
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'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?
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 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]>
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.
Thanks for making the changes!
SYCL RT changes LGTM!
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.