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

[UR] Replace calls to UR in native handle functions to proper OpenCL functions #17016

Open
wants to merge 1 commit into
base: sycl
Choose a base branch
from

Conversation

RossBrunton
Copy link
Contributor

At various points, OpenCL native handles need to be retained to ensure
SYCL semantics. Previously, this relied on the fact that UR handles were
typecast CL handles and shared the same reference count.

However, the SYCL RT shouldn't assume this, so instead we call the
appropriate (dynamically looked-up) CL functions on the native handles
instead.

This is in preperation for oneapi-src/unified-runtime#1176 .

This change should also have no observable effect for SYCL code; there
is no change in lifetime semantics.

@RossBrunton RossBrunton force-pushed the ross/urtocl branch 2 times, most recently from 85c448c to 93ce35e Compare February 18, 2025 16:03
@RossBrunton RossBrunton force-pushed the ross/urtocl branch 2 times, most recently from 688137b to 2238fb5 Compare February 20, 2025 16:10
At various points, OpenCL native handles need to be retained to ensure
SYCL semantics. Previously, this relied on the fact that UR handles were
typecast CL handles and shared the same reference count.

However, the SYCL RT shouldn't assume this, so instead we call the
appropriate (dynamically looked-up) CL functions on the native handles
instead.

This is in preperation for oneapi-src/unified-runtime#1176 .

This change should also have no observable effect for SYCL code; there
is no change in lifetime semantics.
@RossBrunton
Copy link
Contributor Author

@intel/llvm-reviewers-runtime Looks like the only failures now are random ones, so marking this ready. Just to give some context on this change:

In the OpenCL backend, UR handles are just CL handles. So the OpenCL reference count would be used as the UR one (that is, urThingRetain called clRetainThing on the same pointer). The "get_native" style functions exploited this to increase the OpenCL reference count before returning the handle (in SYCL, get_native on OpenCL increases the reference count whereas in UR it does not).

In a future UR change, we would like to break this equivalence so that urThingRetain does not necessarily call clRetainThing. This MR changes any use where urThingRetain was used to increment the cl reference count to directly call clRetainThing. Annoyingly, this means we have to load those functions dynamically from the OpenCL library (which will have been loaded already by UR).

These changes should not be externally visible; a user of SYCL should not encounter any change of behaviour as a result of this patch; get_native handles still need freed by clReleaseThing as before.

It does make testing much more complicated though, requiring a "fake" OpenCL for SYCL to call into that does nothing but track reference counts.

// The library must already have been loaded (perhaps by UR), otherwise this
// function throws.
template <typename fn>
fn *dynLookupFunction([[maybe_unused]] const char *WinName,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is there a more suitable place for this than common.hpp?

Copy link
Contributor

Choose a reason for hiding this comment

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

Probably it should go into os_util.hpp and/or os_util.cpp There are also windows_os_utils.hpp and friends in sycl/source/detail/

}
return reinterpret_cast<fn *>(retVal);
}
#define _OCL_GET_FUNCTION(FN) \
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is this macro name okay, or should it be _SYCL_DETAIL_OCL_GET_FUNCTION or similar?

Copy link
Contributor

Choose a reason for hiding this comment

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

__SYCL_OCL_GET_FUNCTION with two leading underscores

@RossBrunton RossBrunton changed the title RFC: [UR] Replace some UR retains with CL retains [UR] Replace calls to UR in native handle functions to proper OpenCL functions Feb 25, 2025
@aelovikov-intel
Copy link
Contributor

this means we have to load those functions dynamically from the OpenCL library

Why is that better than to provide urRetainNativeHandle?

@RossBrunton
Copy link
Contributor Author

this means we have to load those functions dynamically from the OpenCL library

Why is that better than to provide urRetainNativeHandle?

I considered that, but I don't think that makes sense to be part of the UR API; I think the main purpose of native handles is to be manipulated by code wanting to manipulate the backend directly. In addition, if there's urRetainNativeHandle it probably also makes sense for there to be urReleaseNativeHandle, and overall a lot of added API entry points. We'd also need to define what "Retain" and "Release" mean for targets other than OpenCL, and potentially have error checking and validation. It just feels like a lot of overhead for what, in theory, should just be a call to a single OpenCL function.

@aelovikov-intel
Copy link
Contributor

Is there a unittest when we have several OpenCL platforms available on the system? We need to ensure that dynamic library corresponds to the sycl device/queue/context/etc. we are working with.

@cperkinsintel
Copy link
Contributor

Adding @kbenzie and @nrspruit just to make sure everyone is on the same page. From the long term view it looks like we went from OCL to PI to UR now back to OCL.

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