[Driver][SYCL][NewOffloadModel] Pull in device libraries at device compile#21672
[Driver][SYCL][NewOffloadModel] Pull in device libraries at device compile#21672mdtoguchi wants to merge 7 commits intointel:syclfrom
Conversation
…mpile Pull in the device libraries during compile time when using the new offload model. This is done by using the -mlink-builtin-bitcode option, passing in each individual device library as needed. This frees up the responsibility of linking in the device libraries during link time, and requiring the driver to pass any libraries needed to the clang-linker-wrapper or having the clang-linker-wrapper figure out what device libraries are needed at link. With this move, the new.o device libraries are no longer used during the device linking stage using the clang-linker-wrapper. Made updates to the build to stop creating these binaries. Reland of intel@c2a9bf8 Updated the SYCLInstallationDetector constructor, as there were two and we only need one. The mix of these caused a disconnect in populating the library search locations.
|
This is a reland of c2a9bf8 which was reverted due to some issues with pulling in the device library locations during the device compile step. Updating SYCLInstallationDetector to better match what is upstream fixes the issue seen. |
There was a problem hiding this comment.
Pull request overview
This PR updates Clang’s SYCL new offload model to pull SYCL device libraries into the device compilation step (via -mlink-builtin-bitcode / -mlink-bitcode-file) instead of relying on device-lib linking during the later device link stage, and adjusts libdevice builds/tests accordingly.
Changes:
- Link SYCL device bitcode libraries at SPIR/SPIR-V compile time for the new offload model, including sanitizer/ITT libraries and new diagnostics for missing libs.
- Remove generation/usage of
*.new.o/*.new.objlibdevice artifacts and update CMake + driver tests to expect.bclinkage behavior. - Refactor SYCL device library selection APIs (move to
SYCLToolChainand returnBitCodeLibraryInfo) and update toolchains/tests/wrapper behavior.
Reviewed changes
Copilot reviewed 20 out of 20 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| libdevice/sanitizer/tsan_rtl.cpp | Makes __TsanLaunchInfo weak to avoid hard symbol requirements across device-lib linkage models. |
| libdevice/sanitizer/msan_rtl.cpp | Makes __MsanLaunchInfo weak for the updated device-lib linkage approach. |
| libdevice/crt_wrapper.cpp | Makes RandNext weak to support the new link model and avoid duplicate-definition issues. |
| libdevice/cmake/modules/SYCLLibdevice.cmake | Stops producing obj-new-offload (*.new.o/*.new.obj) artifacts; simplifies IMF host library build/install targets. |
| clang/tools/clang-linker-wrapper/ClangLinkerWrapper.cpp | Skips the second device link step when no device library files are provided. |
| clang/test/lit.site.cfg.py.in | Exposes LLVM_ENABLE_PROJECTS to tests via config.enable_projects. |
| clang/test/lit.cfg.py | Adds a libdevice feature gate based on enabled projects. |
| clang/test/Driver/sycl-spirv-ext.cpp | Restricts test to Linux. |
| clang/test/Driver/sycl-spirv-default-options.cpp | Restricts test to Linux. |
| clang/test/Driver/sycl-offload-new-driver.cpp | Removes checks expecting clang-linker-wrapper to receive *.new.o device libraries. |
| clang/test/Driver/sycl-instrumentation.cpp | Updates expectations to verify -mlink-builtin-bitcode usage for ITT libs. |
| clang/test/Driver/sycl-device-lib.cpp | Updates expectations from *.new.o linkage to compile-time .bc linkage (and adds multi-target ASAN coverage). |
| clang/test/Driver/sycl-device-lib-diag.cpp | Adds a new test validating an error when expected SYCL device libraries are missing. |
| clang/lib/Driver/ToolChains/SYCL.h | Moves device-lib selection APIs onto SYCLToolChain and adds getDeviceLibs override. |
| clang/lib/Driver/ToolChains/SYCL.cpp | Implements compile-time device-lib linking for SPIR/SPIR-V new offload; adds missing-lib diagnostic; refactors sanitizer lib selection. |
| clang/lib/Driver/ToolChains/HIPAMD.cpp | Updates SYCLInstallationDetector construction to use host triple + args. |
| clang/lib/Driver/ToolChains/Cuda.cpp | Updates SYCLInstallationDetector construction to use host triple + args. |
| clang/lib/Driver/ToolChains/Clang.cpp | Updates SYCL installation detection construction and updates SYCL device-lib forwarding to wrapper (no longer passing -sycl-device-libraries). |
| clang/lib/Driver/Driver.cpp | Updates SYCL action builder to use new device-lib selection API (BitCodeLibraryInfo). |
| clang/include/clang/Basic/DiagnosticDriverKinds.td | Adds a new driver error for missing expected SYCL device libraries. |
Comments suppressed due to low confidence (1)
clang/lib/Driver/ToolChains/Clang.cpp:11615
- Device library directory detection still probes for
libsycl-crtwith a.new.o/.new.objsuffix whenIsNewOffloadis true, but this PR removes generation/use of the*.new.odevice libraries. As a result,DeviceLibDirmay not resolve correctly (e.g., when--sysrootis used), and the constructed--bitcode-library=<triple>=<DeviceLibDir>/<bc>paths can point at the wrong directory. Consider switching the probe tolibsycl-crt.bc(or another.bcsentinel) for the new offload model, or derive the directory directly fromSYCLInstallationDetector::getSYCLDeviceLibPathresults without relying on legacy object suffixes.
Args);
SYCLInstallation.getSYCLDeviceLibPath(LibLocCandidates);
SmallString<128> LibName("libsycl-crt");
bool IsNewOffload = D.getUseNewOffloadingDriver();
StringRef LibSuffix = TheTriple.isWindowsMSVCEnvironment()
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 20 out of 20 changed files in this pull request and generated 3 comments.
Comments suppressed due to low confidence (1)
clang/lib/Driver/ToolChains/Clang.cpp:11618
- This directory-detection logic still looks for libsycl-crt with a .new.o/.new.obj suffix when --offload-new-driver is enabled. After this PR, new-offload no longer produces/uses the *.new.o device libraries and relies on *.bc instead; using the .new.o probe can prevent selecting the sysroot/device-lib directory and lead to invalid --bitcode-library paths. Consider probing for the .bc libraries (or deriving the directory from SYCLInstallationDetector/getDeviceLibs) instead of the removed .new.o artifacts.
SmallString<128> LibName("libsycl-crt");
bool IsNewOffload = D.getUseNewOffloadingDriver();
StringRef LibSuffix = TheTriple.isWindowsMSVCEnvironment()
? (IsNewOffload ? ".new.obj" : ".obj")
: (IsNewOffload ? ".new.o" : ".o");
llvm::sys::path::replace_extension(LibName, LibSuffix);
YuriPlyakhin
left a comment
There was a problem hiding this comment.
Only reviewed the last 4 commits, assuming the first commits are identical to what was reviewed initially. lgtm.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 21 out of 21 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
clang/lib/Driver/ToolChains/Clang.cpp:11617
- Device library directory probing still looks for
libsycl-crtwith a.new.o/.new.objsuffix whenIsNewOffloadis true. This PR removes generation/usage of the*.new.odevice libraries and switches to linking*.bcat device compile, so this existence check can fail to find the correct sysroot/install lib directory and leaveDeviceLibDirpointing at the default driver-relative path. Consider probing for a.bclibrary (e.g.libsycl-crt.bc) regardless of offload model, or reuse the same device-lib discovery logic used for compile-time linking (so the wrapper receives correct--bitcode-librarypaths).
SmallString<128> LibName("libsycl-crt");
bool IsNewOffload = D.getUseNewOffloadingDriver();
StringRef LibSuffix = TheTriple.isWindowsMSVCEnvironment()
? (IsNewOffload ? ".new.obj" : ".obj")
: (IsNewOffload ? ".new.o" : ".o");
|
@intel/llvm-reviewers-runtime , @intel/dpcpp-cfe-reviewers , @intel/dpcpp-clang-driver-reviewers , please take a look - thanks! |
| // of device libraries that will be linked against. | ||
| for (const auto &DeviceLib : DeviceLibs) { | ||
| bool DeviceLibFound = false; | ||
| for (const auto &LibraryPath : LibraryPaths) { |
There was a problem hiding this comment.
If LibraryPaths is empty, we would get hit the diagnostic at L1944, which is about missing libraries. This might be confusing because the real root cause is that library search paths are not configured, not that the device lib is missing.
Can we check for missing/empty LibraryPaths and emit a different diagnostic for this case?
There was a problem hiding this comment.
For the case of LibraryPaths being empty, we can emit a different diagnostic. I will submit a bug report for making this update.
| /// Check for expected device library diagnostic. | ||
| // RUN: not %clangxx -fsycl --offload-new-driver %s -### 2>&1 \ | ||
| // RUN: | FileCheck %s -check-prefix=SYCL-DEVICE-LIB-DIAG | ||
| // SYCL-DEVICE-LIB-DIAG: error: cannot find expected SYCL device library 'libsycl-crt.bc'. Pass '--no-offloadlib' to build without the SYCL device libraries |
There was a problem hiding this comment.
cannot find expected SYCL device library 'libsycl-crt.bc'.
This does not tell the users about the full paths searched, ie, where the driver looked at for these libraries.
Is it possible to include the searched dir/path in the diagnostic?
There was a problem hiding this comment.
Maybe a note or remark of some kind after the list of missing libraries is emitted that detail the directories that were searched? I would hope that this situation is not common, as the installed compiler package should have these libraries available and they are not coming from an external source.
There was a problem hiding this comment.
I would hope that this situation is not common, as the installed compiler package should have these libraries available and they are not coming from an external source.
I thought there is not just a single-directory search, but more. Is that not the case?
There was a problem hiding this comment.
The installation candidates only contains one location to search for the device libs, and that is relative to the driver dir in the ../lib directory.
llvm/clang/lib/Driver/ToolChains/SYCL.cpp
Line 27 in e7e85e6
elizabethandrews
left a comment
There was a problem hiding this comment.
I'm not sure why the FE was tagged. I don't see any FE changes other than Driver code. I'm approving to unblock.
| /// Check for expected device library diagnostic. | ||
| // RUN: not %clangxx -fsycl --offload-new-driver %s -### 2>&1 \ | ||
| // RUN: | FileCheck %s -check-prefix=SYCL-DEVICE-LIB-DIAG | ||
| // SYCL-DEVICE-LIB-DIAG: error: cannot find expected SYCL device library 'libsycl-crt.bc'. Pass '--no-offloadlib' to build without the SYCL device libraries |
There was a problem hiding this comment.
Also, what will the device libs file-extension be on Windows?
There was a problem hiding this comment.
On Windows, *.bc files are used there as well.
|
@intel/llvm-gatekeepers please consider merging |
Pull in the device libraries during compile time when using the new offload model. This is done by using the -mlink-builtin-bitcode option, passing in each individual device library as needed.
This frees up the responsibility of linking in the device libraries during link time, and requiring the driver to pass any libraries needed to the clang-linker-wrapper or having the clang-linker-wrapper figure out what device libraries are needed at link.
With this move, the new.o device libraries are no longer used during the device linking stage using the clang-linker-wrapper. Made updates to the build to stop creating these binaries.
Reland of c2a9bf8
Updated the SYCLInstallationDetector constructor, as there were two and we only need one. The mix of these caused a disconnect in populating the library search locations.