[Openmp-commits] [PATCH] D101265: [OpenMP][CMake] Use in-project clang as CUDA->IR compiler.

[Joachim Protze via Phabricator via Openmp-commits]

protze.joachim added inline comments.


================
Comment at: openmp/libomptarget/deviceRTLs/nvptx/CMakeLists.txt:35
+  # Compile the deviceRTL with the clang that is built in the project.
+  set(cuda_compiler clang)
 elseif(${CMAKE_C_COMPILER_ID} STREQUAL "Clang")
----------------
Meinersbur wrote:
> protze.joachim wrote:
> > I think, you want to have:
> > 
> > ```
> > set(cuda_compiler $<TARGET_FILE:clang>)
> > ```
> Having it named `clang` will make CMake assume it is the `clang` target, and add dependency to the target and replace `clang` with that path of the output executable.
> See https://cmake.org/cmake/help/latest/command/add_custom_target.html
> > If COMMAND specifies an executable target name (created by the add_executable() command), it will automatically be replaced by the location of the executable created at build time
> 
> With specifying a full path, CMake may not add the dependency to the `clang` target anymore (maybe it does, haven't checked) and would have to be defined manually.
> 
> However:
> > This target-level dependency does NOT add a file-level dependency that would cause the custom command to re-run whenever the executable is recompiled. List target names with the DEPENDS option to add such file-level dependencies.
> 
> Maybe we do want such a dependency to guarantee that the bitcode is always the latest
> (But then it seems to be illogical that we allow this with pre-compiled clangs via `LIBOMPTARGET_NVPTX_CUDA_COMPILER`).
Thanks for the clarification regarding the add_custom_command behavior

The only case I see where this really matters is, when you recompile in an existing build dir after switching branches.
In that case it makes sense to recompile the BC files, if the compiler is newer than the existing file. 
So, I'd add cuda_compiler as an explicit dependency in the custom target below.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D101265