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

[Michael Kruse via Phabricator via Openmp-commits]

Meinersbur added a comment.

In D101265#2724364 <https://reviews.llvm.org/D101265#2724364>, @tianshilei1992 wrote:

> That's unrelated to the problem we discuss here. The key point is, you propose to let `LLVM_ENABLE_PROJECTS` behave same as `LLVM_ENABLE_RUNTIMES`, and solely for OpenMP. If so, why do we even have this two arguments at the first place? Or let me put it in this way, is there any other project (such as `lld`) that when it is in `LLVM_ENABLE_PROJECTS` along with `clang`, it is actually built by the clang just built? If yes, I'll be totally fine with your change.

OK, there seems to be a misconception. With this patch LLVM_ENABLE_PROJECTS and LLVM_ENABLE_RUNTIMES will still be different. 
LLVM_ENABLE_PROJECTS will continue to compile all host code (libomp.so, libomptarget.so) with the compiler selected by CMake.
This only difference is that **by default** and if available, the LLVM_ENABLE_PROJECTS configuration also uses the just-built clang to cross-compile the deviceRTL to LLVM-IR. The host-compiler selected by CMake is for compiling to host-compatible assembly and unsuitable for cross-compilation for generation of LLVM bitcode.

> Or let me put it in this way, is there any other project (such as lld) that when it is in LLVM_ENABLE_PROJECTS along with clang, it is actually built by the clang just built? If yes, I'll be totally fine with your change.



1. Thread-sanitizer (compiler-rt) compiles libcxx with `$<TARGET_FILE:clang>` to get a thread-sanitized C++ standard library.
2. libtooling uses just-built clang for AST introspection
3. Polly uses just-built clang-tidy to check whether its correctly formatted.
4. libc uses just-built clang-tidy for some linting.



================
Comment at: llvm/CMakeLists.txt:971
 
+add_subdirectory(projects)
+
----------------
protze.joachim wrote:
> Are you sure that this doesn't break cmake dependencies for compiler-rt?
I am not (for compiler-rt or other uses such as those mentioned in the response); due to its global consequences, this change might indeed be risky. Logically, compiler-rt itself is also a runtime and hence should logically be ordered the same way. I would check this further before committing to verify.

In the summary I mentioned an alternative that would work without this change (checking LLVM_TOOL_CLANG_BUILD instead, like Polly).


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D101265