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

Use new UR handles for opencl instead of casting mechanism #12172

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

Conversation

omarahmed1111
Copy link
Contributor

@omarahmed1111 omarahmed1111 commented Dec 14, 2023

This PR changes sycl-rt for opencl interop functionality to use the new UR handles layer for opencl, instead of relying on casting directly handles directly. So, it removes the 1:1 relation between UR/sycl-rt and opencl.

The changes in this PR involves:

  • Changing sycl interop API to not own the opencl native handle passed to sycl-rt by default. SYCL application could be still using the native opencl handle outside of SYCL-RT.
  • Remove the specialized opencl retains in sycl-rt as this will retain UR objects which are already owned by sycl-rt to make it and release it. Before this PR, this was needed as sycl-rt was being passed the opencl handle itself so it could have released it while still being used in the sycl application so we needed to retain it but this PR removes the 1:1 mapping between UR handles and OpenCL handles so this should no longer be needed.
  • Moves SetArgForLocalAccessor.cpp and InteropKernelEnqueue.cpp unittests to be DeprecatedFeatures/set_arg_interop.cpp e2e-test. For more context refer to this comment

UR PR

@omarahmed1111 omarahmed1111 force-pushed the Testing-adding-handles-to-opencl branch from 3acda17 to f3f6360 Compare December 14, 2023 11:44
@omarahmed1111 omarahmed1111 force-pushed the Testing-adding-handles-to-opencl branch 3 times, most recently from 49e2da4 to 69c4f49 Compare December 15, 2023 12:24
@omarahmed1111 omarahmed1111 force-pushed the Testing-adding-handles-to-opencl branch from 69c4f49 to e95d890 Compare December 15, 2023 15:28
@omarahmed1111 omarahmed1111 force-pushed the Testing-adding-handles-to-opencl branch 3 times, most recently from bfe155f to 643dcf3 Compare December 18, 2023 17:59
@omarahmed1111 omarahmed1111 force-pushed the Testing-adding-handles-to-opencl branch from 643dcf3 to 503faa9 Compare December 19, 2023 10:39
@omarahmed1111 omarahmed1111 force-pushed the Testing-adding-handles-to-opencl branch from 503faa9 to 9eed560 Compare December 19, 2023 12:42
@omarahmed1111 omarahmed1111 force-pushed the Testing-adding-handles-to-opencl branch from 9eed560 to de060f6 Compare December 19, 2023 14:48
@omarahmed1111 omarahmed1111 force-pushed the Testing-adding-handles-to-opencl branch from de060f6 to 4ea421e Compare December 20, 2023 10:12
@omarahmed1111 omarahmed1111 force-pushed the Testing-adding-handles-to-opencl branch from 4ea421e to ce846a0 Compare December 20, 2023 12:11
@kbenzie kbenzie force-pushed the Testing-adding-handles-to-opencl branch from 756d15a to 2f89652 Compare January 31, 2025 17:08
@kbenzie
Copy link
Contributor

kbenzie commented Feb 5, 2025

@intel/llvm-reviewers-runtime please review

@@ -328,7 +328,7 @@ make_context(
const async_handler &Handler = {}) {
return detail::make_context(
detail::ur::cast<ur_native_handle_t>(BackendObject), Handler, Backend,
false /* KeepOwnership */);
true /* KeepOwnership */);
Copy link
Contributor

Choose a reason for hiding this comment

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

It seems like this changes the behavior for all paths, i.e. also L0, HIP and CUDA. Is that safe?

Copy link
Contributor

Choose a reason for hiding this comment

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

I'll look into this.

// twice, so there'll be 6 or 7 calls.
ASSERT_EQ(TestCounter, 6 + DeviceRetainCounter - 1)
// Interop object shouldn't be owned by sycl. So, get_native shouldn't retain
// native handles.
Copy link
Contributor

Choose a reason for hiding this comment

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

According to the SYCL 2020 specification's OpenCL backend section:

The lifetime of a SYCL runtime class that encapsulates an OpenCL opaque type and the instance of that opaque type retrieved via the get_native() free function are not tied in either direction given correct usage of OpenCL reference counting. For example if a user were to retrieve a cl_command_queue instance from a SYCL queue instance and then immediately destroy the SYCL queue instance, the cl_command_queue instance is still valid. Or if a user were to construct a SYCL queue instance from a cl_command_queue instance and then immediately release the cl_command_queue instance, the SYCL queue instance is still valid.

Note that a SYCL runtime class that encapsulates an OpenCL opaque type is not responsible for any incorrect use of OpenCL reference counting outside of the SYCL runtime. For example if a user were to retrieve a cl_command_queue instance from a SYCL queue instance and then release the cl_command_queue instance more than once without any prior retain then the SYCL queue instance that the cl_command_queue instance was retrieved from is now undefined.

Based on that, it seems like the old behavior was correct. Unless the UR backend makes sure of this implicitly now?

Copy link
Contributor

Choose a reason for hiding this comment

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

I believe there were some lifetime issues exposed as part of these changes, I'll need to investigate this further since Omar has changed teams.

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