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

[Jon Chesterfield via Phabricator via Openmp-commits]

JonChesterfield added inline comments.


================
Comment at: openmp/libomptarget/DeviceRTL/CMakeLists.txt:12
 ##===----------------------------------------------------------------------===##
 
+set(LIBOMPTARGET_BUILD_DEVICERTL_BCLIB TRUE CACHE BOOL
----------------
Built it by default - only depends on clang (doesn't even use freestanding libc headers at present). This was a feature request on amdgpu/deviceRTL and it's cheap to keep that alive here. Someone building libomptarget with a clang which is too old to compile this library may want to opt out.


================
Comment at: openmp/libomptarget/DeviceRTL/CMakeLists.txt:93
 endforeach()
 
 # Activate RTL message dumps if requested by the user.
----------------
MAX_SM is cruft from the old deviceRTL, not used in the new one


================
Comment at: openmp/libomptarget/DeviceRTL/CMakeLists.txt:97
+  "Activate DeviceRTL debug messages.")
 
 
----------------
these messages are redundant with the ones emitted above


================
Comment at: openmp/libomptarget/DeviceRTL/CMakeLists.txt:178
 
+  # Add a file-level dependency to ensure that llvm-link and opt are up-to-date.
+  # By default, add_custom_command only builds the tool if the executable is missing
----------------
I think it reads better to sort out the dependency for both tools before using them


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


================
Comment at: openmp/libomptarget/DeviceRTL/CMakeLists.txt:18
 
-
-# Check if we can create an LLVM bitcode implementation of the runtime library
-# that could be inlined in the user application. For that we need to find
-# a Clang compiler capable of compiling our CUDA files to LLVM bitcode and
-# an LLVM linker.
-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.")
-
-if (NOT LIBOMPTARGET_NVPTX_CUDA_COMPILER STREQUAL "")
-  set(cuda_compiler ${LIBOMPTARGET_NVPTX_CUDA_COMPILER})
-elseif (LLVM_TOOL_CLANG_BUILD AND NOT CMAKE_CROSSCOMPILING)
-  # Compile the deviceRTL with the clang that is built in the project.
-  set(cuda_compiler "$<TARGET_FILE:clang>")
-elseif(${CMAKE_C_COMPILER_ID} STREQUAL "Clang")
-  set(cuda_compiler ${CMAKE_C_COMPILER})
+if (LLVM_DIR)
+  # Builds that use pre-installed LLVM have LLVM_DIR set.
----------------
This is copy&paste from the amdgpu cmakelists with the debug print slightly changed.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D111983