Repository navigation
Conversation
The linker-wrapper job located all SYCL device libraries by probing for libsycl-crt.bc and assuming every other library lived alongside it. That library isn't one of the NVPTX/AMDGCN/NativeCPU libraries actually passed, and missing libraries were silently passed with a bogus path. Add SYCLInstallationDetector::findSYCLDeviceLib() to search the device library locations for a given library, and use it in SYCLToolChain::getDeviceLibs and the sycl-post-link bfloat16 lookup. --bitcode-library= paths now come from getDeviceLibs, which diagnoses missing libraries. -sycl-device-library-location= is derived from the library the wrapper consumes (NativeCPU utils or native bfloat16). Update sycl-bc-device-libraries.cpp to use -ccc-install-dir so the test is independent of the build tree, and add a missing-library check. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
iclsrc
left a comment
iclsrc
left a comment
There was a problem hiding this comment.
This PR adds SYCLInstallationDetector::findSYCLDeviceLib and uses it in place of several hand-rolled search loops. In the linker wrapper, --bitcode-library paths now come from getDeviceLibs, so they are full resolved paths. -sycl-device-library-location is now derived from where a representative library (nativecpu_utils or native-bfloat16) is found. The refactor looks correct and removes duplication. I found no silent-miscompilation or SYCL pass-invariant issues, because the change is driver-only. One test gap and one behavior note are below.
| DeviceLibPaths.emplace_back(Path.str()); | ||
| } | ||
|
|
||
| std::optional<SmallString<128>> |
There was a problem hiding this comment.
🔵 Suggestion: findSYCLDeviceLib now uses D.getVFS().exists(). The code it replaces used llvm::sys::fs::exists. This is arguably better, since it honors -ivfsoverlay. It is still a behavior change for all callers, so it is worth mentioning in the PR description. The header also uses std::optional and should include <optional> explicitly, to avoid relying on transitive includes.
| @@ -1,13 +1,21 @@ | |||
| /// Test that SYCL bitcode device libraries are properly separated for NVIDIA and AMD targets. | |||
|
|
|||
| /// Check that a device library which cannot be found is diagnosed. | |||
There was a problem hiding this comment.
IIUC, the folder passed in -sycl-device-library-location= is now found by searching for libsycl-native-bfloat16.bc (or libsycl-nativecpu_utils.bc for NativeCPU) instead of libsycl-crt.bc.
In Inputs/SYCL, libsycl-crt.bc is in lib/dpcpp-7/sycl, but those two files are only in lib/.
So on a Linux host, this test's flag should change from .../bin/../lib/dpcpp-7/sycl to .../bin/../lib.
That's also the fallback value, so the test can't tell whether the search found the file or gave up.
Could you add dummy libsycl-native-bfloat16.bc and libsycl-nativecpu_utils.bc under Inputs/SYCL/lib/dpcpp-7/sycl, plus a RUN line with --target=x86_64-unknown-linux-gnu that checks the flag points there?
That would match the Linux install layout (lib/dpcpp-/sycl) and actually exercise the new search.
This branch has not been deployed
4 participants
The linker-wrapper job located all SYCL device libraries by probing for libsycl-crt.bc and assuming every other library lived alongside it. That library isn't one of the NVPTX/AMDGCN/NativeCPU libraries actually passed, and missing libraries were silently passed with a bogus path.
Add SYCLInstallationDetector::findSYCLDeviceLib() to search the device library locations for a given library, and use it in SYCLToolChain::getDeviceLibs and the sycl-post-link bfloat16 lookup. --bitcode-library= paths now come from getDeviceLibs, which diagnoses missing libraries. -sycl-device-library-location= is derived from the library the wrapper consumes (NativeCPU utils or native bfloat16).
Update sycl-bc-device-libraries.cpp to use -ccc-install-dir so the test is independent of the build tree, and add a missing-library check.