From dae06418a1427fefb8e78ec1d778be725574638f Mon Sep 17 00:00:00 2001 From: Ross Brunton Date: Fri, 14 Feb 2025 14:53:05 +0000 Subject: [PATCH] [UR] Replace some UR retains with CL retains 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 https://github.com/oneapi-src/unified-runtime/pull/1176 . This change should also have no observable effect for SYCL code; there is no change in lifetime semantics. --- sycl/include/sycl/detail/common.hpp | 10 ++++++++++ sycl/source/backend.cpp | 6 +++--- sycl/source/detail/context_impl.cpp | 5 +++-- sycl/source/detail/device_image_impl.hpp | 6 ++++-- sycl/source/detail/device_impl.cpp | 7 ++++--- sycl/source/detail/event_impl.cpp | 4 ++-- sycl/source/detail/kernel_impl.hpp | 9 +++++---- sycl/source/detail/queue_impl.cpp | 6 ++++-- sycl/source/detail/queue_impl.hpp | 3 ++- sycl/source/detail/sycl_mem_obj_t.cpp | 10 ++++++---- sycl/source/device.cpp | 2 +- sycl/source/event.cpp | 3 +-- sycl/source/kernel.cpp | 2 +- 13 files changed, 46 insertions(+), 27 deletions(-) diff --git a/sycl/include/sycl/detail/common.hpp b/sycl/include/sycl/detail/common.hpp index 1243ae7536b8e..5f17426cad20b 100644 --- a/sycl/include/sycl/detail/common.hpp +++ b/sycl/include/sycl/detail/common.hpp @@ -8,6 +8,7 @@ #pragma once +#include #include // for __SYCL_ALWAYS_INLINE #include // for __SYCL_EXPORT @@ -379,6 +380,15 @@ static constexpr std::array RepeatValue(const T &Arg) { // classes. struct AllowCTADTag; +template fn *dynLookupFunction(const char *name) { + auto *retVal = reinterpret_cast(dlsym(RTLD_DEFAULT, name)); + assert(retVal); + return retVal; +} + +#define _DYN_LOOKUP_FUNCTION(FN) \ + (::sycl::_V1::detail::dynLookupFunction(#FN)) + } // namespace detail } // namespace _V1 } // namespace sycl diff --git a/sycl/source/backend.cpp b/sycl/source/backend.cpp index 2c876a570e3c6..481e124973b9d 100644 --- a/sycl/source/backend.cpp +++ b/sycl/source/backend.cpp @@ -181,7 +181,7 @@ __SYCL_EXPORT event make_event(ur_native_handle_t NativeHandle, std::make_shared(UrEvent, Context)); if (Backend == backend::opencl) - Adapter->call(UrEvent); + _DYN_LOOKUP_FUNCTION(clRetainEvent)(ur::cast(NativeHandle)); return Event; } @@ -205,7 +205,7 @@ make_kernel_bundle(ur_native_handle_t NativeHandle, "urProgramCreateWithNativeHandle resulted in a null program handle."); if (ContextImpl->getBackend() == backend::opencl) - Adapter->call(UrProgram); + _DYN_LOOKUP_FUNCTION(clRetainProgram)(ur::cast(NativeHandle)); std::vector ProgramDevices; uint32_t NumDevices = 0; @@ -352,7 +352,7 @@ kernel make_kernel(const context &TargetContext, &UrKernel); if (Backend == backend::opencl) - Adapter->call(UrKernel); + _DYN_LOOKUP_FUNCTION(clRetainKernel)(ur::cast(NativeHandle)); // Construct the SYCL queue from UR queue. return detail::createSyclObjFromImpl( diff --git a/sycl/source/detail/context_impl.cpp b/sycl/source/detail/context_impl.cpp index e527d0a0c46a8..d042acd64e589 100644 --- a/sycl/source/detail/context_impl.cpp +++ b/sycl/source/detail/context_impl.cpp @@ -303,10 +303,11 @@ context_impl::findMatchingDeviceImpl(ur_device_handle_t &DeviceUR) const { ur_native_handle_t context_impl::getNative() const { const auto &Adapter = getAdapter(); - if (getBackend() == backend::opencl) - Adapter->call(getHandleRef()); ur_native_handle_t Handle; Adapter->call(getHandleRef(), &Handle); + if (getBackend() == backend::opencl) { + _DYN_LOOKUP_FUNCTION(clRetainContext)(ur::cast(Handle)); + } return Handle; } diff --git a/sycl/source/detail/device_image_impl.hpp b/sycl/source/detail/device_image_impl.hpp index ac58b7b80f467..9a760b6e3de7e 100644 --- a/sycl/source/detail/device_image_impl.hpp +++ b/sycl/source/detail/device_image_impl.hpp @@ -300,11 +300,13 @@ class device_image_impl { const auto &ContextImplPtr = detail::getSyclObjImpl(MContext); const AdapterPtr &Adapter = ContextImplPtr->getAdapter(); - if (ContextImplPtr->getBackend() == backend::opencl) - Adapter->call(MProgram); ur_native_handle_t NativeProgram = 0; Adapter->call(MProgram, &NativeProgram); + if (ContextImplPtr->getBackend() == backend::opencl) { + auto *RetainFun = _DYN_LOOKUP_FUNCTION(clRetainProgram); + RetainFun(ur::cast(NativeProgram)); + } return NativeProgram; } diff --git a/sycl/source/detail/device_impl.cpp b/sycl/source/detail/device_impl.cpp index 5cb7fa1e29585..1d4fb1df534f6 100644 --- a/sycl/source/detail/device_impl.cpp +++ b/sycl/source/detail/device_impl.cpp @@ -98,7 +98,7 @@ bool device_impl::is_affinity_supported( cl_device_id device_impl::get() const { // TODO catch an exception and put it to list of asynchronous exceptions - getAdapter()->call(MDevice); + _DYN_LOOKUP_FUNCTION(clRetainDevice)(ur::cast(getNative())); return ur::cast(getNative()); } @@ -345,10 +345,11 @@ std::vector device_impl::create_sub_devices() const { ur_native_handle_t device_impl::getNative() const { auto Adapter = getAdapter(); - if (getBackend() == backend::opencl) - Adapter->call(getHandleRef()); ur_native_handle_t Handle; Adapter->call(getHandleRef(), &Handle); + if (getBackend() == backend::opencl) { + _DYN_LOOKUP_FUNCTION(clRetainDevice)(ur::cast(Handle)); + } return Handle; } diff --git a/sycl/source/detail/event_impl.cpp b/sycl/source/detail/event_impl.cpp index 520b4de1ae888..74693522130f9 100644 --- a/sycl/source/detail/event_impl.cpp +++ b/sycl/source/detail/event_impl.cpp @@ -500,10 +500,10 @@ ur_native_handle_t event_impl::getNative() { this->setHandle(UREvent); Handle = UREvent; } - if (MContext->getBackend() == backend::opencl) - Adapter->call(Handle); ur_native_handle_t OutHandle; Adapter->call(Handle, &OutHandle); + if (MContext->getBackend() == backend::opencl) + _DYN_LOOKUP_FUNCTION(clRetainEvent)(ur::cast(OutHandle)); return OutHandle; } diff --git a/sycl/source/detail/kernel_impl.hpp b/sycl/source/detail/kernel_impl.hpp index 1b07d866dcc4c..e4ba72a68a418 100644 --- a/sycl/source/detail/kernel_impl.hpp +++ b/sycl/source/detail/kernel_impl.hpp @@ -75,10 +75,10 @@ class kernel_impl { /// /// \return a valid cl_kernel instance cl_kernel get() const { - getAdapter()->call(MKernel); ur_native_handle_t nativeHandle = 0; getAdapter()->call(MKernel, &nativeHandle); + _DYN_LOOKUP_FUNCTION(clRetainKernel)(ur::cast(nativeHandle)); return ur::cast(nativeHandle); } @@ -179,12 +179,13 @@ class kernel_impl { ur_native_handle_t getNative() const { const AdapterPtr &Adapter = MContext->getAdapter(); - if (MContext->getBackend() == backend::opencl) - Adapter->call(MKernel); - ur_native_handle_t NativeKernel = 0; Adapter->call(MKernel, &NativeKernel); + if (MContext->getBackend() == backend::opencl) { + _DYN_LOOKUP_FUNCTION(clRetainKernel)(ur::cast(NativeKernel)); + } + return NativeKernel; } diff --git a/sycl/source/detail/queue_impl.cpp b/sycl/source/detail/queue_impl.cpp index d8ca3fb8c1544..38ae17796ebd7 100644 --- a/sycl/source/detail/queue_impl.cpp +++ b/sycl/source/detail/queue_impl.cpp @@ -731,8 +731,6 @@ void queue_impl::destructorNotification() { ur_native_handle_t queue_impl::getNative(int32_t &NativeHandleDesc) const { const AdapterPtr &Adapter = getAdapter(); - if (getContextImplPtr()->getBackend() == backend::opencl) - Adapter->call(MQueues[0]); ur_native_handle_t Handle{}; ur_queue_native_desc_t UrNativeDesc{UR_STRUCTURE_TYPE_QUEUE_NATIVE_DESC, nullptr, nullptr}; @@ -740,6 +738,10 @@ ur_native_handle_t queue_impl::getNative(int32_t &NativeHandleDesc) const { Adapter->call(MQueues[0], &UrNativeDesc, &Handle); + if (getContextImplPtr()->getBackend() == backend::opencl) { + _DYN_LOOKUP_FUNCTION(clRetainCommandQueue) + (ur::cast(Handle)); + } return Handle; } diff --git a/sycl/source/detail/queue_impl.hpp b/sycl/source/detail/queue_impl.hpp index bf18e97c50fca..c8f00b54940ac 100644 --- a/sycl/source/detail/queue_impl.hpp +++ b/sycl/source/detail/queue_impl.hpp @@ -273,10 +273,11 @@ class queue_impl { /// \return an OpenCL interoperability queue handle. cl_command_queue get() { - getAdapter()->call(MQueues[0]); ur_native_handle_t nativeHandle = 0; getAdapter()->call(MQueues[0], nullptr, &nativeHandle); + _DYN_LOOKUP_FUNCTION(clRetainCommandQueue) + (ur::cast(nativeHandle)); return ur::cast(nativeHandle); } diff --git a/sycl/source/detail/sycl_mem_obj_t.cpp b/sycl/source/detail/sycl_mem_obj_t.cpp index 25e092232ae7f..493cf6522ed62 100644 --- a/sycl/source/detail/sycl_mem_obj_t.cpp +++ b/sycl/source/detail/sycl_mem_obj_t.cpp @@ -57,8 +57,9 @@ SYCLMemObjT::SYCLMemObjT(ur_native_handle_t MemObject, make_error_code(errc::invalid), "Input context must be the same as the context of cl_mem"); - if (MInteropContext->getBackend() == backend::opencl) - Adapter->call(MInteropMemObject); + if (MInteropContext->getBackend() == backend::opencl) { + _DYN_LOOKUP_FUNCTION(clRetainMemObject)(ur::cast(MemObject)); + } } ur_mem_type_t getImageType(int Dimensions) { @@ -112,8 +113,9 @@ SYCLMemObjT::SYCLMemObjT(ur_native_handle_t MemObject, make_error_code(errc::invalid), "Input context must be the same as the context of cl_mem"); - if (MInteropContext->getBackend() == backend::opencl) - Adapter->call(MInteropMemObject); + if (MInteropContext->getBackend() == backend::opencl) { + _DYN_LOOKUP_FUNCTION(clRetainMemObject)(ur::cast(MemObject)); + } } void SYCLMemObjT::releaseMem(ContextImplPtr Context, void *MemAllocation) { diff --git a/sycl/source/device.cpp b/sycl/source/device.cpp index 13590e980841c..5844463f90c00 100644 --- a/sycl/source/device.cpp +++ b/sycl/source/device.cpp @@ -43,7 +43,7 @@ device::device(cl_device_id DeviceId) { auto Platform = detail::platform_impl::getPlatformFromUrDevice(Device, Adapter); impl = Platform->getOrMakeDeviceImpl(Device, Platform); - Adapter->call(impl->getHandleRef()); + _DYN_LOOKUP_FUNCTION(clRetainDevice)(DeviceId); } device::device(const device_selector &deviceSelector) { diff --git a/sycl/source/event.cpp b/sycl/source/event.cpp index df68777ca6df4..caed7b39294ad 100644 --- a/sycl/source/event.cpp +++ b/sycl/source/event.cpp @@ -30,8 +30,7 @@ event::event(cl_event ClEvent, const context &SyclContext) // This is a special interop constructor for OpenCL, so the event must be // retained. // TODO(pi2ur): Don't just cast from cl_event above - impl->getAdapter()->call( - detail::ur::cast(ClEvent)); + _DYN_LOOKUP_FUNCTION(clRetainEvent)(ClEvent); } bool event::operator==(const event &rhs) const { return rhs.impl == impl; } diff --git a/sycl/source/kernel.cpp b/sycl/source/kernel.cpp index b0055f74ef4bc..8adb7761cccbd 100644 --- a/sycl/source/kernel.cpp +++ b/sycl/source/kernel.cpp @@ -30,7 +30,7 @@ kernel::kernel(cl_kernel ClKernel, const context &SyclContext) { // This is a special interop constructor for OpenCL, so the kernel must be // retained. if (get_backend() == backend::opencl) { - impl->getAdapter()->call(hKernel); + _DYN_LOOKUP_FUNCTION(clRetainKernel)(ClKernel); } }