Repository navigation
feat(compiler): Enable LLVM_ENABLE_PER_TARGET_RUNTIME_DIR - #7082
Conversation
|
The patch files here will be removed later once they trickle into TheRock. I just need to test everything together. |
✅ All Checks Passed — Ready for Review
📖 Need help? See the Policy FAQ for details on every check and how to fix failures. |
therock-pr-bot
Bot
added
the
Not ready to Review
|
🎉 All checks passed! This PR is ready for review. |
2456917 to
9072888
Compare
…0522) I've been monitoring the rocjitsu race-detection sidecar that landed in [PR #9969](#9969). The first sidecars to run after the merge were triggered on [PR #9631](#9631), and both failed before reaching either detector workload: ```text OpenMP runtime not found: .../lib/llvm/lib/x86_64-unknown-linux-gnu/libomp.so ``` This looked familiar because a missing `libomp.so` was one of the reasons the original sidecar was temporarily reverted in [PR #9938](#9938). The original failure and the current failure have the same underlying dependency, but the compiler artifact layout changed: - PR #9969's green CI proof used a compiler artifact with `libomp.so` under: ```text lib/llvm/lib/x86_64-unknown-linux-gnu/libomp.so ``` - Current `develop` uses a newer TheRock revision that includes [TheRock PR #6802](ROCm/TheRock#6802), which reverted host per-target runtime directories. Its compiler artifact places the runtime at: ```text lib/llvm/lib/libomp.so ``` [TheRock PR #7082](ROCm/TheRock#7082) is now re-enabling `LLVM_ENABLE_PER_TARGET_RUNTIME_DIR`. Changing the hard-coded path to the current flat location would therefore regress when the nested layout returns. This PR instead supports both expected layouts explicitly. The sidecar script added the historical host-triple directory to `LD_LIBRARY_PATH`, but it also required `libomp.so` to exist at that exact path. With the current artifact, the parent LLVM library directory is already in `LD_LIBRARY_PATH`, but the hard-coded preflight check exits before the workload can run. This PR replaces that exact-path assumption with a deterministic ordered lookup: 1. `lib/llvm/lib/libomp.so` 2. `lib/llvm/lib/<machine>-unknown-linux-gnu/libomp.so` If neither expected path contains `libomp.so`, the script still fails early and prints both attempted locations. ## Validation Focused validation invoked the real script against temporary ROCm roots while stubbing the later compiler invocation. The observed results were: ```text flat: LIBOMP_PATH=.../lib/llvm/lib/libomp.so lib/llvm/lib appears once in LD_LIBRARY_PATH nested: LIBOMP_PATH=.../lib/llvm/lib/x86_64-unknown-linux-gnu/libomp.so both the root and host-triple directories appear in LD_LIBRARY_PATH missing: OpenMP runtime not found at either expected path: .../lib/llvm/lib/libomp.so .../lib/llvm/lib/x86_64-unknown-linux-gnu/libomp.so layout validation passed: flat, nested, missing ``` ## Issue Tracking Related: AIHPBLAS-3605
7101087 to
e4cb2e4
Compare
e4cb2e4 to
7cc4543
Compare
c79c047 to
5a46503
Compare
|
Failing checks seem to be unrelated (Tests for: rocgdb-corefile, rocgdb-gpu, rocprofiler-sdk, rocr-debug-agent). Will retrigger failing checks. |
ScottTodd
left a comment
ScottTodd
left a comment
There was a problem hiding this comment.
Generally LGTM. I'm a bit worried that downstream consumers of ROCm may be depending on the current directory so they may need similar extensions to LD_LIBRARY_PATH to pick up the host triple path though...
| def setup_env(): | ||
| environ_vars["ROCM_PATH"] = str(THEROCK_PATH) | ||
| environ_vars["ROCPROFSYS_INSTALL_DIR"] = str(THEROCK_PATH) | ||
| environ_vars["ROCPROFSYS_MAX_THREADS"] = "64" | ||
|
|
||
| old_path = os.getenv("PATH", "") | ||
| rocm_bin = str(THEROCK_BIN_PATH) | ||
| environ_vars["PATH"] = f"{rocm_bin}:{old_path}" if old_path else rocm_bin | ||
|
|
||
| ld_paths = [ | ||
| str(THEROCK_PATH / "share" / "rocprofiler-systems" / "examples" / "lib"), | ||
| ] | ||
| if host_triple and THEROCK_LLVM_LIB_HOST_TRIPLE_PATH.exists(): | ||
| ld_paths += [f"{THEROCK_LLVM_LIB_HOST_TRIPLE_PATH}"] | ||
| ld_paths_str = ":".join(ld_paths) | ||
| old_ld_path = os.getenv("LD_LIBRARY_PATH", "") | ||
| environ_vars["LD_LIBRARY_PATH"] = ( | ||
| f"{ld_paths_str}:{old_ld_path}" if old_ld_path else ld_paths_str | ||
| ) |
There was a problem hiding this comment.
Not needed as part of this PR, but we could extract some of the common LD_LIBRARY_PATH environment setup to a shared script handled as part of initializing the test artifacts. I've been wanting something like that for ASan setup too (a script that sets LD_LIBRARY_PATH and LD_PRELOAD by following the instructions at https://github.com/ROCm/TheRock/blob/main/docs/development/sanitizers.md#using-asan-instrumented-libraries)
See also:
(That's more user-facing for building/running applications after installing ROCm, while this would be for running ROCm tests)
| BUILD_DEPS | ||
| amd-llvm | ||
| therock-googletest |
There was a problem hiding this comment.
Is this new build dep on amd-llvm (and the corresponding change to test_tools/therock_consumer_graph.json) for mxDataGenerator really needed? Most projects just need to depend on the COMPILER_TOOLCHAIN.
There was a problem hiding this comment.
mxDataGenerator (interface library) has target_link_libraries(mxDataGenerator INTERFACE OpenMP::OpenMP_CXX). mxDataGenerator-config.cmake then uses the openmp target for INTERFACE_LINK_LIBRARIES:
set_target_properties(mxDataGenerator PROPERTIES INTERFACE_LINK_LIBRARIES "OpenMP::OpenMP_CXX" ...
The amd-hip toolchain is not enough to resolve this openmp target in the superproject provided packages. amd-llvm subproject is updated in this patch to provide openmp via therock_cmake_subproject_provide_package(amd-llvm OpenMP lib/llvm/lib/cmake/openmp).
At least locally, the changes to The change to |
This is needed to detect the openmp-config.cmake from amd-llvm.
estewart08
added
the
ci:host-asan
…profiler_systems.py This resolves libomp.so not found when LLVM_ENABLE_PER_TARGET_DIR=ON.
This will be committed directly to rocm-libraries.
This will be promoted in a SMP update, I just need to test this with everything combined.
…inux * The sanitizer RPATH directories hardcoded lib/linux, so every ASAN instrumented executable got a dead BUILD_RPATH entry. Since clang links the shared runtime by absolute path without adding an -rpath, an instrumented build time host tool has no way to find libclang_rt.asan.so and dies with "error while loading shared libraries", stopping the build. * THEROCK_SANITIZER_LAUNCHER resolved the runtime by querying the arch suffixed libclang_rt.asan-x86_64.so. clang echoes an unfound query back verbatim, so the launcher became LD_PRELOAD=libclang_rt.asan-x86_64.so, which ld.so reports and then ignores, silently running build time tools with no sanitizer at all. Both now derive the runtime location from clang and only accept a location that actually holds the runtime. The reported runtime dir is used only when it really contains shared sanitizer runtimes, since a toolchain can report a per-target dir holding only flang_rt while compiler-rt stays in lib/linux; otherwise the RPATH derivation falls back to the previous lib/linux behavior with a diagnostic instead of emitting a dead entry. Failing to resolve the launcher runtime is a hard error rather than a warning: an unresolvable LD_PRELOAD is ignored by the loader, so continuing would run build time tools completely unsanitized while appearing to work, which is the exact failure mode this is meant to eliminate. A sanitized build should either be sanitized or fail. The error names the sanitizer, compiler, resource dir, runtime dir and every soname probed with its raw --print-file-name output so it can be diagnosed from the message alone.
This will be applied to rocm-systems. Test only.
This reverts commit 5fcd371. Patch landed in rccl.
This reverts commit 8007faa. Patch landed in amd-llvm.
This reverts commit e4078b9. Patch landed in rocm-libraries.
The rpp tests are built with a separate cmake invocation in test_rpp.py and do not inherit the RPATH set after each subproject is configured. LLVM_ENABLE_PER_TARGET_DIR=ON moves the location of libomp.so down one level, which causes tests to fail.
Initialize host_triple for error checking. Ensure THEROCK_LLVM_LIB_HOST_TRIPLE_PATH exists.
mxdatagenerator uses the openmp config from amd-llvm.
5a46503 to
221086b
Compare
|
Had to rebase and fix conflict in compiler/CMakeLists.txt. |
Pre-commit check failed⛔ pre-commit failed Please run locally:
This repo uses |
therock-pr-bot
Bot
removed
the
Not ready to Review
…rride (#4179) ## Downstream delta removed `amd-staging` carries a one-line AMD-local override that clobbers upstream's platform default immediately after it is computed: ```cmake if(CMAKE_SYSTEM_NAME MATCHES "BSD|Linux|OS390|AIX") set(LLVM_ENABLE_PER_TARGET_RUNTIME_DIR_default ON) else() set(LLVM_ENABLE_PER_TARGET_RUNTIME_DIR_default OFF) endif() set(LLVM_ENABLE_PER_TARGET_RUNTIME_DIR_default OFF) # <-- removed ``` After this change `llvm/CMakeLists.txt` has **zero** `LLVM_ENABLE_PER_TARGET_RUNTIME_DIR` divergence from `upstream/main`. Upstream is moving toward keeping this ON by default and dropping the option entirely, so carrying the override moves us further from upstream over time. ## Why it existed ROCm consumers assumed the flat `lib/llvm/lib` layout. With the per-target directory on, `libomp.so` moves to `lib/llvm/lib/<host-triple>/` while consumer RUNPATHs still point at `lib/llvm/lib` — the library ships but cannot be loaded. ## Why TheRock is unaffected TheRock passes `-DLLVM_ENABLE_PER_TARGET_RUNTIME_DIR` explicitly (ROCm/TheRock#7559; ROCm/TheRock#7082 moves it to `ON` and fixes the consumers). A command-line `-D` beats a `set(... CACHE ...)` without `FORCE`, so TheRock's value wins regardless of the default here.
This breaks backwards compatibility with binaries/libraries built with 10.0. Original PR: #7082 This is a limited revert that restores the old library layout of lib/llvm/lib while keeping other infrastructure patches in place for a future reland.
This breaks backwards compatibility with binaries/libraries built with 10.0. Original PR: #7082 This is a limited revert that restores the old library layout of lib/llvm/lib while keeping other infrastructure patches in place for a future reland. JIRA ID: ROCM-30441
Motivation
Add support to use LLVM_ENABLE_PER_TARGET_RUNTIME_DIR=ON. This option is currently disabled by default in ROCm. There is talk upstream of keeping this ON by default and removing the option entirely. This also is a prequel step to using multlib build variants for debug/asan libraries for openmp/offload.
The layout change has broad ripple effects:
LLVM_ENABLE_PER_TARGET_RUNTIME_DIR=ON; amd-llvm provides openmp packageTest Plan
Inspect current CI tests with my PR's modifications, which should not show any issues loading libraries found in lib/llvm/lib/x86_64-unknown-linux-gnu or lib/clang/23/lib/x86_64-unknown-linux-gnu. This needs to include asan builds.
ROCmValidationSuite (RVS) needs updated as well as the rvs binary uses libomp.so:
ROCm/ROCmValidationSuite#1301
Fixes:
ISSUE ID: #6801
Partially AI Assisted