Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
bader merged 5 commits into from
May 19, 2021
Merged

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

Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
0b0b31f
Remove dependecy on libdevice binary for sycl-device-lib test
jinge90 Apr 28, 2021
57fc554
Merge remote-tracking branch 'origin/sycl' into remove_libdevice_test…
jinge90 May 12, 2021
a433194
configure devicelib binrary path based on sysroot
jinge90 May 12, 2021
812992b
Add SYCLInstallationDetector
jinge90 May 17, 2021
60bc311
fix clang-format
jinge90 May 17, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion clang/lib/Driver/Driver.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4165,7 +4165,8 @@ class OffloadingActionBuilder final {
SmallString<128> LibName(LibLoc);
llvm::sys::path::append(LibName, Lib.devicelib_name);
llvm::sys::path::replace_extension(LibName, LibSuffix);
if (llvm::sys::fs::exists(LibName)) {
if (llvm::sys::fs::exists(LibName) ||
C.getArgs().hasArg(options::OPT__HASH_HASH_HASH)) {
Copy link
Contributor

@bader bader Apr 28, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@jinge90 jinge90 Apr 28, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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,

  1. adding an option for sycl device library path to override the default path
  2. Use an env var for the sycl device library path
    Thanks very much.

Copy link
Contributor

@bader bader Apr 28, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@jinge90 jinge90 Apr 28, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@mdtoguchi mdtoguchi Apr 28, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

@jinge90 jinge90 Apr 29, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@bader bader Apr 29, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@jinge90 jinge90 Apr 29, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

  1. Adding libsycldevice as dependency for sycl-device-lib test.
  2. 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.

Copy link
Contributor Author

@jinge90 jinge90 May 11, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@mdtoguchi mdtoguchi May 11, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

++NumOfDeviceLibLinked;
Arg *InputArg = MakeInputArg(Args, C.getDriver().getOpts(),
Args.MakeArgString(LibName));
Expand Down