[Openmp-commits] [PATCH] D111983: [libomptarget][DeviceRTL] Generalise and simplify cmakelists

[Michael Kruse via Phabricator via Openmp-commits]

Meinersbur added inline comments.


================
Comment at: openmp/libomptarget/DeviceRTL/CMakeLists.txt:34-37
-set(LIBOMPTARGET_NVPTX_CUDA_COMPILER "" CACHE STRING
-  "Location of a CUDA compiler capable of emitting LLVM bitcode.")
-set(LIBOMPTARGET_NVPTX_BC_LINKER "" CACHE STRING
-  "Location of a linker capable of linking LLVM bitcode objects.")
----------------
JonChesterfield wrote:
> Meinersbur wrote:
> > JonChesterfield wrote:
> > > Meinersbur wrote:
> > > > This removes support for `LIBOMPTARGET_NVPTX_CUDA_COMPILER`/`LIBOMPTARGET_NVPTX_BC_LINKER` settings? Instead, it uses clang found in `LLVM_TOOLS_BINARY_DIR`, which very much prefer over `${CMAKE_C_COMPILER}`. This this work with an LLVM_DIR to a pre-installed LLVM in `/usr` or `/usr/local`? 
> > > > 
> > > Changes the flag yeah. This is openmp code - no particular reason why NVPTX_CUDA_COMPILER is a good name for which one to use.
> > > 
> > > The cmake will have a go with whatever LLVM_DIR is set, whether that will succeed or not depends on whether that compiler is recent enough to deal with the openmp features used. Variant is probably the limiting factor.
> > > 
> > > We're trying to encourage people to build this via ENABLE_RUNTIMES as than ensures that the clang used can actually build it, but there are still these hooks if people want to try with a different one. 
> > Have you seen https://lists.llvm.org/pipermail/llvm-dev/2021-October/153238.html ?
> > 
> > I tried the "Default" build with openmp and it did not work:
> > ```
> > CMake Error at /home/meinersbur/src/llvm-project/openmp/runtime/src/CMakeLists.txt:314 (string):
> >   string sub-command REGEX, mode MATCH needs at least 5 arguments total to
> >   command.
> > ```
> I read that earlier today but haven't tried to work out the implications for openmp. 
> 
> > string(REGEX MATCH "[0-9]+\\.[0-9]+(\\.[0-9]+)?" CLANG_VERSION ${PACKAGE_VERSION})
> 
> I don't know offhand what that's trying to do. Seems unrelated to this patch? 
Just asking whether we have a plan to make this work if we want to support "Default builds" as well.  I think it does, since it uses the same `<root>/runtimes/CMakeLists.txt` that sets `LLVM_DIR` using:
```
find_package(LLVM PATHS "${LLVM_BINARY_DIR}" NO_DEFAULT_PATH NO_CMAKE_FIND_ROOT_PATH)
find_package(Clang PATHS "${LLVM_BINARY_DIR}" NO_DEFAULT_PATH NO_CMAKE_FIND_ROOT_PATH)
```
This will import LLVM/Clang's `*Targets.cmake`. Instead of find_program, one can get the executable path directly using [[ https://cmake.org/cmake/help/latest/prop_tgt/IMPORTED_LOCATION.html | IMPORTED_LOCATION ]].




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D111983