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

[Jose Manuel Monsalve Diaz via Phabricator via Openmp-commits]

josemonsalve2 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:
> > 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.
```



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D101663