[SYCL] Add delayloading for ze_loader #11902

ykhatav · 2023-11-15T21:55:56Z

Currently, on Windows, ze_loader is loaded using load-time linking and since the OS loads it we don't have any control over the search path. To control the search path add /DELAYLOAD link option for ze_loader to load it securely from the Sytem32 folder.

This reverts commit 7102637.

This reverts commit 9b73e99.

ykhatav · 2023-11-21T16:25:22Z

@jandres742 friendly ping!

jandres742

thanks @ykhatav.

+1

@smaslov-intel : could you take a look?

sycl/plugins/level_zero/pi_level_zero.cpp

smaslov-intel · 2023-11-22T21:38:59Z

sycl/plugins/level_zero/pi_level_zero.cpp

+    break;
+  case dliNotePreLoadLibrary: {
+    if (strcmp(pdli->szDll, "ze_loader.dll") == 0) {
+      return (FARPROC)LoadLibraryExA("ze_loader.dll", nullptr,


are you doing the same to ur_loader?

No I am not sure/familiar if ur_loader also has load-time linking but this change is specific for ze_loader.

Same as ze_loader:

llvm/sycl/plugins/unified_runtime/CMakeLists.txt

Line 149 in 1060b20

UnifiedRuntimeLoader

Actually, now that I look at it, ur_loader.dll is not present in the C:\System32 folder instead i think we package and install it with sycl in sycl..\bin folder. So I don't think the same can be done for ur_loader., i.e we can't use LOAD_LIBRARY_SEARCH_SYSTEM32 flag, we would have to load it using an absolute path. In any case, i don't think it should be a part of this change, it needs separate PR in the UR repo.

smaslov-intel · 2023-11-23T03:40:27Z

sycl/plugins/level_zero/CMakeLists.txt

 add_dependencies(pi_level_zero ze-api)

+if (WIN32)
+ target_link_libraries(pi_level_zero PRIVATE delayimp)
+ target_link_options(pi_level_zero PRIVATE /DELAYLOAD:ze_loader.dll)


can we somehow use LevelZeroLoader as above at https://github.com/intel/llvm/pull/11902/files#diff-e162c7afd5dbcb57ced84354c427518f0f37b190538d136e871855d78975533bR98 and not hardcode ze_loader.dll?

Passing LevelZeroLoader to /DELAYLOAD doesn't seem to work, it needs the exact name of the DLL.

target_link_options supports generator expressions and LevelZeroLoader is a target, try TARGET_FILE like this:

Suggested change

target_link_options(pi_level_zero PRIVATE /DELAYLOAD:ze_loader.dll)

target_link_options(pi_level_zero PRIVATE /DELAYLOAD:$<TARGET_FILE:LevelZeroLoader>)

Thanks for the suggestion @kbenzie! However, i get a build error: Error evaluating generator expression:
$<TARGET_FILE:LevelZeroLoader>
Target "LevelZeroLoader" is not an executable or library.

kbenzie

Are these changes which should be mirrored in UR so this functionality doesn't get dropped when we switch to UR by default later on?

ykhatav · 2023-11-23T14:07:09Z

Are these changes which should be mirrored in UR so this functionality doesn't get dropped when we switch to UR by default later on?

Yes, these changes should be mirrored in UR as well.

kbenzie · 2023-11-23T14:09:33Z

Yes, these changes should be mirrored in UR as well.

Okay, would you be able to do that please?

Here's our Contribution Guide.

ykhatav · 2023-11-23T16:27:39Z

Yes, these changes should be mirrored in UR as well.

Okay, would you be able to do that please?

Here's our Contribution Guide.

Created a draft PR here: oneapi-src/unified-runtime#1117

ykhatav · 2023-11-29T15:03:39Z

sycl/plugins/level_zero/pi_level_zero.cpp


 // Defined in tracing.cpp
 void enableZeTracing();
 void disableZeTracing();
+#ifdef _WIN32
+FARPROC WINAPI delayHook(unsigned dliNotify, PDelayLoadInfo pdli) {


I will have to remove this hook function from here since we plan to mirror these changes in UR otherwise we will get multiple definition build error. @kbenzie please review my UR changes(oneapi-src/unified-runtime#1117) and I will update this PR accordingly. FYI, I have tested both the changes together in this draft PR- #12030

ykhatav · 2023-12-04T21:49:22Z

@kbenzie can you approve and merge this PR?

kbenzie

I don't have merge permissions so can't merge myself.

This reverts commit 3045806.

ykhatav added 3 commits November 15, 2023 13:51

[SYCL] Add delayloading for ze_loader

4cc3c48

[SYCL] remove delayimp

0b4f022

apply clang-format

9673d6b

ykhatav temporarily deployed to WindowsCILock November 15, 2023 22:17 — with GitHub Actions Inactive

ykhatav temporarily deployed to WindowsCILock November 15, 2023 23:02 — with GitHub Actions Inactive

add delayimp only on win

4129de9

ykhatav had a problem deploying to WindowsCILock November 16, 2023 03:44 — with GitHub Actions Failure

add delayimp only for windows

05c6ebe

ykhatav temporarily deployed to WindowsCILock November 16, 2023 15:09 — with GitHub Actions Inactive

ykhatav had a problem deploying to WindowsCILock November 16, 2023 15:54 — with GitHub Actions Failure

ykhatav added 2 commits November 16, 2023 09:36

add delay unload for ze_loader

9b73e99

apply clang-format

7102637

ykhatav temporarily deployed to WindowsCILock November 16, 2023 18:32 — with GitHub Actions Inactive

ykhatav temporarily deployed to WindowsCILock November 16, 2023 20:10 — with GitHub Actions Inactive

ykhatav added 2 commits November 16, 2023 17:23

Revert "apply clang-format"

4fe8e15

This reverts commit 7102637.

Revert "add delay unload for ze_loader"

3340f0a

This reverts commit 9b73e99.

ykhatav temporarily deployed to WindowsCILock November 17, 2023 01:31 — with GitHub Actions Inactive

ykhatav temporarily deployed to WindowsCILock November 17, 2023 08:56 — with GitHub Actions Inactive

ykhatav marked this pull request as ready for review November 17, 2023 14:03

ykhatav requested a review from a team as a code owner November 17, 2023 14:03

kbenzie requested a review from jandres742 November 17, 2023 14:05

jandres742 reviewed Nov 21, 2023

View reviewed changes

jandres742 requested a review from smaslov-intel November 21, 2023 16:46

smaslov-intel reviewed Nov 22, 2023

View reviewed changes

sycl/plugins/level_zero/pi_level_zero.cpp Outdated Show resolved Hide resolved

smaslov-intel reviewed Nov 22, 2023

View reviewed changes

smaslov-intel reviewed Nov 23, 2023

View reviewed changes

kbenzie reviewed Nov 23, 2023

View reviewed changes

ykhatav added 2 commits November 23, 2023 08:43

use strncmp instead of strcmp

6255aea

apply clang-format

07ee2b6

ykhatav temporarily deployed to WindowsCILock November 24, 2023 08:14 — with GitHub Actions Inactive

ykhatav temporarily deployed to WindowsCILock November 24, 2023 08:49 — with GitHub Actions Inactive

ykhatav requested a review from smaslov-intel November 28, 2023 13:48

ykhatav commented Nov 29, 2023

View reviewed changes

remove the delay load hook function

563eb9e

ykhatav temporarily deployed to WindowsCILock November 30, 2023 16:56 — with GitHub Actions Inactive

ykhatav temporarily deployed to WindowsCILock November 30, 2023 17:52 — with GitHub Actions Inactive

cperkinsintel requested a review from againull December 4, 2023 17:05

againull approved these changes Dec 4, 2023

View reviewed changes

againull requested a review from kbenzie December 4, 2023 22:21

kbenzie approved these changes Dec 5, 2023

View reviewed changes

againull merged commit 3045806 into intel:sycl Dec 5, 2023

ykhatav added a commit to ykhatav/llvm that referenced this pull request Dec 19, 2023

Revert "[SYCL] Add delayloading for ze_loader (intel#11902)"

96a49c3

This reverts commit 3045806.

againull pushed a commit that referenced this pull request Dec 19, 2023

Revert "[SYCL] Add delayloading for ze_loader (#11902)" (#12208)

7b921ea

This reverts commit 3045806.

	target_link_options(pi_level_zero PRIVATE /DELAYLOAD:ze_loader.dll)
	target_link_options(pi_level_zero PRIVATE /DELAYLOAD:$<TARGET_FILE:LevelZeroLoader>)

[SYCL] Add delayloading for ze_loader #11902

[SYCL] Add delayloading for ze_loader #11902

Uh oh!

Conversation

ykhatav commented Nov 15, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ykhatav commented Nov 21, 2023

Uh oh!

jandres742 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ykhatav Nov 23, 2023 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kbenzie left a comment

Choose a reason for hiding this comment

Uh oh!

ykhatav commented Nov 23, 2023

Uh oh!

kbenzie commented Nov 23, 2023

Uh oh!

ykhatav commented Nov 23, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ykhatav commented Dec 4, 2023

Uh oh!

kbenzie left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ykhatav commented Nov 15, 2023 •

edited

Loading

ykhatav Nov 23, 2023 •

edited

Loading