[Openmp-commits] [PATCH] D95376: [OpenMP][Libomptarget] Fix check-libomptarget

[Shilei Tian via Phabricator via Openmp-commits]

tianshilei1992 added inline comments.


================
Comment at: openmp/libomptarget/CMakeLists.txt:78
   set(LIBOMPTARGET_OPENMP_HEADER_FOLDER "${CMAKE_CURRENT_BINARY_DIR}/../runtime/src")
+  set(LIBOMPTARGET_OPENMP_HOST_RTL_FOLDER "${LIBOMP_LIBRARY_DIR}")
 endif()
----------------
ggeorgakoudis wrote:
> ggeorgakoudis wrote:
> > vzakhari wrote:
> > > tianshilei1992 wrote:
> > > > ggeorgakoudis wrote:
> > > > > ggeorgakoudis wrote:
> > > > > > vzakhari wrote:
> > > > > > > tianshilei1992 wrote:
> > > > > > > > What about using `"${CMAKE_CURRENT_BINARY_DIR}/../runtime/src"` directly, like the one above?
> > > > > > > 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.
> > > > > > 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?
> > > > > 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`.
> > > > 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
> > > > ```
> > > 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 :)
> > 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.
> @vzakhari Point taken. It looks both are problematic :)
Yeah, I mentioned that in your patch.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D95376/new/

https://reviews.llvm.org/D95376