[Libomptarget] Output the DeviceRTL alongside the other libraries #73705

jhuber6 · 2023-11-28T22:05:43Z

Summary:
Currently, the libomp.so and libomptarget.so are emitted in the
./lib build directory generally. This logic is internal to the
add_llvm_library function we use to build libomptarget. The
DeviceRTl static library however is in the middle of the OpenMP runtime
build, which can vary depending on if this is a runtimes or projects
build. This patch changes this to install the DeviceRTL static library
alongside the other OpenMP libraries so they are easier to find.

shiltian · 2023-11-28T22:19:12Z

openmp/libomptarget/DeviceRTL/CMakeLists.txt

-set_target_properties(omptarget.devicertl PROPERTIES LINKER_LANGUAGE CXX)
+set_target_properties(omptarget.devicertl PROPERTIES
+                      LINKER_LANGUAGE CXX
+                      ARCHIVE_OUTPUT_DIRECTORY ${LLVM_LIBRARY_OUTPUT_INTDIR}


Will this break standalone build?

I was thinking about that and wasn't sure. As far as I can tell, add_llvm_library uses this string internally, which is how libomptarget ends up where it does. So I'm thinking so long as we're using that, it will be defined somewhere. But I haven't tested it or anything.

I guess that's because install(TARGETS omptarget.devicertl ARCHIVE DESTINATION ${OPENMP_INSTALL_LIBDIR}) overrides it, which is what we need; otherwise it will be installed into where LLVM is installed, which is not right if build standalone.

I could change it to only do this if it's standalone, should probably be fine.

Can we build the DeviceRTL standalone? Do we want to support that?

I guess theoretically? I've never heard of anyone doing it though. It should ideally just require an up-to-date clang compiler and an LLVM installation.

I'm constantly using standalone build.

Can you test this then? Not really sure what the expected output would be. I guess the issue happens with tests, but do we even support tests out of tree?

shiltian · 2023-11-28T22:43:57Z

openmp/libomptarget/DeviceRTL/CMakeLists.txt

@@ -304,4 +304,10 @@ add_library(omptarget.devicertl STATIC)
 set_target_properties(omptarget.devicertl PROPERTIES LINKER_LANGUAGE CXX)
 target_link_libraries(omptarget.devicertl PRIVATE omptarget.devicertl.all_objs)

+# Install this alongside the LLVM libraries is possible.
+if(NOT OPENMP_STANDALONE_BUILD)


I think this may break standalone build, especially with the changes in lit.cfg.

I'm still not sure if this is even an issue, considering we use add_llvm_library which already uses this value internally and does the same operation to set its output directory.

shiltian · 2023-11-28T22:53:09Z

openmp/libomptarget/test/lit.cfg

-               config.library_dir + "/libomptarget.devicertl.a"
-    return source + " " + config.library_dir + "/libomptarget.devicertl.a"
+               config.llvm_library_dir + "/libomptarget.devicertl.a"
+    return source + " " + config.llvm_library_dir + "/libomptarget.devicertl.a"


But this change can affect both right? In standalone build, I think it is still config.library_dir instead of config.llvm_library_dir.

In the general case, AFAICT both the standalone and LLVM build just default to ./lib as their output, so it'll succeed superficially. If we want to be sure I could manually set the output to something that's either the LLVM dir or the OpenMP one and then make a new lit variable.

With that being said, I'm not sure what this patch tries to fix or improve. Does it try to set the output path at build time or install time?

It's build time. The issue is that I wanted libc to fish this out of the install directory, but where it's written to is variable and in a random directory. I wanted it to be next to libomptarget.so so I have a single static search path I can use for internal resolution of the library.

I tried your patch and I think it will indeed break standalone build.

config.library_dir = "/gpfs/jlse-fs0/users/ac.shilei.tian/build/openmp/release/libomptarget" config.llvm_library_dir = "/gpfs/jlse-fs0/users/ac.shilei.tian/build/llvm/release/./lib"

The DeviceRTL library is in /gpfs/jlse-fs0/users/ac.shilei.tian/build/openmp/release/libomptarget.

And even w/o this patch, libomptarget.devicertl.a sits next to libomptarget.so.

➜ libomptarget ll total 4.1M drwxr-xr-x 8 ac.shilei.tian jlse 4.0K Nov 28 18:20 ./ drwxr-xr-x 8 ac.shilei.tian jlse 4.0K Nov 28 18:19 ../ drwxr-xr-x 2 ac.shilei.tian jlse 4.0K Nov 28 18:19 CMakeFiles/ -rw-r--r-- 1 ac.shilei.tian jlse 2.4K Nov 28 18:19 cmake_install.cmake drwxr-xr-x 3 ac.shilei.tian jlse 256K Nov 28 18:20 DeviceRTL/ -rw-r--r-- 1 ac.shilei.tian jlse 1.8M Nov 28 18:20 libomptarget.devicertl.a lrwxrwxrwx 1 ac.shilei.tian jlse 32 Nov 28 18:20 libomptarget.rtl.amdgpu.so -> libomptarget.rtl.amdgpu.so.18git* -rwxr-xr-x 1 ac.shilei.tian jlse 443K Nov 28 18:20 libomptarget.rtl.amdgpu.so.18git* lrwxrwxrwx 1 ac.shilei.tian jlse 30 Nov 28 18:20 libomptarget.rtl.cuda.so -> libomptarget.rtl.cuda.so.18git* -rwxr-xr-x 1 ac.shilei.tian jlse 360K Nov 28 18:20 libomptarget.rtl.cuda.so.18git* lrwxrwxrwx 1 ac.shilei.tian jlse 32 Nov 28 18:20 libomptarget.rtl.x86_64.so -> libomptarget.rtl.x86_64.so.18git* -rwxr-xr-x 1 ac.shilei.tian jlse 303K Nov 28 18:20 libomptarget.rtl.x86_64.so.18git* lrwxrwxrwx 1 ac.shilei.tian jlse 21 Nov 28 18:20 libomptarget.so -> libomptarget.so.18git* -rwxr-xr-x 1 ac.shilei.tian jlse 209K Nov 28 18:20 libomptarget.so.18git* drwxr-xr-x 11 ac.shilei.tian jlse 4.0K Nov 28 18:19 plugins-nextgen/ drwxr-xr-x 3 ac.shilei.tian jlse 4.0K Nov 28 18:19 src/ drwxr-xr-x 8 ac.shilei.tian jlse 4.0K Nov 28 18:19 test/ drwxr-xr-x 5 ac.shilei.tian jlse 4.0K Nov 28 18:19 tools/

Definitely doesn't in my build tree, both projects and runtimes. I think standalone defaults to ./lib as well?

It is not in ./lib because it is build time. Eventually after installation it will be in ./lib, but it is in config.library_dir.

Ah, the build is what I care about. It currently gets installed to the same place, yes. I'm fishing these libraries out of the user's build so it needs to be in a consistent place.

Have you tried ninja -t missingdeps?

jdoerfert · 2023-11-28T23:04:35Z

I'm fine with this, Shilei will verify standalone build is not broken.

Summary: Currently, the `libomp.so` and `libomptarget.so` are emitted in the `./lib` build directory generally. This logic is internal to the `add_llvm_library` function we use to build `libomptarget`. The DeviceRTl static library however is in the middle of the OpenMP runtime build, which can vary depending on if this is a runtimes or projects build. This patch changes this to install the DeviceRTL static library alongside the other OpenMP libraries so they are easier to find.

shiltian

LG

jhuber6 requested review from jdoerfert and shiltian November 28, 2023 22:05

llvmbot added the openmp:libomptarget OpenMP offload runtime label Nov 28, 2023

shiltian reviewed Nov 28, 2023

View reviewed changes

jhuber6 force-pushed the LibraryLocation branch 2 times, most recently from a8f0245 to 014c68d Compare November 30, 2023 21:14

jhuber6 force-pushed the LibraryLocation branch from 014c68d to 318526f Compare November 30, 2023 21:32

shiltian approved these changes Nov 30, 2023

View reviewed changes

jhuber6 merged commit 0ec4b82 into llvm:main Nov 30, 2023

tuliom mentioned this pull request Dec 1, 2023

[libomptarget] Regression when LLVM_LIBRARY_OUTPUT_INTDIR is not set #74079

Closed

[Libomptarget] Output the DeviceRTL alongside the other libraries #73705

[Libomptarget] Output the DeviceRTL alongside the other libraries #73705

Uh oh!

Conversation

jhuber6 commented Nov 28, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jdoerfert commented Nov 28, 2023

Uh oh!

shiltian left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!