[SYCL] Enhance the sycl device libraries filename check when constructing ll… #2944

jinge90 · 2020-12-23T04:23:38Z

…vm-link commands

We check input files' name when constructing "llvm-link -only-needed" for linking SYCL device libraries. Currently, the criteria is:

has the prefix 'libsycl-'
has the postfix '.o'
Such check is too fragile, this PR aims to enhance the check, input SYCL device libraries' filename pattern is "libsycl-${pure-name}-$random.o" where ${pure-name} is in list {'crt', 'cmath', 'cmath-fp64', 'complex', 'complex-fp64', 'fallback-cassert', 'fallback-cmath', 'fallback-cmath-fp64', 'fallback-complex', 'fallback-complex-fp64'} and ${random} is a random string,
so the updated criteria is:
has the prefix 'libsycl-'
has the postfix '.o'
includes at least 2 '-' character in the filename
the 'pure name' extracted from the input filename should be in sycl device library 'pure name' list.
Input file list includes 2 files at least(when using llvm-link -only-needed, we will put user's device code and several sycl device libraries into input list which means there are 2 input files at least)

This enhancement can only mitigate the fragility since the check is still based on filename. If we switch to approach of "-mlink-builtin-bitcode" for SYCL device library link in the future, this problem will be gone automatically.

…vm-link commands Signed-off-by: gejin <[email protected]>

jinge90 · 2020-12-23T04:33:38Z

/summary:run

sndmitriev · 2020-12-23T06:13:56Z

clang/lib/Driver/ToolChains/SYCL.cpp

          return false;
        });
-    bool LinkSYCLDeviceLibs = (SYCLDeviceLibIter != InputFiles.end());
+    bool LinkSYCLDeviceLibs =
+        (SYCLDeviceLibIter != InputFiles.end()) && (InputFiles.size() > 1);


I think you need to verify that all inputs except the first are sycl libraries. Otherwise I guess it will not work as you expect it to.

Make sense, I have updated the patch.
Thanks very much.

…first one, we add -only-needed Signed-off-by: gejin <[email protected]>

jinge90 · 2020-12-23T07:45:39Z

/summary:run

clang/lib/Driver/ToolChains/SYCL.cpp

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

jinge90 · 2021-01-07T14:19:32Z

/summary:run

clang/lib/Driver/ToolChains/SYCL.cpp

mdtoguchi

Thanks, LGTM

jinge90 · 2021-01-11T07:45:50Z

@AGindinson
Could you help me review this PR?
Thanks very much.

AGindinson

Could you please add a test of some sort, at least a negative one?

AGindinson · 2021-01-11T08:12:20Z

clang/lib/Driver/ToolChains/SYCL.cpp

+    "crt",
+    "cmath",
+    "cmath-fp64",
+    "complex",
+    "complex-fp64",
+    "fallback-cassert",
+    "fallback-cmath",
+    "fallback-cmath-fp64",
+    "fallback-complex",
+    "fallback-complex-fp64"};


Does this mean that if we were to add a new device library, we'd have to put its base name here as well? Or is it that no new library is likely to be added?

Currently, those device libraries can meet our requirements but we may add new ones in the future and we need to put the basename here as well then.
Thanks very much.

The explanatory comment you've added should help, I think. I wonder if, in the future, we could make some sort of a configuration file with the device library base names, parsing that both in the relevant CMakeLists and in the driver code (via #include for compile-time?). I understand this is somewhat a vague approach - for example, where should we put the configuration file so that it could be conveniently accessed from all relevant LLVM subprojects? But just as something to consider for future improvements.

…b_check

jinge90 · 2021-01-12T03:29:36Z

Could you please add a test of some sort, at least a negative one?

Hi, @AGindinson
It seems very difficult to craft a test case to meet our requirement. The reason is the SYCL device library file name check is done by clang driver which users can't intercept. But we can analyze based on current compiler driver's behavior. The latest compiler will do 2 step link:

use normal llvm-link to link all user's input modules into one.
use llvm-link to link the "complete" user's device image generated from step 1 with all SYCL device library modules with "OnlyNeeded" flag.

We need to recognize whether current llvm-link phase is used to do "step-2" link, in order to achieve this, we check llvm-link's input list. This PR aims to enhance check for SYCL device libraries, the logic is:

the first input module is not a SYCL device library module
All other input modules in input list are SYCL device library module.

The SYCL device library modules are pre-built binary whose file name matches the pattern "libsycl-${pure file name}.o/obj" and after clang-offload-bundler extracting the internal LLVM IR as the input of llvm-link, the extracted LLVM IR file name matches pattern "libsycl-${pure file name}-${random temporary string}.o/obj", so the enhanced check logic is:

SYCL device library module should have the prefix "libsycl-" and postfix ".o" or ".obj"(Windows + CL mode)
Pure file name of SYCL device module should be in list {crt, cmath, cmath-fp64, complex.complex-fp64,fallback-cassert, fallback-cmath, fallback-cmath-fp64, fallback-complex, fallback-complex-fp64}.

User's input device module is generated by llvm-link and will always have the postfix ".bc" which won't be recognized as SYCL device library module. The SYCL device library module's file name are defined in libdevice's CMakeFile.

Thanks very much.

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

AGindinson · 2021-01-12T08:00:21Z

@jinge90, thanks a lot, your recent comment is really helpful!

Based on your explanation, a following simple test case could be envisioned: clang driver is fed with a "user" object that is named "almost like" the actual device libraries (e.g. "libsycl-test-user-library.o"). The checks would have to ensure that the "user" object is processed in the regular llvm-link call, and that a separate --only-needed call is generated for the actual device libraries. Basically, this would be just like the existing test case, but with the user object renamed to something more resembling the libraries, maybe even replacing the existing test case if you find this more preferable. Doing this would, to some extent, test the new filtering mechanism that you've implemented.

Does this make sense, or am I missing some more nuances (or, for that matter, am I giving this too much of a priority)?

jinge90 · 2021-01-12T08:13:21Z

@jinge90, thanks a lot, your recent comment is really helpful!

Based on your explanation, a following simple test case could be envisioned: clang driver is fed with a "user" object that is named "almost like" the actual device libraries (e.g. "libsycl-test-user-library.o"). The checks would have to ensure that the "user" object is processed in the regular llvm-link call, and that a separate --only-needed call is generated for the actual device libraries. Basically, this would be just like the existing test case, but with the user object renamed to something more resembling the libraries, maybe even replacing the existing test case if you find this more preferable. Doing this would, to some extent, test the new filtering mechanism that you've implemented.

Does this make sense, or am I missing some more nuances (or, for that matter, am I giving this too much of a priority)?

So, do you mean we use a input file whose name is same as some SYCL device libraries to see if the 2-step llvm-link still works as expected?

AGindinson · 2021-01-12T08:19:06Z

So, do you mean we use a input file whose name is same as some SYCL device libraries to see if the 2-step llvm-link still works as expected?

Yep, a name that would be almost similar to the real device library names.

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

clang/test/Driver/sycl-device-lib.cpp

jinge90 · 2021-01-13T03:08:54Z

/summary:run

jinge90 · 2021-01-13T03:13:21Z

Hi, @mdtoguchi
I updated the patch to add a test, could you help me review and +1 again?
Thanks very much.

jinge90 · 2021-01-15T03:41:12Z

Hi, @mdtoguchi
I updated the patch to add a test, could you help me review and +1 again?
Thanks very much.

Hi, @mdtoguchi
Could you help me approve again? The updated patch only adds some test.
Thanks very much.

* sycl: [SYCL][NFC] Fix warnings reported by Clang (intel#3050) [SYCL] Enhance device libraries filename check when constructing llvm-link commands (intel#2944) [SYCL][NFC] Add vtable ABI checks (intel#3032) [SYCL] Cleanup the deprecated Intel attribute logic (intel#3036)

Motivations is similar as f729c49. This PR addresses SYCL device global which may have attributes "sycl-device-image-scope", "sycl-host-access" and "sycl-unique-id". Failure to preserve "sycl-unique-id" after llvm-spirv translation triggers assert at https://github.com/intel/llvm/blob/2824f61dd36790448a224cd596985bd01cbcd0f3/llvm/lib/SYCLLowerIR/DeviceGlobals.cpp#L85 Also preserve GlobalVariable metadata as an improvement, though there is no test to show this is really needed. Original commit: KhronosGroup/SPIRV-LLVM-Translator@f2d913cb1a22cb3

Enhance the sycl device libraries filename check when constructing ll…

9956e5d

…vm-link commands Signed-off-by: gejin <[email protected]>

jinge90 requested review from AGindinson and mdtoguchi as code owners December 23, 2020 04:23

jinge90 requested review from vzakhari, bader and sndmitriev December 23, 2020 04:24

sndmitriev reviewed Dec 23, 2020

View reviewed changes

If all input files to llvm-link are sycl device libraries except the …

ca73d2e

…first one, we add -only-needed Signed-off-by: gejin <[email protected]>

mdtoguchi reviewed Dec 30, 2020

View reviewed changes

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

support clang-cl on Windows

62d6126

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

mdtoguchi reviewed Jan 7, 2021

View reviewed changes

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

mdtoguchi previously approved these changes Jan 8, 2021

View reviewed changes

AGindinson reviewed Jan 11, 2021

View reviewed changes

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

6648c4b

…b_check

jinge90 dismissed mdtoguchi’s stale review via 3535089 January 12, 2021 03:48

Add comments

3535089

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

Add test

afe4210

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

AGindinson reviewed Jan 12, 2021

View reviewed changes

clang/test/Driver/sycl-device-lib.cpp Show resolved Hide resolved

jinge90 changed the title ~~[SYCL]Enhance the sycl device libraries filename check when constructing ll…~~ [SYCL] Enhance the sycl device libraries filename check when constructing ll… Jan 13, 2021

AGindinson approved these changes Jan 13, 2021

View reviewed changes

mdtoguchi approved these changes Jan 15, 2021

View reviewed changes

romanovvlad merged commit f930a01 into intel:sycl Jan 18, 2021

jinge90 deleted the enhance_sycl_devicelib_check branch January 26, 2021 01:20

[SYCL] Enhance the sycl device libraries filename check when constructing ll… #2944

[SYCL] Enhance the sycl device libraries filename check when constructing ll… #2944

Uh oh!

Conversation

jinge90 commented Dec 23, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jinge90 commented Dec 23, 2020

Uh oh!

sndmitriev Dec 23, 2020

Choose a reason for hiding this comment

Uh oh!

jinge90 Dec 23, 2020

Choose a reason for hiding this comment

Uh oh!

jinge90 commented Dec 23, 2020

Uh oh!

Uh oh!

jinge90 commented Jan 7, 2021

Uh oh!

Uh oh!

mdtoguchi left a comment

Choose a reason for hiding this comment

Uh oh!

jinge90 commented Jan 11, 2021

Uh oh!

AGindinson left a comment

Choose a reason for hiding this comment

Uh oh!

AGindinson Jan 11, 2021

Choose a reason for hiding this comment

Uh oh!

jinge90 Jan 11, 2021 • 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!

AGindinson Jan 12, 2021

Choose a reason for hiding this comment

Uh oh!

jinge90 commented Jan 12, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AGindinson commented Jan 12, 2021

Uh oh!

jinge90 commented Jan 12, 2021

Uh oh!

AGindinson commented Jan 12, 2021

Uh oh!

Uh oh!

jinge90 commented Jan 13, 2021

Uh oh!

jinge90 commented Jan 13, 2021

Uh oh!

jinge90 commented Jan 15, 2021

Uh oh!

Uh oh!

jinge90 commented Dec 23, 2020 •

edited

Loading

jinge90 Jan 11, 2021 •

edited

Loading

jinge90 commented Jan 12, 2021 •

edited

Loading