Skip to content

Commit e1cf106

Browse files
RossBruntonRossBrunton
authored
[UR] Replace calls to UR in native handle functions to proper OpenCL functions (#17016)
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.
1 parent 732abd1 commit e1cf106

25 files changed

+225
-42
lines changed

‎sycl/cmake/modules/AddSYCLUnitTest.cmake

Lines changed: 17 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,8 @@ macro(add_sycl_unittest test_dirname link_variant)
77
set(LLVM_REQUIRES_EH ON)
88
set(LLVM_REQUIRES_RTTI ON)
99

10+
get_target_property(SYCL_BINARY_DIR sycl-toolchain BINARY_DIR)
11+
1012
string(TOLOWER "${CMAKE_BUILD_TYPE}" build_type_lower)
1113
if (MSVC AND build_type_lower MATCHES "debug")
1214
set(sycl_obj_target "sycld_object")
@@ -59,7 +61,7 @@ macro(add_sycl_unittest test_dirname link_variant)
5961
SYCL_CONFIG_FILE_NAME=null.cfg
6062
SYCL_DEVICELIB_NO_FALLBACK=1
6163
SYCL_CACHE_DIR="${CMAKE_BINARY_DIR}/sycl_cache"
62-
"LD_LIBRARY_PATH=${CMAKE_BINARY_DIR}/lib:$ENV{LD_LIBRARY_PATH}"
64+
"LD_LIBRARY_PATH=${SYCL_BINARY_DIR}/unittests/lib:${CMAKE_BINARY_DIR}/lib:$ENV{LD_LIBRARY_PATH}"
6365
${CMAKE_CURRENT_BINARY_DIR}/${test_dirname}
6466
DEPENDS
6567
${test_dirname}
@@ -68,15 +70,28 @@ macro(add_sycl_unittest test_dirname link_variant)
6870

6971
add_dependencies(check-sycl-unittests check-sycl-${test_dirname})
7072

73+
if(WIN32)
74+
# Windows doesn't support LD_LIBRARY_PATH, so instead we copy the mock OpenCL binary next to the test and ensure
75+
# that the test itself links to OpenCL (rather than through ur_adapter_opencl.dll)
76+
set(mock_ocl ${CMAKE_CURRENT_BINARY_DIR}/OpenCL.dll)
77+
add_custom_command(TARGET ${test_dirname} POST_BUILD
78+
COMMAND ${CMAKE_COMMAND} -E copy $<TARGET_FILE:mockOpenCL> ${mock_ocl}
79+
DEPENDS mockOpenCL
80+
BYPRODUCTS ${mock_ocl}
81+
COMMAND_EXPAND_LISTS
82+
)
83+
endif()
84+
7185
target_link_libraries(${test_dirname}
7286
PRIVATE
87+
mockOpenCL
7388
LLVMTestingSupport
7489
OpenCL-Headers
7590
unified-runtime::mock
7691
${SYCL_LINK_LIBS}
7792
)
7893

79-
add_dependencies(${test_dirname} ur_adapter_mock)
94+
add_dependencies(${test_dirname} ur_adapter_mock mockOpenCL)
8095

8196
if(SYCL_ENABLE_EXTENSION_JIT)
8297
target_link_libraries(${test_dirname} PRIVATE sycl-jit)

‎sycl/include/sycl/detail/os_util.hpp

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -106,6 +106,24 @@ void fileTreeWalk(const std::string Path,
106106
std::function<void(const std::string)> Func,
107107
bool ignoreErrors = false);
108108

109+
void *dynLookup(const char *WinName, const char *LinName, const char *FunName);
110+
111+
// Look up a function name that was dynamically linked
112+
// This is used by the runtime where it needs to manipulate native handles (e.g.
113+
// retaining OpenCL handles). On Windows, the symbol name is looked up in
114+
// `WinName`. In Linux, it uses `LinName`.
115+
//
116+
// The library must already have been loaded (perhaps by UR), otherwise this
117+
// function throws a SYCL runtime exception.
118+
template <typename fn>
119+
fn *dynLookupFunction(const char *WinName, const char *LinName,
120+
const char *FunName) {
121+
return reinterpret_cast<fn *>(dynLookup(WinName, LinName, FunName));
122+
}
123+
#define __SYCL_OCL_CALL(FN, ...) \
124+
(sycl::_V1::detail::dynLookupFunction<decltype(FN)>( \
125+
"OpenCL", "libOpenCL.so", #FN)(__VA_ARGS__))
126+
109127
} // namespace detail
110128
} // namespace _V1
111129
} // namespace sycl

‎sycl/source/backend.cpp

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -181,7 +181,7 @@ __SYCL_EXPORT event make_event(ur_native_handle_t NativeHandle,
181181
std::make_shared<event_impl>(UrEvent, Context));
182182

183183
if (Backend == backend::opencl)
184-
Adapter->call<UrApiKind::urEventRetain>(UrEvent);
184+
__SYCL_OCL_CALL(clRetainEvent, ur::cast<cl_event>(NativeHandle));
185185
return Event;
186186
}
187187

@@ -205,7 +205,7 @@ make_kernel_bundle(ur_native_handle_t NativeHandle,
205205
"urProgramCreateWithNativeHandle resulted in a null program handle.");
206206

207207
if (ContextImpl->getBackend() == backend::opencl)
208-
Adapter->call<UrApiKind::urProgramRetain>(UrProgram);
208+
__SYCL_OCL_CALL(clRetainProgram, ur::cast<cl_program>(NativeHandle));
209209

210210
std::vector<ur_device_handle_t> ProgramDevices;
211211
uint32_t NumDevices = 0;
@@ -352,7 +352,7 @@ kernel make_kernel(const context &TargetContext,
352352
&UrKernel);
353353

354354
if (Backend == backend::opencl)
355-
Adapter->call<UrApiKind::urKernelRetain>(UrKernel);
355+
__SYCL_OCL_CALL(clRetainKernel, ur::cast<cl_kernel>(NativeHandle));
356356

357357
// Construct the SYCL queue from UR queue.
358358
return detail::createSyclObjFromImpl<kernel>(

‎sycl/source/detail/buffer_impl.cpp

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -87,7 +87,7 @@ buffer_impl::getNativeVector(backend BackendName) const {
8787
auto Adapter = Platform->getAdapter();
8888

8989
if (Platform->getBackend() == backend::opencl) {
90-
Adapter->call<UrApiKind::urMemRetain>(NativeMem);
90+
__SYCL_OCL_CALL(clRetainMemObject, ur::cast<cl_mem>(NativeMem));
9191
}
9292

9393
ur_native_handle_t Handle = 0;

‎sycl/source/detail/context_impl.cpp

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -303,10 +303,11 @@ context_impl::findMatchingDeviceImpl(ur_device_handle_t &DeviceUR) const {
303303

304304
ur_native_handle_t context_impl::getNative() const {
305305
const auto &Adapter = getAdapter();
306-
if (getBackend() == backend::opencl)
307-
Adapter->call<UrApiKind::urContextRetain>(getHandleRef());
308306
ur_native_handle_t Handle;
309307
Adapter->call<UrApiKind::urContextGetNativeHandle>(getHandleRef(), &Handle);
308+
if (getBackend() == backend::opencl) {
309+
__SYCL_OCL_CALL(clRetainContext, ur::cast<cl_context>(Handle));
310+
}
310311
return Handle;
311312
}
312313

‎sycl/source/detail/device_image_impl.hpp

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -300,11 +300,11 @@ class device_image_impl {
300300
const auto &ContextImplPtr = detail::getSyclObjImpl(MContext);
301301
const AdapterPtr &Adapter = ContextImplPtr->getAdapter();
302302

303-
if (ContextImplPtr->getBackend() == backend::opencl)
304-
Adapter->call<UrApiKind::urProgramRetain>(MProgram);
305303
ur_native_handle_t NativeProgram = 0;
306304
Adapter->call<UrApiKind::urProgramGetNativeHandle>(MProgram,
307305
&NativeProgram);
306+
if (ContextImplPtr->getBackend() == backend::opencl)
307+
__SYCL_OCL_CALL(clRetainProgram, ur::cast<cl_program>(NativeProgram));
308308

309309
return NativeProgram;
310310
}

‎sycl/source/detail/device_impl.cpp

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -99,7 +99,7 @@ bool device_impl::is_affinity_supported(
9999

100100
cl_device_id device_impl::get() const {
101101
// TODO catch an exception and put it to list of asynchronous exceptions
102-
getAdapter()->call<UrApiKind::urDeviceRetain>(MDevice);
102+
__SYCL_OCL_CALL(clRetainDevice, ur::cast<cl_device_id>(getNative()));
103103
return ur::cast<cl_device_id>(getNative());
104104
}
105105

@@ -346,10 +346,11 @@ std::vector<device> device_impl::create_sub_devices() const {
346346

347347
ur_native_handle_t device_impl::getNative() const {
348348
auto Adapter = getAdapter();
349-
if (getBackend() == backend::opencl)
350-
Adapter->call<UrApiKind::urDeviceRetain>(getHandleRef());
351349
ur_native_handle_t Handle;
352350
Adapter->call<UrApiKind::urDeviceGetNativeHandle>(getHandleRef(), &Handle);
351+
if (getBackend() == backend::opencl) {
352+
__SYCL_OCL_CALL(clRetainDevice, ur::cast<cl_device_id>(Handle));
353+
}
353354
return Handle;
354355
}
355356

‎sycl/source/detail/event_impl.cpp

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -511,10 +511,10 @@ ur_native_handle_t event_impl::getNative() {
511511
this->setHandle(UREvent);
512512
Handle = UREvent;
513513
}
514-
if (MContext->getBackend() == backend::opencl)
515-
Adapter->call<UrApiKind::urEventRetain>(Handle);
516514
ur_native_handle_t OutHandle;
517515
Adapter->call<UrApiKind::urEventGetNativeHandle>(Handle, &OutHandle);
516+
if (MContext->getBackend() == backend::opencl)
517+
__SYCL_OCL_CALL(clRetainEvent, ur::cast<cl_event>(OutHandle));
518518
return OutHandle;
519519
}
520520

‎sycl/source/detail/kernel_impl.hpp

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -75,10 +75,10 @@ class kernel_impl {
7575
///
7676
/// \return a valid cl_kernel instance
7777
cl_kernel get() const {
78-
getAdapter()->call<UrApiKind::urKernelRetain>(MKernel);
7978
ur_native_handle_t nativeHandle = 0;
8079
getAdapter()->call<UrApiKind::urKernelGetNativeHandle>(MKernel,
8180
&nativeHandle);
81+
__SYCL_OCL_CALL(clRetainKernel, ur::cast<cl_kernel>(nativeHandle));
8282
return ur::cast<cl_kernel>(nativeHandle);
8383
}
8484

@@ -212,12 +212,12 @@ class kernel_impl {
212212
ur_native_handle_t getNative() const {
213213
const AdapterPtr &Adapter = MContext->getAdapter();
214214

215-
if (MContext->getBackend() == backend::opencl)
216-
Adapter->call<UrApiKind::urKernelRetain>(MKernel);
217-
218215
ur_native_handle_t NativeKernel = 0;
219216
Adapter->call<UrApiKind::urKernelGetNativeHandle>(MKernel, &NativeKernel);
220217

218+
if (MContext->getBackend() == backend::opencl)
219+
__SYCL_OCL_CALL(clRetainKernel, ur::cast<cl_kernel>(NativeKernel));
220+
221221
return NativeKernel;
222222
}
223223

‎sycl/source/detail/os_util.cpp

Lines changed: 33 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -291,6 +291,39 @@ size_t getDirectorySize(const std::string &Path, bool ignoreErrors) {
291291
return DirSizeVar;
292292
}
293293

294+
// Look up a function name that was dynamically linked
295+
// This is used by the runtime where it needs to manipulate native handles (e.g.
296+
// retaining OpenCL handles). On Windows, the symbol name is looked up in
297+
// `WinName`. In Linux, it uses `LinName`.
298+
//
299+
// The library must already have been loaded (perhaps by UR), otherwise this
300+
// function throws a SYCL runtime exception.
301+
void *dynLookup([[maybe_unused]] const char *WinName,
302+
[[maybe_unused]] const char *LinName, const char *FunName) {
303+
#ifdef __SYCL_RT_OS_WINDOWS
304+
auto handle = GetModuleHandleA(WinName);
305+
if (!handle) {
306+
throw sycl::exception(make_error_code(errc::runtime),
307+
std::string(WinName) + " library is not loaded");
308+
}
309+
auto *retVal = GetProcAddress(handle, FunName);
310+
#else
311+
auto handle = dlopen(LinName, RTLD_LAZY | RTLD_NOLOAD);
312+
if (!handle) {
313+
throw sycl::exception(make_error_code(errc::runtime),
314+
std::string(LinName) + " library is not loaded");
315+
}
316+
auto *retVal = dlsym(handle, FunName);
317+
dlclose(handle);
318+
#endif
319+
if (!retVal) {
320+
throw sycl::exception(make_error_code(errc::runtime),
321+
"Symbol " + std::string(FunName) +
322+
" could not be found");
323+
}
324+
return retVal;
325+
}
326+
294327
} // namespace detail
295328
} // namespace _V1
296329
} // namespace sycl

‎sycl/source/detail/queue_impl.cpp

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -725,15 +725,16 @@ void queue_impl::destructorNotification() {
725725

726726
ur_native_handle_t queue_impl::getNative(int32_t &NativeHandleDesc) const {
727727
const AdapterPtr &Adapter = getAdapter();
728-
if (getContextImplPtr()->getBackend() == backend::opencl)
729-
Adapter->call<UrApiKind::urQueueRetain>(MQueues[0]);
730728
ur_native_handle_t Handle{};
731729
ur_queue_native_desc_t UrNativeDesc{UR_STRUCTURE_TYPE_QUEUE_NATIVE_DESC,
732730
nullptr, nullptr};
733731
UrNativeDesc.pNativeData = &NativeHandleDesc;
734732

735733
Adapter->call<UrApiKind::urQueueGetNativeHandle>(MQueues[0], &UrNativeDesc,
736734
&Handle);
735+
if (getContextImplPtr()->getBackend() == backend::opencl)
736+
__SYCL_OCL_CALL(clRetainCommandQueue, ur::cast<cl_command_queue>(Handle));
737+
737738
return Handle;
738739
}
739740

‎sycl/source/detail/queue_impl.hpp

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -273,10 +273,10 @@ class queue_impl {
273273
/// \return an OpenCL interoperability queue handle.
274274

275275
cl_command_queue get() {
276-
getAdapter()->call<UrApiKind::urQueueRetain>(MQueues[0]);
277276
ur_native_handle_t nativeHandle = 0;
278277
getAdapter()->call<UrApiKind::urQueueGetNativeHandle>(MQueues[0], nullptr,
279278
&nativeHandle);
279+
__SYCL_OCL_CALL(clRetainCommandQueue, ur::cast<cl_command_queue>(nativeHandle));
280280
return ur::cast<cl_command_queue>(nativeHandle);
281281
}
282282

‎sycl/source/detail/sycl_mem_obj_t.cpp

Lines changed: 6 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -57,8 +57,9 @@ SYCLMemObjT::SYCLMemObjT(ur_native_handle_t MemObject,
5757
make_error_code(errc::invalid),
5858
"Input context must be the same as the context of cl_mem");
5959

60-
if (MInteropContext->getBackend() == backend::opencl)
61-
Adapter->call<UrApiKind::urMemRetain>(MInteropMemObject);
60+
if (MInteropContext->getBackend() == backend::opencl) {
61+
__SYCL_OCL_CALL(clRetainMemObject, ur::cast<cl_mem>(MemObject));
62+
}
6263
}
6364

6465
ur_mem_type_t getImageType(int Dimensions) {
@@ -112,8 +113,9 @@ SYCLMemObjT::SYCLMemObjT(ur_native_handle_t MemObject,
112113
make_error_code(errc::invalid),
113114
"Input context must be the same as the context of cl_mem");
114115

115-
if (MInteropContext->getBackend() == backend::opencl)
116-
Adapter->call<UrApiKind::urMemRetain>(MInteropMemObject);
116+
if (MInteropContext->getBackend() == backend::opencl) {
117+
__SYCL_OCL_CALL(clRetainMemObject, ur::cast<cl_mem>(MemObject));
118+
}
117119
}
118120

119121
void SYCLMemObjT::releaseMem(ContextImplPtr Context, void *MemAllocation) {

‎sycl/source/device.cpp

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -43,7 +43,7 @@ device::device(cl_device_id DeviceId) {
4343
auto Platform =
4444
detail::platform_impl::getPlatformFromUrDevice(Device, Adapter);
4545
impl = Platform->getOrMakeDeviceImpl(Device, Platform);
46-
Adapter->call<detail::UrApiKind::urDeviceRetain>(impl->getHandleRef());
46+
__SYCL_OCL_CALL(clRetainDevice, DeviceId);
4747
}
4848

4949
device::device(const device_selector &deviceSelector) {

‎sycl/source/event.cpp

Lines changed: 1 addition & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -29,9 +29,7 @@ event::event(cl_event ClEvent, const context &SyclContext)
2929
detail::ur::cast<ur_event_handle_t>(ClEvent), SyclContext)) {
3030
// This is a special interop constructor for OpenCL, so the event must be
3131
// retained.
32-
// TODO(pi2ur): Don't just cast from cl_event above
33-
impl->getAdapter()->call<detail::UrApiKind::urEventRetain>(
34-
detail::ur::cast<ur_event_handle_t>(ClEvent));
32+
__SYCL_OCL_CALL(clRetainEvent, ClEvent);
3533
}
3634

3735
bool event::operator==(const event &rhs) const { return rhs.impl == impl; }

‎sycl/source/kernel.cpp

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -30,7 +30,7 @@ kernel::kernel(cl_kernel ClKernel, const context &SyclContext) {
3030
// This is a special interop constructor for OpenCL, so the kernel must be
3131
// retained.
3232
if (get_backend() == backend::opencl) {
33-
impl->getAdapter()->call<detail::UrApiKind::urKernelRetain>(hKernel);
33+
__SYCL_OCL_CALL(clRetainKernel, ClKernel);
3434
}
3535
}
3636

‎sycl/test/Unit/lit.cfg.py

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -70,8 +70,11 @@ def find_shlibpath_var():
7070
for shlibpath_var in find_shlibpath_var():
7171
# in stand-alone builds, shlibdir is clang's build tree
7272
# while llvm_libs_dir is installed LLVM (and possibly older clang)
73+
# For unit tests, we have a "mock" OpenCL which needs to have
74+
# priority and so is at the start of the shlibpath list
7375
shlibpath = os.path.pathsep.join(
7476
(
77+
os.path.join(config.test_exec_root, "lib"),
7578
config.shlibdir,
7679
config.llvm_libs_dir,
7780
config.environment.get(shlibpath_var, ""),

‎sycl/unittests/CMakeLists.txt

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,8 @@ string(TOLOWER "${CMAKE_BUILD_TYPE}" build_type_lower)
2323

2424
include(AddSYCLUnitTest)
2525

26+
add_subdirectory(mock_opencl)
27+
2628
add_custom_target(check-sycl-unittests)
2729

2830
add_subdirectory(ur)

‎sycl/unittests/Extensions/CompositeDevice.cpp

Lines changed: 3 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,5 @@
11
#include "sycl/platform.hpp"
2+
#include <detail/device_impl.hpp>
23
#include <sycl/sycl.hpp>
34

45
#include <helpers/UrMock.hpp>
@@ -143,8 +144,7 @@ TEST(CompositeDeviceTest, PlatformExtOneAPIGetCompositeDevices) {
143144
// We don't expect to see COMPOSITE_DEVICE_1 here, because one of its
144145
// components (COMPONENT_DEVICE_D) is not available.
145146
ASSERT_EQ(Composites.size(), 1u);
146-
ASSERT_EQ(sycl::bit_cast<ur_device_handle_t>(
147-
sycl::get_native<sycl::backend::opencl>(Composites.front())),
147+
ASSERT_EQ(sycl::detail::getSyclObjImpl(Composites.front())->getHandleRef(),
148148
COMPOSITE_DEVICE_0);
149149
}
150150

@@ -162,8 +162,7 @@ TEST(CompositeDeviceTest, SYCLExtOneAPIExperimentalGetCompositeDevices) {
162162
// We don't expect to see COMPOSITE_DEVICE_1 here, because one of its
163163
// components (COMPONENT_DEVICE_D) is not available.
164164
ASSERT_EQ(Composites.size(), 1u);
165-
ASSERT_EQ(sycl::bit_cast<ur_device_handle_t>(
166-
sycl::get_native<sycl::backend::opencl>(Composites.front())),
165+
ASSERT_EQ(sycl::detail::getSyclObjImpl(Composites.front())->getHandleRef(),
167166
COMPOSITE_DEVICE_0);
168167
}
169168

0 commit comments

Comments
 (0)