[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.")
----------------
Meinersbur wrote:
> 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 ]].
> 
> 
The [[ https://cmake.org/cmake/help/latest/command/add_executable.html#imported-executables | documentation  ]] also mentions that imported add_executable can be used in `add_custom_command`. Maybe
```
add_custom_command(
  clang <args> ... 
```
would just work as well? (this would also work for in-tree clang instead of `$<TARGET_FILE:clang>`).


================
Comment at: openmp/libomptarget/DeviceRTL/CMakeLists.txt:186
+  if("${OPT_TOOL}" STREQUAL "$<TARGET_FILE:opt>")
+    add_custom_command(OUTPUT ${CMAKE_CURRENT_BINARY_DIR}/${bclib_name}_opt
+      DEPENDS opt
----------------
JonChesterfield wrote:
> This looks dubious - the tailing _opt corresponds to the following custom command, but we're calling opt as opt file.bc -o file.bc. Probably want to link to file.link.bc or similar then opt to file.bc so we've got both on disk after the build. I vaguely recall problems reading & writing to the same file on windows. Leaving for a later change.
The `OUTPUT` must match the `OUTPUT` of the other `add_custom_command` on line 192 to add a `DEPENDS` dependency to the generation of that output dependency. Thus add_custom_command itself does not execute anything.

With the `APPEND` option I think this must come after the main `add_custom_command` line on line 192. Alternatively, you could add the `DEPENDS opt` to that line as well, but it would result in an error if the `opt` target does not exist. It actually seem to (either the in-tree target or the imported target), since otherwise `add_dependencies(${bclib_target_name} opt llvm-link)` would give an error es well.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D111983