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

Mon Jan 25 14:43:13 PST 2021

vzakhari 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.

----------------
ggeorgakoudis wrote:
> 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?
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.

================
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")
----------------
ggeorgakoudis wrote:
> 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.
Thank you!

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D95376