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

[Vyacheslav Zakharin via Phabricator via Openmp-commits]

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


================
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")
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D95376