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

[Giorgis Georgakoudis via Phabricator via Openmp-commits]

ggeorgakoudis added inline comments.


================
Comment at: openmp/README.rst:251
 **LIBOMPTARGET_OPENMP_HOST_RTL_FOLDER** = ``""``
-  Path of the folder that contains ``libomp.so``.  This is required for testing
-  out-of-tree builds.
+  Path of the folder that contains ``libomp.so``.  This is required for testing.
 
----------------
vzakhari wrote:
> 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.
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?


================
Comment at: openmp/libomptarget/CMakeLists.txt:73
+  "Path to folder containing omp.h")
+set(LIBOMPTARGET_OPENMP_HOST_RTL_FOLDER "${LIBOMP_LIBRARY_DIR}" CACHE STRING
+  "Path to folder containing libomp.so")
----------------
vzakhari wrote:
> 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.
I have updated the string to include information on `LLVMSupport` for profiling.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D95376