The check-libomptarget fails when building with LLVM_ENABLE_PROJECTS. This is because test configuration misses the path to libomp.so and libLLVMSupport.so when time profiling is enabled (both libraries have the same path when building). This patch add the path to the configuration.
Details
Details
Diff Detail
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
| openmp/libomptarget/CMakeLists.txt | ||
|---|---|---|
| 71 | What about using "${CMAKE_CURRENT_BINARY_DIR}/../runtime/src" directly, like the one above? | |
| openmp/libomptarget/CMakeLists.txt | ||
|---|---|---|
| 71 | It looks like we should keep it as a cache variable, so that users may actually override it via cmake command even when the host runtime is also being built in-tree. But then it will probably not help to resolve the Support dependency... Alternatively, I guess, we can just keep the documentation untouched making it clear that LIBOMPTARGET_OPENMP_HOST_RTL_FOLDER is only honored for out-of-tree builds. | |
| openmp/libomptarget/CMakeLists.txt | ||
|---|---|---|
| 71 | Thanks for the comment Shilei! The path ${CMAKE_CURRENT_BINARY_DIR}/../runtime/src is correct for out-of-tree builds but not for in-tree ones. I've been actually digging more and perhaps the right way to go is to remove if(OPENMP_STANDALONE_BUILD) and unify those branch to: set(LIBOMPTARGET_OPENMP_HEADER_FOLDER "${LIBOMP_INCLUDE_DIR}")
set(LIBOMPTARGET_OPENMP_HOST_RTL_FOLDER "${LIBOMP_LIBRARY_DIR}")since both LIBOMP_INCLUDE_DIR, LIBOMP_LIBRARY_DIR are set in libomp building and available at this scope. Does that make sense? | |
| openmp/libomptarget/CMakeLists.txt | ||
|---|---|---|
| 71 | Thank you @vzakhari. When building out-of-tree, time profiling is not enabled so there is no dependency on Support. In my build, LIBOMPTARGET_OPENMP_HOST_RTL_FOLDER is needed for in-tree too even if time profiling is disabled, otherwise libomp.so is not in the LD_LIBRARY_PATH. | |
| openmp/libomptarget/CMakeLists.txt | ||
|---|---|---|
| 71 | Sounds good to me. From my local in-tree build, the structure is still correct: ➜ openmp tree -L 2 . ├── CMakeFiles ├── cmake_install.cmake ├── docs ├── libomptarget │ ├── CMakeFiles │ ├── cmake_install.cmake │ ├── deviceRTLs │ ├── libomptarget-nvptx-sm_75.bc │ ├── libomptarget.rtl.cuda.so │ ├── libomptarget.rtl.x86_64.so │ ├── libomptarget.so -> libomptarget.so.12git │ ├── libomptarget.so.12git │ ├── plugins │ ├── src │ └── test ├── runtime │ ├── CMakeFiles │ ├── cmake_install.cmake │ ├── src │ │ ├── CMakeFiles │ │ ├── cmake_install.cmake │ │ ├── kmp_config.h │ │ ├── kmp_i18n_default.inc │ │ ├── kmp_i18n_id.inc │ │ ├── libgomp.so -> libomp.so │ │ ├── libiomp5.so -> libomp.so │ │ ├── libomp.so │ │ ├── omp.h │ │ └── omp-tools.h │ └── test └── tools ➜ openmp pwd /nvm/0/shiltian/build/llvm/onepass/runtimes/runtimes-bins/openmp | |
| openmp/libomptarget/CMakeLists.txt | ||
|---|---|---|
| 71 | I am just trying to say that with your changes LIBOMPTARGET_OPENMP_HOST_RTL_FOLDER is ignored in an in-tree build, thus, either the documentation or the code is incorrect :) | |
| openmp/libomptarget/CMakeLists.txt | ||
|---|---|---|
| 71 | I'm doing the build with LLVM_ENABLE_PROJECTS instead of LLVM_ENABLE_RUNTIMES and in that case libomp.so is not generated under <top>/runtime/src. I'll follow with another patch for LLVM_ENABLE_RUNTIMES because there is the more basic problem that it doesn't compile with time profiling enabled at the moment. | |
| 71 | @vzakhari Point taken. It looks both are problematic :) | |
| openmp/libomptarget/CMakeLists.txt | ||
|---|---|---|
| 71 | Yeah, I mentioned that in your patch. | |
| openmp/README.rst | ||
|---|---|---|
| 251 | IIUIC, omp target is a dependency for check-libomptarget, so it will always be built if testing is involved, so required seems too strong to me. I would rephrase this along these lines: "This option allows overriding the default libiomp.so in libomptarget testing. The path is also used to resolve LLVMSupport dependency for libomptarget builds with BUILD_SHARED_LIBS=ON and profiling enabled". The first statement also fits LIBOMPTARGET_OPENMP_HEADER_FOLDER description well. | |
| openmp/libomptarget/CMakeLists.txt | ||
| 73 | It is worth adding a comment that we expect that LIBOMP_LIBRARY_DIR may also be used for resolving LLVMSupport dependency for omptarget with BUILD_SHARED_LIBS=ON and profiling enabled. | |
| openmp/README.rst | ||
|---|---|---|
| 251 | I have updated the description for LLVMSupport. My understanding is that these variables are needed only for testing and that libomptarget can built only as a shared library? | |
| openmp/libomptarget/CMakeLists.txt | ||
| 73 | I have updated the string to include information on LLVMSupport for profiling. | |
| openmp/README.rst | ||
|---|---|---|
| 251 | I am not sure they are required for testing, since libomptarget LIT tests will force libomp.so build on their own and will take it from LIBOMP_LIBRARY_DIR, so the variable should be optional. I am okay with the current documentation, though. | |
| openmp/libomptarget/CMakeLists.txt | ||
| 73 | Thank you! | |
Comment Actions
Does this mean the tests will (now) find libomp.so without setting environment variables? Great if so!
Comment Actions
@tstellar Can this patch be cherry-picked into release/12.x? It fixes libomptarget testing in downstream compilers.
IIUIC, omp target is a dependency for check-libomptarget, so it will always be built if testing is involved, so required seems too strong to me.
I would rephrase this along these lines: "This option allows overriding the default libiomp.so in libomptarget testing. The path is also used to resolve LLVMSupport dependency for libomptarget builds with BUILD_SHARED_LIBS=ON and profiling enabled".
The first statement also fits LIBOMPTARGET_OPENMP_HEADER_FOLDER description well.