[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>
----------------
josemonsalve2 wrote:
> tianshilei1992 wrote:
> > Meinersbur wrote:
> > > 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.
> > > 
> > > How would `BUILD_VIA_RUNTIME` be detected? `target clang`/`target llvm-link` are not targets in the runtimes build CMake configurations.
> > You could refer to line 7 at `openmp/CMakeLists.txt`. That's what we used to determine if it is standalone build. If `OPENMP_STANDALONE_BUILD` is false, then we could set the two variables accordingly because in that case, it is either in runtime build, or project build, where in both cases you wanna use the in-tree build clang.
> > 
> > >Makes me wonder why this kind if pollution OK, but setting LIBOMPTARGET_NVPTX_CUDA_COMPILER is not.
> > That changes apply for all OpenMP arguments, `libomp`, `libomptarget`, plugins, device runtimes, tests, you name it. It's not just for the one single NVIDIA device runtime. What's more important, we obviously have a more elegant way to do it in project's own directory to avoid leaking those project specific arguments all over the place.
> I know this is been a while since this happened. But I was looking at this code and found this revision. 
> 
> I just wanted to add. Even though they have `compiler-rt` specific code, they recognize it is something that would need to be fixed. see `llvm-project/runtimes/CMakeLists.txt:110` or nearby. The comment says:
> 
> ```
> # TODO: compiler-rt has to use standalone build for now. We tried to remove
>   # this in D57992 but this broke the build because compiler-rt assumes that
>   # LLVM and Clang are configured in the same build to set up dependencies. We
>   # should clean up the compiler-rt build and remove this eventually.
> ```
> 
I still have the opinion that we should not assume that `CMAKE_CXX_COMPILER` (C++-to-`.o`) is also the compiler to cross-compile to `.bc` even more than why `CMAKE_C_COMPILER` is also different from `CMAKE_CXX_COMPILER`, `CMAKE_CUDA_COMPILER` or `CMAKE_CUDA_HOST_COMPILER` (even though they probably are all able to compile C-code to the host target), and you need a different `CMAKE_C_COMPILER` when cross-compiling.

What CMake variable specifies what compiler to use as "OpenMP/CUDA-to-bc" compiler is not libomptarget-specific. It could also be named `CMAKE_OPENMP_<TARGET>_CROSSCOMPILER`.

Alternatively, the deviceRTL could be built in another CMake build configuration that is configured to cross-compiler to NVPTX (in the same sense that `compiler-rt` is configured to cross-compile to the architecture(s) that just-built Clang targets), but this requires much more effort.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D101663