[SYCL] Link SYCL device libraries by default. #2400

jinge90 · 2020-09-01T07:46:48Z

As not all backends have supported spv file online link, we decided to
disable fallback spv libraries online link at this time. Instead, all
wrapper and fallback .o files will be linked offline. When all backends
support spv online link, we will switch to online link for jit compilation
transparently.

Signed-off-by: gejin [email protected]

As not all backends have supported spv file online link, we decided to disable fallback spv libraries online link at this time. Instead, all wrapper and fallback .o files will be linked offline. When all backends support spv online link, we will switch to online link for jit compilation transparently. Signed-off-by: gejin <[email protected]>

jinge90 · 2020-09-01T07:51:54Z

Hi, @vzakhari and @mdtoguchi
As we discussed, I extracted the "linking" part from #2277
In this PR, all device libraries including wrapper and fallback device libraries are linked offline by default. I disabled the spv fallback device libraries online link in SYCL runtime.
When all backends support spv online link, we will switch back to online link for jit compilation transparently.

jinge90 · 2020-09-01T08:42:53Z

/summary:run

clang/lib/Driver/Driver.cpp

…c and libm-fp32 libs are linked by default Signed-off-by: gejin <[email protected]>

…ault_in_aot_style

Signed-off-by: gejin <[email protected]>

clang/lib/Driver/Driver.cpp

Signed-off-by: gejin <[email protected]>

clang/include/clang/Driver/Options.td

clang/lib/Driver/Driver.cpp

Signed-off-by: gejin <[email protected]>

jinge90 · 2020-09-03T13:03:43Z

Hi, @mdtoguchi
I have added the -f[no-]sycl-device-lib option, could you help me review it?
Thank you very much.

jinge90 · 2020-09-03T14:41:46Z

/summary:run

vzakhari

LGTM for devicelib tests.

mdtoguchi

Could you add a driver specific test verifying that the various expected device libs are being pulled in for unbundling and use?

Signed-off-by: gejin <[email protected]>

…ault_in_aot_style

jinge90 · 2020-09-04T12:32:05Z

/summary:run

jinge90 · 2020-09-04T12:37:42Z

Could you add a driver specific test verifying that the various expected device libs are being pulled in for unbundling and use?

Hi, @mdtoguchi , I have added 2 driver lit tests(Linux + Windows) for "-f[no-]sycl-device-lib" option. Please help review.
Thank you very much.

jinge90 · 2020-09-04T12:39:26Z

LGTM for devicelib tests.

Hi, @vzakhari , I think we can enable the devicelib lit test for GPU platform since the "AOT" style link should work on all backend including those who haven't supported online spv link. The latest patch has enabled GPU.

jinge90 · 2020-09-05T01:36:31Z

/summary:run

mdtoguchi

Driver LGTM

…ault_in_aot_style

Signed-off-by: gejin <[email protected]>

sycl/test/devicelib/cmath_fp64_test.cpp

Signed-off-by: gejin <[email protected]>

sycl/test/devicelib/cmath_fp64_test.cpp

sycl/test/devicelib/cmath_test.cpp

sycl/test/devicelib/math_fp64_test.cpp

sycl/test/devicelib/math_windows_test.cpp

sycl/test/devicelib/std_complex_math_fp64_test.cpp

sycl/test/devicelib/std_complex_math_test.cpp

Signed-off-by: gejin <[email protected]>

sycl/source/detail/program_manager/program_manager.cpp

Signed-off-by: gejin <[email protected]>

clang/lib/Driver/Driver.cpp

mdtoguchi

Thanks - LGTM

jinge90 · 2020-09-11T02:01:53Z

Hi, @AGindinson ,@intel/llvm-reviewers-runtime and @pvchupin
Could you help review this PR?
Thank you very much.

s-kanaev

Looks good overall.
Though, I have only a single comment.

s-kanaev · 2020-09-15T07:42:13Z

sycl/source/detail/program_manager/program_manager.cpp

-  // The Level Zero driver support for online linking currently has bugs, but
-  // we think the DPC++ runtime support is ready.  This environment variable
-  // gates the runtime support for online linking, so we can try enabling if a
-  // new driver is released before the next DPC++ release.
-  static bool EnableLevelZeroLink = std::getenv("SYCL_ENABLE_LEVEL_ZERO_LINK");
-  if (!EnableLevelZeroLink) {
-    if (Context->getPlugin().getBackend() == backend::level_zero) {
-      LinkDeviceLibs = false;
-    }
-  }


Online linking was disabled for Level Zero only. Now it's disabled for each and every backend. Why is that?
Another solution is to enable it for OpenCL backend.

Hi, @s-kanaev
There are 2 reasons:

The device library shouldn't depend on one specific backend to work, our final goal is to make device libraries to have same behavior in all backend. So, we switch to offline link for all backends.

Simplifying the usage requires us to make some device libraries to be linked in by default in offline build phase. If we enable devicelib for OCL backend, we must only link "wrapper" obj device libs offline.
However, to make L0 to work, we must link both "wrapper" and "fallback" libraries offline. Unfortunately, we can't know which backend will be used in offline build, so we have to disable all backends and switch back to "online" link transparently when L0 is ready.
Thank you very much.

It looks like we are reaching the limit of a system if at the end everybody has to align on the intersection of all the features of the back-ends, which converges to the... empty set!
And if the root cause is a political back-end that nobody cares except the people who invented it, I am not super excited about this story jeopardizing all the other back-ends... :-/

I agree with @keryell that special-casing for particular BEs in the generic SYCL runtime is something which should be avoided. The supposed way to avoid this is to extract the diverging piece of code into a separate function and make it part of the PI interface, hiding all specifics inside the plugin. @jinge90 - I'd suggest to add a TODO at least and maybe file an issue.

Note that this PR removes the code that special-cases the L0 plugin.

Upon second look, it seems nothing level zero-specific is in the patch. LinkDeviceLibs have existed before. But still, would be good to have TODO/issue for BE-specific stuff.

Hi, @kbobrovs @keryell and @gmlueck
I think all backends including Level zero will support online link, this patch is a temporary solution to make device library to work on all backends with same behaviors. Once all backends are ready, we will switch to "online" link transparently. I will add a "TODO" in another PR.
Thank you very much.

* upstream/sycl: (405 commits) [SYCL] Implement new env var SYCL_DEVICE_FILTER (intel#2239) [Driver][SYCL] Make /MD the default for -fsycl (intel#2478) [SYCL]: basic support of contexts with multiple devices in Level-Zero (intel#2440) [SYCL] Fix LIT regression after 9dd18ca (intel#2481) [SYCL][L0] Kernel Destroy in piKernelRelease (intel#2475) [SYCL] Emit an aliased function only if it is used (intel#2430) [Driver][SYCL] Add defaultlib directive for sycl lib (intel#2464) [Driver][SYCL] Improve situations where .exe is added for AOT tools (intel#2467) [SYCL][L0]: Check Queue refcnt prior to using members in event wait/release (intel#2471) [SYCL] Unroll several loops in __init method accessor class (intel#2449) [SYCL][Doc] Add link to use pinned memory spec (intel#2463) [SYCL] Link SYCL device libraries by default. (intel#2400) Revert "[SYCL] XFAIL test blcoking pulldown" Avoid usage of deprecated "VectorType::getNumElements" (intel#737) Fix nullptr dereference (intel#741) Do not translate arbitrary precision operations without corresponding extensions (intel#714) Add Constrained Floating-Point Intrinsics support [SYCL] Take into account auxiliary cmake options for Level Zero loader [InstCombine] improve fold of pointer differences [InstCombine] add ptr difference tests; NFC ...

Revert "Add new launch property to support work_group_scratch_memory"

jinge90 requested review from AGindinson, kbobrovs, mdtoguchi and a team as code owners September 1, 2020 07:46

jinge90 requested a review from v-klochkov September 1, 2020 07:46

jinge90 requested review from asavonic and vzakhari and removed request for v-klochkov September 1, 2020 07:47

jinge90 mentioned this pull request Sep 1, 2020

[SYCL]Device lib default link #2277

Closed

mdtoguchi reviewed Sep 1, 2020

View reviewed changes

clang/lib/Driver/Driver.cpp Outdated Show resolved Hide resolved

clang/lib/Driver/Driver.cpp Outdated Show resolved Hide resolved

clang/lib/Driver/Driver.cpp Outdated Show resolved Hide resolved

keryell reviewed Sep 1, 2020

View reviewed changes

clang/lib/Driver/Driver.cpp Outdated Show resolved Hide resolved

clang/lib/Driver/Driver.cpp Outdated Show resolved Hide resolved

jinge90 mentioned this pull request Sep 2, 2020

[SYCL] Adjust devicelib install location #2405

Merged

jinge90 added 3 commits September 3, 2020 15:17

add options -f[no-]sycl-device-lib=<libc,libm-fp32,libm-fp64,all> lib…

6d1f7d3

…c and libm-fp32 libs are linked by default Signed-off-by: gejin <[email protected]>

Merge remote-tracking branch 'origin/sycl' into link_devicelib_by_def…

00dbea9

…ault_in_aot_style

remove ununsed headr

c871ceb

Signed-off-by: gejin <[email protected]>

jinge90 commented Sep 3, 2020

View reviewed changes

clang/lib/Driver/Driver.cpp Outdated Show resolved Hide resolved

jinge90 added 2 commits September 3, 2020 16:11

Fix driver lit test using -fno-sycl-device-lib=all

6024034

Signed-off-by: gejin <[email protected]>

Use correct option value for devicelib test

a1bca27

Signed-off-by: gejin <[email protected]>

kbobrovs reviewed Sep 3, 2020

View reviewed changes

clang/include/clang/Driver/Options.td Outdated Show resolved Hide resolved

clang/include/clang/Driver/Options.td Outdated Show resolved Hide resolved

clang/lib/Driver/Driver.cpp Outdated Show resolved Hide resolved

Fix aot lit test

dfc2fff

Signed-off-by: gejin <[email protected]>

vzakhari previously approved these changes Sep 3, 2020

View reviewed changes

mdtoguchi reviewed Sep 3, 2020

View reviewed changes

jinge90 dismissed vzakhari’s stale review via 640ba0d September 4, 2020 07:50

jinge90 added 2 commits September 4, 2020 15:54

Add Driver Lit tests for -f[no-]sycl-device-lib option

640ba0d

Signed-off-by: gejin <[email protected]>

Merge remote-tracking branch 'origin/sycl' into link_devicelib_by_def…

f562cdf

…ault_in_aot_style

mdtoguchi previously approved these changes Sep 5, 2020

View reviewed changes

Merge remote-tracking branch 'origin/sycl' into link_devicelib_by_def…

52cf2fe

…ault_in_aot_style

jinge90 dismissed mdtoguchi’s stale review via 7803a86 September 7, 2020 04:31

update helper text for -f[no-]sycl-device-lib

7803a86

Signed-off-by: gejin <[email protected]>

vzakhari reviewed Sep 8, 2020

View reviewed changes

sycl/test/devicelib/cmath_fp64_test.cpp Outdated Show resolved Hide resolved

jinge90 added 2 commits September 9, 2020 10:10

Merge latest sycl branch and adjust devicelib path in addSYCLDeviceLibs

ad3d7e0

Signed-off-by: gejin <[email protected]>

Adjust devicelib suffix in IsSYCLDeviceLibs

fedbeef

Signed-off-by: gejin <[email protected]>

vzakhari reviewed Sep 9, 2020

View reviewed changes

sycl/test/devicelib/cmath_fp64_test.cpp Show resolved Hide resolved

vzakhari reviewed Sep 9, 2020

View reviewed changes

jinge90 added 3 commits September 9, 2020 17:28

remove unnecessary restrictions for devicelib lit tests

ba91f75

Signed-off-by: gejin <[email protected]>

fix double issue in float test for MSVC

7a36310

Signed-off-by: gejin <[email protected]>

remove ldexp/frexp in math test to avoid Windows failure

5aeb56d

Signed-off-by: gejin <[email protected]>

gmlueck reviewed Sep 9, 2020

View reviewed changes

sycl/source/detail/program_manager/program_manager.cpp Outdated Show resolved Hide resolved

Disable fallback devicelibs in one place

9d7dba8

Signed-off-by: gejin <[email protected]>

kbobrovs approved these changes Sep 10, 2020

View reviewed changes

jinge90 commented Sep 10, 2020

View reviewed changes

clang/lib/Driver/Driver.cpp Show resolved Hide resolved

mdtoguchi approved these changes Sep 10, 2020

View reviewed changes

vzakhari approved these changes Sep 11, 2020

View reviewed changes

s-kanaev approved these changes Sep 15, 2020

View reviewed changes

romanovvlad merged commit 9dd18ca into intel:sycl Sep 15, 2020

gmlueck mentioned this pull request Sep 15, 2020

[SYCL] Fix build log for L0 plugin #2479

Merged

bader mentioned this pull request Oct 11, 2020

Dev cloud back end JIT failure #1642

Closed

jinge90 deleted the link_devicelib_by_default_in_aot_style branch January 26, 2021 01:21

Chenyang-L pushed a commit that referenced this pull request Feb 18, 2025

Merge pull request #2400 from callumfare/revert-1968

63a46d7

Revert "Add new launch property to support work_group_scratch_memory"

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SYCL] Link SYCL device libraries by default. #2400

[SYCL] Link SYCL device libraries by default. #2400

jinge90 commented Sep 1, 2020

jinge90 commented Sep 1, 2020

jinge90 commented Sep 1, 2020

jinge90 commented Sep 3, 2020

jinge90 commented Sep 3, 2020

vzakhari left a comment

mdtoguchi left a comment

jinge90 commented Sep 4, 2020

jinge90 commented Sep 4, 2020

jinge90 commented Sep 4, 2020

jinge90 commented Sep 5, 2020

mdtoguchi left a comment

mdtoguchi left a comment

jinge90 commented Sep 11, 2020 •

edited

Loading

s-kanaev left a comment

s-kanaev Sep 15, 2020

jinge90 Sep 15, 2020

keryell Sep 15, 2020

kbobrovs Sep 15, 2020

gmlueck Sep 15, 2020

kbobrovs Sep 15, 2020

jinge90 Sep 16, 2020

[SYCL] Link SYCL device libraries by default. #2400

[SYCL] Link SYCL device libraries by default. #2400

Conversation

jinge90 commented Sep 1, 2020

jinge90 commented Sep 1, 2020

jinge90 commented Sep 1, 2020

jinge90 commented Sep 3, 2020

jinge90 commented Sep 3, 2020

vzakhari left a comment

Choose a reason for hiding this comment

mdtoguchi left a comment

Choose a reason for hiding this comment

jinge90 commented Sep 4, 2020

jinge90 commented Sep 4, 2020

jinge90 commented Sep 4, 2020

jinge90 commented Sep 5, 2020

mdtoguchi left a comment

Choose a reason for hiding this comment

mdtoguchi left a comment

Choose a reason for hiding this comment

jinge90 commented Sep 11, 2020 • edited Loading

s-kanaev left a comment

Choose a reason for hiding this comment

s-kanaev Sep 15, 2020

Choose a reason for hiding this comment

jinge90 Sep 15, 2020

Choose a reason for hiding this comment

keryell Sep 15, 2020

Choose a reason for hiding this comment

kbobrovs Sep 15, 2020

Choose a reason for hiding this comment

gmlueck Sep 15, 2020

Choose a reason for hiding this comment

kbobrovs Sep 15, 2020

Choose a reason for hiding this comment

jinge90 Sep 16, 2020

Choose a reason for hiding this comment

jinge90 commented Sep 11, 2020 •

edited

Loading