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

RossBrunton · 2025-02-14T14:57:51Z

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 · 2025-03-17T15:53:57Z

@cperkinsintel Do you still have concerns about this approach?

@aelovikov-intel I'm not sure what testing could be done here. If my understanding is correct, only one "OpenCL" implementation may be loaded at a time, and if that were unloaded in some way it would break a lot of things internal to the OpenCL UR backend.

@nrspruit After an embarrassing amount of time trying to get Windows to work, CI seems to be working now.

aelovikov-intel · 2025-03-17T22:46:53Z

@aelovikov-intel I'm not sure what testing could be done here. If my understanding is correct, only one "OpenCL" implementation may be loaded at a time, and if that were unloaded in some way it would break a lot of things internal to the OpenCL UR backend.

I think the answer to my question is that ICD loader "unifies" all available OpenCL implementations in the system and properly dispatches to the correct one when called. As such, that function pointer is the same for both OpenCL CPU and GPU devices (and is provided by the ICD loader itself) even though the actual implementations are different.

RossBrunton · 2025-03-18T11:37:47Z

@aelovikov-intel I'm not sure what testing could be done here. If my understanding is correct, only one "OpenCL" implementation may be loaded at a time, and if that were unloaded in some way it would break a lot of things internal to the OpenCL UR backend.

I think the answer to my question is that ICD loader "unifies" all available OpenCL implementations in the system and properly dispatches to the correct one when called. As such, that function pointer is the same for both OpenCL CPU and GPU devices (and is provided by the ICD loader itself) even though the actual implementations are different.

That's my understanding as well; each handle type in OpenCL has a field for the "implementation" which points to a struct containing function pointers for each cl method. The ICD implements cl functions such that they dispatch to the appropriate function pointer for the specific handle.

On Linux, running gdb on clinfo shows that only one libOpenCL.so is loaded even though I have multiple opencl implementations installed.

RossBrunton · 2025-03-19T15:47:15Z

@intel/llvm-reviewers-runtime Can I get this reviewed? It's blocking a few UR improvements I want to merge.

aelovikov-intel · 2025-03-19T16:33:57Z

sycl/include/sycl/detail/os_util.hpp

+#define __SYCL_OCL_GET_FUNCTION(FN)                                            \
+  (sycl::_V1::detail::dynLookupFunction<decltype(FN)>("OpenCL",                \
+                                                      "libOpenCL.so", #FN))


IMO, this generalization is unnecessary at this point. Can we just have a non-macro

template <typename FnTy> FnTy *get_ocl_func(const char *FuncName) { ... } void *get_ocl_func_impl(const char *FuncName);

? All the win/lin dispatch for the shared library name can be done inside libsycl.so/.dll

Maybe even

void *get_ocl_func_impl(const char *FuncName); template <typename... Tys> auto call_ocl(const char *func, Tys&& ...args);

Your get_ocl_func requires specifying the function name twice; once as a string and once as a value for decltype(x). Without that, callers would have to do something like:

void *fn = reinterpret_cast<decltype(clDoThing)>("clDoThing");

which I think is less ergonomic than a macro.

call_ocl could work, but it looks like it relies on the programmer getting the signature exactly correct, and gives no feedback if they get it wrong.

How about __SYCL_OCL_CALL macro then?

Yes, __SYCL_OCL_CALL looks much nicer. Updated.

sycl/unittests/helpers/UrMock.hpp

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 · 2025-03-21T12:20:33Z

@intel/llvm-gatekeepers Please merge.

sarnex · 2025-03-21T14:22:31Z

@RossBrunton @kbenzie Build is failing in Windows postcommit

FAILED: tools/sycl/source/CMakeFiles/sycl_object.dir/detail/os_util.cpp.obj 
ccache C:\PROGRA~2\Intel\oneAPI\compiler\latest\bin\icx.exe  /nologo /TP -DCL_TARGET_OPENCL_VERSION=300 -DSYCL2020_DISABLE_DEPRECATION_WARNINGS -DSYCL_EXT_JIT_ENABLE -DSYCL_RT_ZSTD_NOT_AVAIABLE -DUNICODE -DXPTI_ENABLE_INSTRUMENTATION -DXPTI_STATIC_LIBRARY -D_CRT_NONSTDC_NO_DEPRECATE -D_CRT_NONSTDC_NO_WARNINGS -D_CRT_SECURE_NO_DEPRECATE -D_CRT_SECURE_NO_WARNINGS -D_GLIBCXX_ASSERTIONS -D_SCL_SECURE_NO_DEPRECATE -D_SCL_SECURE_NO_WARNINGS -D_UNICODE -D__STDC_CONSTANT_MACROS -D__STDC_FORMAT_MACROS -D__STDC_LIMIT_MACROS -D__SYCL_BUILD_SYCL_DLL -D__SYCL_INTERNAL_API -ID:\github\_work\llvm\llvm\build\tools\sycl\source -ID:\github\_work\llvm\llvm\src\sycl\source -ID:\github\_work\llvm\llvm\build\include -ID:\github\_work\llvm\llvm\src\llvm\include -ID:\github\_work\llvm\llvm\src\xpti\include -ID:\github\_work\llvm\llvm\src\sycl\ur_win_proxy_loader -ID:\github\_work\llvm\llvm\src\sycl\include -ID:\github\_work\llvm\llvm\build\_deps\boost_unordered-src\include -ID:\github\_work\llvm\llvm\build\_deps\boost_assert-src\include -ID:\github\_work\llvm\llvm\build\_deps\boost_config-src\include -ID:\github\_work\llvm\llvm\build\_deps\boost_container_hash-src\include -ID:\github\_work\llvm\llvm\build\_deps\boost_core-src\include -ID:\github\_work\llvm\llvm\build\_deps\boost_describe-src\include -ID:\github\_work\llvm\llvm\build\_deps\boost_mp11-src\include -ID:\github\_work\llvm\llvm\build\_deps\boost_predef-src\include -ID:\github\_work\llvm\llvm\build\_deps\boost_static_assert-src\include -ID:\github\_work\llvm\llvm\build\_deps\boost_throw_exception-src\include -ID:\github\_work\llvm\llvm\src\sycl-jit\common\include -ID:\github\_work\llvm\llvm\src\sycl-jit\jit-compiler\include -ID:\github\_work\llvm\llvm\src\unified-runtime\include -ID:\github\_work\llvm\llvm\build\_deps\opencl-headers-src /WX /W4 /fp:precise /clang:-Wno-nonportable-include-path /clang:-Wno-cast-function-type-mismatch /Zc:inline /Zc:__cplusplus /Oi /bigobj /permissive- /W4 -wd4141 -wd4146 -wd4244 -wd4267 -wd4291 -wd4351 -wd4456 -wd4457 -wd4458 -wd4459 -wd4503 -wd4624 -wd4722 -wd4100 -wd4127 -wd4512 -wd4505 -wd4610 -wd4510 -wd4702 -wd4245 -wd4706 -wd4310 -wd4701 -wd4703 -wd4389 -wd4611 -wd4805 -wd4204 -wd4577 -wd4091 -wd4592 -wd4319 -wd4709 -wd5105 -wd4324 -wd4251 -wd4275 -w14062 -we4238 /Gw /EHsc /Zi -Wno-covered-switch-default /O2 /Ob2  -Qstd:c++17 -MD   -wd4996 -UNDEBUG /MD -Winstantiation-after-specialization -QMD -QMT tools\sycl\source\CMakeFiles\sycl_object.dir\detail\os_util.cpp.obj -QMF tools\sycl\source\CMakeFiles\sycl_object.dir\detail\os_util.cpp.obj.d /Fotools\sycl\source\CMakeFiles\sycl_object.dir\detail\os_util.cpp.obj /Fdtools\sycl\source\CMakeFiles\sycl_object.dir\ -c D:\github\_work\llvm\llvm\src\sycl\source\detail\os_util.cpp
D:\github\_work\llvm\llvm\src\sycl\source\detail\os_util.cpp(324,10): error: implicit conversion between pointer-to-function and pointer-to-object is a Microsoft extension [-Werror,-Wmicrosoft-cast]
  324 |   return retVal;
      |          ^~~~~~

Can you please fix asap or revert? Thx

#17016 (comment)

kbenzie · 2025-03-21T14:45:45Z

@RossBrunton @kbenzie Build is failing in Windows postcommit

FAILED: tools/sycl/source/CMakeFiles/sycl_object.dir/detail/os_util.cpp.obj 
ccache C:\PROGRA~2\Intel\oneAPI\compiler\latest\bin\icx.exe  /nologo /TP -DCL_TARGET_OPENCL_VERSION=300 -DSYCL2020_DISABLE_DEPRECATION_WARNINGS -DSYCL_EXT_JIT_ENABLE -DSYCL_RT_ZSTD_NOT_AVAIABLE -DUNICODE -DXPTI_ENABLE_INSTRUMENTATION -DXPTI_STATIC_LIBRARY -D_CRT_NONSTDC_NO_DEPRECATE -D_CRT_NONSTDC_NO_WARNINGS -D_CRT_SECURE_NO_DEPRECATE -D_CRT_SECURE_NO_WARNINGS -D_GLIBCXX_ASSERTIONS -D_SCL_SECURE_NO_DEPRECATE -D_SCL_SECURE_NO_WARNINGS -D_UNICODE -D__STDC_CONSTANT_MACROS -D__STDC_FORMAT_MACROS -D__STDC_LIMIT_MACROS -D__SYCL_BUILD_SYCL_DLL -D__SYCL_INTERNAL_API -ID:\github\_work\llvm\llvm\build\tools\sycl\source -ID:\github\_work\llvm\llvm\src\sycl\source -ID:\github\_work\llvm\llvm\build\include -ID:\github\_work\llvm\llvm\src\llvm\include -ID:\github\_work\llvm\llvm\src\xpti\include -ID:\github\_work\llvm\llvm\src\sycl\ur_win_proxy_loader -ID:\github\_work\llvm\llvm\src\sycl\include -ID:\github\_work\llvm\llvm\build\_deps\boost_unordered-src\include -ID:\github\_work\llvm\llvm\build\_deps\boost_assert-src\include -ID:\github\_work\llvm\llvm\build\_deps\boost_config-src\include -ID:\github\_work\llvm\llvm\build\_deps\boost_container_hash-src\include -ID:\github\_work\llvm\llvm\build\_deps\boost_core-src\include -ID:\github\_work\llvm\llvm\build\_deps\boost_describe-src\include -ID:\github\_work\llvm\llvm\build\_deps\boost_mp11-src\include -ID:\github\_work\llvm\llvm\build\_deps\boost_predef-src\include -ID:\github\_work\llvm\llvm\build\_deps\boost_static_assert-src\include -ID:\github\_work\llvm\llvm\build\_deps\boost_throw_exception-src\include -ID:\github\_work\llvm\llvm\src\sycl-jit\common\include -ID:\github\_work\llvm\llvm\src\sycl-jit\jit-compiler\include -ID:\github\_work\llvm\llvm\src\unified-runtime\include -ID:\github\_work\llvm\llvm\build\_deps\opencl-headers-src /WX /W4 /fp:precise /clang:-Wno-nonportable-include-path /clang:-Wno-cast-function-type-mismatch /Zc:inline /Zc:__cplusplus /Oi /bigobj /permissive- /W4 -wd4141 -wd4146 -wd4244 -wd4267 -wd4291 -wd4351 -wd4456 -wd4457 -wd4458 -wd4459 -wd4503 -wd4624 -wd4722 -wd4100 -wd4127 -wd4512 -wd4505 -wd4610 -wd4510 -wd4702 -wd4245 -wd4706 -wd4310 -wd4701 -wd4703 -wd4389 -wd4611 -wd4805 -wd4204 -wd4577 -wd4091 -wd4592 -wd4319 -wd4709 -wd5105 -wd4324 -wd4251 -wd4275 -w14062 -we4238 /Gw /EHsc /Zi -Wno-covered-switch-default /O2 /Ob2  -Qstd:c++17 -MD   -wd4996 -UNDEBUG /MD -Winstantiation-after-specialization -QMD -QMT tools\sycl\source\CMakeFiles\sycl_object.dir\detail\os_util.cpp.obj -QMF tools\sycl\source\CMakeFiles\sycl_object.dir\detail\os_util.cpp.obj.d /Fotools\sycl\source\CMakeFiles\sycl_object.dir\detail\os_util.cpp.obj /Fdtools\sycl\source\CMakeFiles\sycl_object.dir\ -c D:\github\_work\llvm\llvm\src\sycl\source\detail\os_util.cpp
D:\github\_work\llvm\llvm\src\sycl\source\detail\os_util.cpp(324,10): error: implicit conversion between pointer-to-function and pointer-to-object is a Microsoft extension [-Werror,-Wmicrosoft-cast]
  324 |   return retVal;
      |          ^~~~~~

Can you please fix asap or revert? Thx

I've created #17574 which I hope will resolve this

#17016 (comment)

… OpenCL functions (intel#17016)" This reverts commit e1cf106. In testing, it turns out a number of people link against `libOpenCL.so.1` rather than `libOpenCL.so`, which is considered a seperate library by the linker. Reverting this change for now while we consider the best option.

RossBrunton had a problem deploying to WindowsCILock February 14, 2025 14:58 — with GitHub Actions Error

RossBrunton force-pushed the ross/urtocl branch from 8a1e5a3 to d3e930c Compare February 14, 2025 15:05

RossBrunton had a problem deploying to WindowsCILock February 14, 2025 15:06 — with GitHub Actions Failure

RossBrunton force-pushed the ross/urtocl branch from d3e930c to cd73105 Compare February 14, 2025 15:55

RossBrunton had a problem deploying to WindowsCILock February 14, 2025 15:56 — with GitHub Actions Error

RossBrunton force-pushed the ross/urtocl branch from cd73105 to dae0641 Compare February 14, 2025 16:02

RossBrunton had a problem deploying to WindowsCILock February 14, 2025 16:04 — with GitHub Actions Failure

RossBrunton force-pushed the ross/urtocl branch from dae0641 to 7664104 Compare February 14, 2025 17:02

RossBrunton had a problem deploying to WindowsCILock February 14, 2025 17:03 — with GitHub Actions Error

RossBrunton force-pushed the ross/urtocl branch from 7664104 to 6b55d56 Compare February 14, 2025 17:15

RossBrunton had a problem deploying to WindowsCILock February 14, 2025 17:16 — with GitHub Actions Error

RossBrunton force-pushed the ross/urtocl branch from 6b55d56 to 38563a4 Compare February 14, 2025 17:18

RossBrunton had a problem deploying to WindowsCILock February 14, 2025 17:19 — with GitHub Actions Failure

RossBrunton force-pushed the ross/urtocl branch from 38563a4 to 55f9034 Compare February 17, 2025 14:27

RossBrunton had a problem deploying to WindowsCILock February 17, 2025 14:28 — with GitHub Actions Failure

RossBrunton force-pushed the ross/urtocl branch 2 times, most recently from 85c448c to 93ce35e Compare February 18, 2025 16:03

RossBrunton had a problem deploying to WindowsCILock February 18, 2025 16:04 — with GitHub Actions Failure

RossBrunton force-pushed the ross/urtocl branch from 93ce35e to 7c29c52 Compare February 19, 2025 12:13

RossBrunton had a problem deploying to WindowsCILock February 19, 2025 12:15 — with GitHub Actions Failure

RossBrunton force-pushed the ross/urtocl branch from 7c29c52 to d16b32d Compare February 19, 2025 14:06

RossBrunton had a problem deploying to WindowsCILock February 19, 2025 14:07 — with GitHub Actions Failure

RossBrunton force-pushed the ross/urtocl branch from d16b32d to 4b996d2 Compare February 20, 2025 12:11

RossBrunton had a problem deploying to WindowsCILock February 20, 2025 12:12 — with GitHub Actions Failure

RossBrunton force-pushed the ross/urtocl branch 2 times, most recently from 688137b to 2238fb5 Compare February 20, 2025 16:10

RossBrunton had a problem deploying to WindowsCILock February 20, 2025 16:12 — with GitHub Actions Failure

RossBrunton had a problem deploying to WindowsCILock February 21, 2025 13:04 — with GitHub Actions Error

RossBrunton force-pushed the ross/urtocl branch from a73f57f to 18a4a0c Compare February 24, 2025 13:20

RossBrunton temporarily deployed to WindowsCILock March 13, 2025 12:33 — with GitHub Actions Inactive

RossBrunton force-pushed the ross/urtocl branch from a91b855 to 1cd11bc Compare March 14, 2025 12:37

RossBrunton temporarily deployed to WindowsCILock March 14, 2025 12:37 — with GitHub Actions Inactive

RossBrunton temporarily deployed to WindowsCILock March 14, 2025 13:43 — with GitHub Actions Inactive

RossBrunton force-pushed the ross/urtocl branch from 1cd11bc to 75bbc1d Compare March 17, 2025 13:54

RossBrunton temporarily deployed to WindowsCILock March 17, 2025 13:54 — with GitHub Actions Inactive

RossBrunton temporarily deployed to WindowsCILock March 17, 2025 14:43 — with GitHub Actions Inactive

aelovikov-intel reviewed Mar 19, 2025

View reviewed changes

sycl/unittests/helpers/UrMock.hpp Outdated Show resolved Hide resolved

aelovikov-intel approved these changes Mar 19, 2025

View reviewed changes

RossBrunton force-pushed the ross/urtocl branch from 75bbc1d to 4c0d2be Compare March 20, 2025 10:56

RossBrunton temporarily deployed to WindowsCILock March 20, 2025 10:57 — with GitHub Actions Inactive

RossBrunton temporarily deployed to WindowsCILock March 20, 2025 11:52 — with GitHub Actions Inactive

aelovikov-intel approved these changes Mar 20, 2025

View reviewed changes

kbenzie merged commit e1cf106 into intel:sycl Mar 21, 2025
32 of 34 checks passed

kbenzie added a commit that referenced this pull request Mar 21, 2025

[UR] Fix post commit Windows failure

3eee306

#17016 (comment)

kbenzie mentioned this pull request Mar 21, 2025

[SYCL][UR] Fix post commit Windows failure #17574

Merged

sarnex pushed a commit that referenced this pull request Mar 21, 2025

[SYCL][UR] Fix post commit Windows failure (#17574)

cb38c59

#17016 (comment)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

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

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

Uh oh!

RossBrunton commented Feb 14, 2025

Uh oh!

RossBrunton commented Mar 17, 2025

Uh oh!

aelovikov-intel commented Mar 17, 2025

Uh oh!

RossBrunton commented Mar 18, 2025

Uh oh!

RossBrunton commented Mar 19, 2025

Uh oh!

aelovikov-intel Mar 19, 2025 •

edited

Loading

Uh oh!

RossBrunton Mar 19, 2025

Uh oh!

aelovikov-intel Mar 19, 2025

Uh oh!

RossBrunton Mar 20, 2025

Uh oh!

Uh oh!

RossBrunton commented Mar 21, 2025

Uh oh!

Uh oh!

sarnex commented Mar 21, 2025

Uh oh!

kbenzie commented Mar 21, 2025

Uh oh!

Uh oh!

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

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

Uh oh!

Conversation

RossBrunton commented Feb 14, 2025

Uh oh!

RossBrunton commented Mar 17, 2025

Uh oh!

aelovikov-intel commented Mar 17, 2025

Uh oh!

RossBrunton commented Mar 18, 2025

Uh oh!

RossBrunton commented Mar 19, 2025

Uh oh!

aelovikov-intel Mar 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

RossBrunton Mar 19, 2025

Choose a reason for hiding this comment

Uh oh!

aelovikov-intel Mar 19, 2025

Choose a reason for hiding this comment

Uh oh!

RossBrunton Mar 20, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

RossBrunton commented Mar 21, 2025

Uh oh!

Uh oh!

sarnex commented Mar 21, 2025

Uh oh!

kbenzie commented Mar 21, 2025

Uh oh!

Uh oh!

aelovikov-intel Mar 19, 2025 •

edited

Loading