[OpenMP] Respect LLVM per-target install directories #83282
Conversation
Summary: One recurring problem we have with the OpenMP libraries is that they are potentially conflicting with ones found on the system, this occurs when there are two copies and one is used for linking that it not attached to the correspoding clang compiler. LLVM already uses target specific directories for this, like with libc++, which are always searched first. This patch changes the install directory to be `lib/x86_64-unknown-linux-gnu` for example. Notable changes would be that users will need to change their LD_LIBRARY_PATH settings optionally, or use default rt-rpath options. This should fix problems were users are linking the wrong versions of static libraries
jhuber6
requested review from
alexey-bataev,
carlobertolli,
jdoerfert,
JonChesterfield,
MaskRay,
ronlieb and
shiltian
llvmbot
added
clang
|
@llvm/pr-subscribers-clang @llvm/pr-subscribers-clang-driver Author: Joseph Huber (jhuber6) ChangesSummary: Notable changes would be that users will need to change their Full diff: https://github.com/llvm/llvm-project/pull/83282.diff 4 Files Affected:
diff --git a/clang/lib/Driver/ToolChains/CommonArgs.cpp b/clang/lib/Driver/ToolChains/CommonArgs.cpp
index faceee85a2f8dc..382c8b3612a0af 100644
--- a/clang/lib/Driver/ToolChains/CommonArgs.cpp
+++ b/clang/lib/Driver/ToolChains/CommonArgs.cpp
@@ -2763,13 +2763,13 @@ void tools::addOpenMPDeviceRTL(const Driver &D,
const llvm::opt::ArgList &DriverArgs,
llvm::opt::ArgStringList &CC1Args,
StringRef BitcodeSuffix,
- const llvm::Triple &Triple) {
+ const llvm::Triple &Triple,
+ const ToolChain &HostTC) {
SmallVector<StringRef, 8> LibraryPaths;
- // Add path to clang lib / lib64 folder.
- SmallString<256> DefaultLibPath = llvm::sys::path::parent_path(D.Dir);
- llvm::sys::path::append(DefaultLibPath, CLANG_INSTALL_LIBDIR_BASENAME);
- LibraryPaths.emplace_back(DefaultLibPath.c_str());
+ // Check all of the standard library search paths used by the compiler.
+ for (const auto &LibPath : HostTC.getFilePaths())
+ LibraryPaths.emplace_back(LibPath);
// Add user defined library paths from LIBRARY_PATH.
std::optional<std::string> LibPath =
diff --git a/clang/lib/Driver/ToolChains/CommonArgs.h b/clang/lib/Driver/ToolChains/CommonArgs.h
index 2db0f889ca8209..b8f649aab4bdd2 100644
--- a/clang/lib/Driver/ToolChains/CommonArgs.h
+++ b/clang/lib/Driver/ToolChains/CommonArgs.h
@@ -214,7 +214,8 @@ void addMachineOutlinerArgs(const Driver &D, const llvm::opt::ArgList &Args,
void addOpenMPDeviceRTL(const Driver &D, const llvm::opt::ArgList &DriverArgs,
llvm::opt::ArgStringList &CC1Args,
- StringRef BitcodeSuffix, const llvm::Triple &Triple);
+ StringRef BitcodeSuffix, const llvm::Triple &Triple,
+ const ToolChain &HostTC);
void addOutlineAtomicsArgs(const Driver &D, const ToolChain &TC,
const llvm::opt::ArgList &Args,
diff --git a/clang/lib/Driver/ToolChains/Cuda.cpp b/clang/lib/Driver/ToolChains/Cuda.cpp
index ff3687ca7dae33..177fd6310e7ee2 100644
--- a/clang/lib/Driver/ToolChains/Cuda.cpp
+++ b/clang/lib/Driver/ToolChains/Cuda.cpp
@@ -903,7 +903,7 @@ void CudaToolChain::addClangTargetOptions(
return;
addOpenMPDeviceRTL(getDriver(), DriverArgs, CC1Args, GpuArch.str(),
- getTriple());
+ getTriple(), HostTC);
}
}
diff --git a/openmp/CMakeLists.txt b/openmp/CMakeLists.txt
index 03068af22629f7..3c4ff76ad6d161 100644
--- a/openmp/CMakeLists.txt
+++ b/openmp/CMakeLists.txt
@@ -46,9 +46,15 @@ if (OPENMP_STANDALONE_BUILD)
set(CMAKE_CXX_EXTENSIONS NO)
else()
set(OPENMP_ENABLE_WERROR ${LLVM_ENABLE_WERROR})
- # If building in tree, we honor the same install suffix LLVM uses.
- set(OPENMP_INSTALL_LIBDIR "lib${LLVM_LIBDIR_SUFFIX}" CACHE STRING
- "Path where built OpenMP libraries should be installed.")
+
+ # When building in tree we install the runtime according to the LLVM settings.
+ if(LLVM_ENABLE_PER_TARGET_RUNTIME_DIR AND NOT APPLE)
+ set(OPENMP_INSTALL_LIBDIR lib${LLVM_LIBDIR_SUFFIX}/${LLVM_DEFAULT_TARGET_TRIPLE} CACHE STRING
+ "Path where built openmp libraries should be installed.")
+ else()
+ set(OPENMP_INSTALL_LIBDIR "lib${LLVM_LIBDIR_SUFFIX}" CACHE STRING
+ "Path where built OpenMP libraries should be installed.")
+ endif()
if (NOT MSVC)
set(OPENMP_TEST_C_COMPILER ${LLVM_RUNTIME_OUTPUT_INTDIR}/clang)
|
| SmallString<256> DefaultLibPath = llvm::sys::path::parent_path(D.Dir); | ||
| llvm::sys::path::append(DefaultLibPath, CLANG_INSTALL_LIBDIR_BASENAME); | ||
| LibraryPaths.emplace_back(DefaultLibPath.c_str()); | ||
| // Check all of the standard library search paths used by the compiler. |
There was a problem hiding this comment.
clang/test/Driver/linux-cross.cpp has examples how to test -L.
|
We are having some problems with this patch on a server where the file /lib64/libomptarget-nvptx-sm_52.bc exists. Problem 1 Problem 2 Now, with your path, I guess it starts picking up the no longer holds with your patch. |
I think it's standard to prioritize library path stuff. Does this work if you just flip the order we fill the library search path? I think the behavioral change here is that we didn't used to look in the system directory. |
|
Problem 1 can be solved by flipping the order. |
Honestly, we should just remove the second test. We just treat these things as libraries and it doesn't make sense for a test to ensure that |
#83573 I made a patch to fix it. |
Should libomptarget follow the relocation of libomp? |
That's surprising. Maybe this is interacting incorrectly with the fact that we build these as LLVM libs? |
|
It seems being installed twice both under |
Makes sense, like |
|
@jhuber6 unfortunately after 2fb764d amdgpu plut is missing. |
This is confusing. I have it in my local install no problem and the |
|
Still a myth on my side, I saw but there is no installing of the |
|
@jhuber6 could you build openmp as a project instead of runtime? |
Ah, I could try that. Though I believe that Johannes is going to completely deprecate the projects build once moving to llvm/offload. |
@jdoerfert I would like to see the device code compilation (on device runtime) and host runtime compilation fully separate. Then I can build the runtime with gcc or sanitizer without disturbing device code compilation. |
Could you elaborate on this? One of my long-term goals is to build the OpenMP device runtime similarly to how I build the GPU C library. That is, we have a |
I need a clean way to add |
We could potentially do what I do with the |
|
Could you explain what each line does exactly? |
This is hypothetical, but it's a potential way to keep it from having a separate project -DLLVM_ENABLE_RUNTIMES="openmp" -DRUNTIMES_amdgcn-amd-amdhsa_LLVM_ENABLE_RUNTIMES=openmp Theoretically this would let us treat these as separate builds so you could pass totally different things to them. Just throwing ideas around. |
|
I'm OK with the first two I actually think default should automatically include amdgcn and nvptx when I have amdgcn and nvptx backend turned on. Then openmp cmake can react to the contents of |
|
Fixed the issue 0fa04b6 |
Hah, I probably should've noticed that. Explains why I didn't notice because I always have tests enabled. |
Summary: One recurring problem we have with the OpenMP libraries is that they are potentially conflicting with ones found on the system, this occurs when there are two copies and one is used for linking that it not attached to the correspoding clang compiler. LLVM already uses target specific directories for this, like with libc++, which are always searched first. This patch changes the install directory to be `lib/x86_64-unknown-linux-gnu` for example. Notable changes would be that users will need to change their LD_LIBRARY_PATH settings optionally, or use default rt-rpath options. This should fix problems were users are linking the wrong versions of static libraries
Summary:
One recurring problem we have with the OpenMP libraries is that they are
potentially conflicting with ones found on the system, this occurs when
there are two copies and one is used for linking that it not attached to
the correspoding clang compiler. LLVM already uses target specific
directories for this, like with libc++, which are always searched first.
This patch changes the install directory to be
lib/x86_64-unknown-linux-gnufor example.Notable changes would be that users will need to change their
LD_LIBRARY_PATH settings optionally, or use default rt-rpath options.
This should fix problems were users are linking the wrong versions of
static libraries