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

[Michael Kruse via Phabricator via Openmp-commits]

Meinersbur 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>
----------------
tianshilei1992 wrote:
> 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`.
How would `BUILD_VIA_RUNTIME` be detected? `target clang`/`target llvm-link` are not targets in the runtimes build CMake configurations.

I think setting `LIBOMPTARGET_NVPTX_CUDA_COMPILER` is exactly the right option because this is the variable a user would need to set if they want a standalone build without using `LLVM_ENABLE_RUNTIMES`. Not additional magic needed.

> This is not true. See line 178 at llvm/runtimes/CMakeLists.txt.

Correct, did not see that. Makes me wonder why this kind if pollution OK, but setting `LIBOMPTARGET_NVPTX_CUDA_COMPILER` is not.



================
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.
----------------
tianshilei1992 wrote:
> 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.
The user sets `LIBOMPTARGET_NVPTX_CUDA_COMPILER` manually, it is their responsibility. If they do not specify `LIBOMPTARGET_NVPTX_CUDA_COMPILER`, but gets a broken build due to the default being inadequate we cannot blame the user.

The only adequacy test I can think of that cannot result in a broken build is to execute `clang --version` and compare it the commit hash.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D101663