[PATCH] D101663: [OpenMP] Avoid unintentional use of host compiler as bclib compiler.

[Shilei Tian via Phabricator via llvm-commits]

tianshilei1992 added inline comments.


================
Comment at: llvm/runtimes/CMakeLists.txt:238
                                       -DCMAKE_ASM_COMPILER_WORKS=ON
+                                      -DLIBOMPTARGET_NVPTX_CUDA_COMPILER=$<TARGET_FILE:clang>
+                                      -DLIBOMPTARGET_NVPTX_BC_LINKER=$<TARGET_FILE:llvm-link>
----------------
Meinersbur wrote:
> Meinersbur wrote:
> > tianshilei1992 wrote:
> > > I don't think it's a good idea to "pollute" LLVM CMake files for this purpose. There are plenty of ways to tell whether OpenMP is built via `LLVM_ENABLE_RUNTIMES`. I'd set the two CMake variables in OpenMP by checking whether we're in `LLVM_ENABLE_RUNTIMES`.
> > Adding project-specific options is already done for `COMPILER_RT`. This seems to be the established approach.
> Btw, PASSTHROUGH_PREFIXES, will pass all `OPENMP_` options (even those that are left to their defaults) to the nested CMake configuration. It will not do so with `LIBOMPTARGET_NVPTX_CUDA_COMPILER` because the prefix for the openmp project is assumed to be `OPENMP_` and `LIBOMPTARGET_` is missing.
I mean, you could do something in CUDA's CMake file in the following way (pseudo code):
```
if (BUILD_VIA_RUNTIME)
set(LIBOMPTARGET_NVPTX_CUDA_COMPILER target clang)
set(BC_LINKER target llvm-link)
endif()
```
where `BUILD_VIA_RUNTIME` can be detected. You don't have to do that in LLVM's CMake file.

> Btw, PASSTHROUGH_PREFIXES, will pass all OPENMP_ options (even those that are left to their defaults) to the nested CMake configuration. It will not do so with LIBOMPTARGET_NVPTX_CUDA_COMPILER because the prefix for the openmp project is assumed to be OPENMP_ and LIBOMPTARGET_ is missing.
This is not true. See line 178 at `llvm/runtimes/CMakeLists.txt`.


================
Comment at: openmp/libomptarget/deviceRTLs/nvptx/CMakeLists.txt:36
   set(cuda_compiler "$<TARGET_FILE:clang>")
-elseif(${CMAKE_C_COMPILER_ID} STREQUAL "Clang")
-  # Compile the device runtime with the compiler that OpenMP is built with.
----------------
Meinersbur wrote:
> tianshilei1992 wrote:
> > Removing this can cause issue if I compile OpenMP standalone. We cannot assume people all compile OpenMP along with LLVM either with `LLVM_ENABLE_RUNTIMES` or `LLVM_ENABLE_PROJECTS`.
> > Like I said in your previous patch, we need a mechanism to check whether the provided `clang` is qualified.
> As mentioned in the summary, automatically using the host compiler may result in unpredictable LLVM-IR that e.g. include vendor extensions. That is, the C/C++ to host-assembly compiler is just the wrong tool for CUDA-to-LLVM-IR compilation.
> 
> I think the only clang we would want to support is the clang from that same git commit.
That doesn't make sense. It's fine that if building in-tree, use the one from same commit, but it should never be the only way. Again (I have said that for three times), we need to check if the clang is qualified. Users can of course set `LIBOMPTARGET_NVPTX_CUDA_COMPILER` to a random clang. This change doesn't solve the root problem: is the compiler qualified? What we really need is, no matter where the compiler is from (it can be the host compiler detected by CMake, it can also be the one specified by users), check its qualification before use it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D101663