[SYCL] Remove dependency on libdevice binary for sycl-device-lib test #3639

jinge90 · 2021-04-28T02:35:54Z

jinge90 · 2021-04-28T02:39:03Z

Hi, @bader
This patch aims to fix #3630
Thanks very much.

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

bader · 2021-04-28T04:36:39Z

clang/lib/Driver/Driver.cpp

+          if (llvm::sys::fs::exists(LibName) ||
+              C.getArgs().hasArg(options::OPT__HASH_HASH_HASH)) {


I don't think customizing Driver's behavior for the test is a good solution to the problem.
The common approach in clang LIT infrastructure is to create an "input files", so you we test all scenarios (see https://github.com/intel/llvm/tree/sycl/clang/test/Driver/Inputs).
Please, create corresponding files and test Driver's behavior when no/part/all libraries are found.

Hi, @bader and @mdtoguchi
Current implementation for adding SYCL device libraries in driver doesn't support such "input files". The reason is we have assumption for SYCL device library binaries, in https://github.com/intel/llvm/blob/sycl/clang/lib/Driver/Driver.cpp#L4141
we get the real path of compiler(suppose it is ${compiler_bin_path}) and assume SYCL device binaries are located in ${compiler_bin_path}/../lib/. In other targets including CUDA, compiler driver supports configuring the libdevice binary path based on sys_root env var.
If we want to use the "input file" mechanism in lit test for SYCL device library, can we consider to provide a way to configure SYCL device library path? For example,

adding an option for sycl device library path to override the default path

Use an env var for the sycl device library path
Thanks very much.

This sounds like a bug. Why can't we distribute these libraries in SYSTEM paths, which are configured through standard command line options (or SYCL specific)?

Here is a couple of examples how it's done for:

CUDA: clang/test/Driver/cuda-detect-path.cu, clang/test/Driver/cuda-detect.cu

ROCM: clang/test/Driver/hip-device-libs.hip clang/test/Driver/rocm-detect.cl clang/test/Driver/rocm-device-libs.cl

Hi, @bader
SYCL device library binaries are shipped together with all sycl runtime libraries such as libsycl.so, libze_loader.so....
Do we have any mechanism to configure path of those libraries through any command line option?
It seems that other targets have such mechanism. For CUDA, "-cuda-path=XX" is used: https://github.com/intel/llvm/blob/sycl/clang/lib/Driver/ToolChains/Cuda.cpp#L137
Thanks very much.

Possibly introduce a similar option for us: -fsycl-devicelib-path=<arg> which will point to the desired location for the device libraries. Currently we rely on expected locations and the known directory structure to find and pick these up. Is there a build target we can set a REQUIRES to for the test?

Adding the libsycldevice as dependency for sycl-device-lib makes sense for fixing this issue. I agree that we can consider adding a driver option like "-fsycl-devicelib-path=XXX" as a separate issue.
Hi, @bader and @andykaylor
Do you have any concern if we follow this approach?
Thanks very much.

I don't think it's a good solution. The dependency on libsycldevice is not needed to test clang's driver functionality. Having mock files in test Inputs and configuring search path through existing options like -system should be enough.

Hi, @bader @vzakhari @mdtoguchi @andykaylor
It seems that we have 2 options here:

Adding libsycldevice as dependency for sycl-device-lib test.

Having mock input files and providing some way to configure the search patch for device library binaries in driver.

For 1,
Pros: No change needed in clang driver.
Cons: This approach doesn't align with other targets like CUDA.

For 2,
Pros: align with other targets' approach and this clang driver lit test won't have any dependency other than OS.
Cons: we need support configuring sycl device library search path in driver, either via any existing options like -isystem, -isysroot or sycl specific option like "-fsycl-device-lib-path".

It seems that we can consider another question firstly, do we need to support configuring sycl device library search path in driver? If we have strong reasons to support, I think approach 2 is better since it aligns with other targets and libsycldevice dependency will be unnecessary at that time.
Thanks very much.

Hi, @bader @vzakhari @mdtoguchi @andykaylor
Do you have any preference for option 1 and option 2 mentioned above?
If no objection, I plan to follow Alexey's suggestion to support searching sycl device libraries through existing option "--sysroot".
Thanks very much.

Having the test be compartmentalized (driver specific) is a good thing. I'm OK with #2.

bader · 2021-04-28T04:38:09Z

Hi, @bader
This patch aims to fix #3630
Thanks very much.

Please, link the issue, so it's closed when pull request is merged.

…_dependency

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

mdtoguchi

Overall, this looks OK. For practical purposes, would there be any expectation that the device libraries will ever be located in which we require usage of --sysroot?

No objections for new approach from my side. Thanks!

jinge90 · 2021-05-13T02:24:15Z

Overall, this looks OK. For practical purposes, would there be any expectation that the device libraries will ever be located in which we require usage of --sysroot?

Hi, @mdtoguchi
Currently, we don't have such expectation. This patch only follows other targets' solution.
Thanks very much.

bader · 2021-05-13T08:48:04Z

clang/lib/Driver/Driver.cpp

-      llvm::sys::path::append(LibLoc, "/../lib");
+      SmallVector<SmallString<128>, 4> LibLocCandidates;
+      LibLocCandidates.emplace_back(TC->getDriver().Dir + "/../lib");
+      LibLocCandidates.emplace_back(TC->getDriver().SysRoot + "/lib");


According to my understanding SysRoot default value matches Dir/../, so probably we can simplify the code and use only one candidate. Right?

Hi, @bader
I just checked that if we don't specify "--sysroot=..." in command line, the "TC->getDriver().SysRoot" will be empty string.
So, we have to keep 2 candidates here.
Thanks very much.

Okay. I just noticed that similar logic for HIP/ROCM is implemented in toolchain specific file.
See RocmInstallationDetector in https://github.com/intel/llvm/blob/sycl/clang/lib/Driver/ToolChains/AMDGPU.cpp#L171 .
@mdtoguchi, should we move this code to https://github.com/intel/llvm/blob/sycl/clang/lib/Driver/ToolChains/SYCL.cpp?

Sure - maybe add a dir vector that holds the known locations in which the libs can be searched for.

Hi, @bader and @mdtoguchi
So, do you mean we need to add a "SyclInstallationDectector" in SYCL.cpp and use it to record search paths for libs, bins, include?
Thanks very much.

@jinge90, I don't think we need to add a full installation detector as everything is provided with the compiler. I think just add the lib search directories when the SYCLToolChain is created.

Hi, @mdtoguchi
I suggest to add a very simple "SyclInstallationDectector" which only provide APIs to query lib search path for now. If we have new request, we can extend the simple "SyclInstallDetector", is it OK?
Thanks very much.

Hi, @mdtoguchi and @bader
I added a very simple SYCLInstallationDetector, there is no much to do in this function since we don't have many requirements for detecting installation structure currently. If we have some requirements in the future such as adding some option like "-fsycl-installation=...", it is easy to extend this class.
Thanks very much.

I think just add the lib search directories when the SYCLToolChain is created.

I thought that Mike suggestion is that we move addSYCLDeviceLibs to SYCLToolChain.

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

jinge90 · 2021-05-17T06:06:34Z

/summary:run

mdtoguchi

Looks reasonable to me. We could use this framework for setting up the search location for the headers too.

jinge90 requested review from AGindinson and mdtoguchi as code owners April 28, 2021 02:35

jinge90 requested a review from bader April 28, 2021 02:36

Remove dependecy on libdevice binary for sycl-device-lib test

0b0b31f

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

bader previously requested changes Apr 28, 2021

View reviewed changes

jinge90 linked an issue Apr 28, 2021 that may be closed by this pull request

[Driver] Clang :: Driver/sycl-device-lib.cpp fails if libdevice project is not pre-built #3630

Closed

jinge90 requested a review from bader April 28, 2021 06:48

bader requested review from vzakhari and andykaylor April 28, 2021 07:16

jinge90 added 2 commits May 12, 2021 09:49

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

57fc554

…_dependency

configure devicelib binrary path based on sysroot

a433194

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

mdtoguchi reviewed May 12, 2021

View reviewed changes

jinge90 requested a review from mdtoguchi May 13, 2021 02:24

bader reviewed May 13, 2021

View reviewed changes

jinge90 added 2 commits May 17, 2021 11:30

Add SYCLInstallationDetector

812992b

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

fix clang-format

60bc311

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

mdtoguchi approved these changes May 18, 2021

View reviewed changes

bader merged commit 1307a22 into intel:sycl May 19, 2021

bader mentioned this pull request Jun 3, 2021

[SYCL] Unique stable name rebase #3835

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SYCL] Remove dependency on libdevice binary for sycl-device-lib test #3639

[SYCL] Remove dependency on libdevice binary for sycl-device-lib test #3639

jinge90 commented Apr 28, 2021

jinge90 commented Apr 28, 2021

bader Apr 28, 2021

jinge90 Apr 28, 2021 •

edited

Loading

bader Apr 28, 2021

jinge90 Apr 28, 2021

mdtoguchi Apr 28, 2021

jinge90 Apr 29, 2021

bader Apr 29, 2021

jinge90 Apr 29, 2021

jinge90 May 11, 2021

mdtoguchi May 11, 2021

bader commented Apr 28, 2021

mdtoguchi left a comment

jinge90 commented May 13, 2021

bader May 13, 2021

jinge90 May 13, 2021

bader May 13, 2021

mdtoguchi May 13, 2021

jinge90 May 14, 2021

mdtoguchi May 14, 2021

jinge90 May 14, 2021

jinge90 May 17, 2021

bader May 17, 2021

jinge90 commented May 17, 2021

mdtoguchi left a comment

		if (llvm::sys::fs::exists(LibName) \|\|
		C.getArgs().hasArg(options::OPT__HASH_HASH_HASH)) {

[SYCL] Remove dependency on libdevice binary for sycl-device-lib test #3639

[SYCL] Remove dependency on libdevice binary for sycl-device-lib test #3639

Conversation

jinge90 commented Apr 28, 2021

jinge90 commented Apr 28, 2021

Choose a reason for hiding this comment

jinge90 Apr 28, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bader commented Apr 28, 2021

mdtoguchi left a comment

Choose a reason for hiding this comment

jinge90 commented May 13, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jinge90 commented May 17, 2021

mdtoguchi left a comment

Choose a reason for hiding this comment

jinge90 Apr 28, 2021 •

edited

Loading