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

[Shilei Tian via Phabricator via Openmp-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:
> 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.


================
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:
> > 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.
> 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.
That is not sufficient. What if users build LLVM w/o expected targets? You can still get a "valid" clang (in terms of version) and use it for NVPTX, but in fact it doesn't support it at all. We did encounter this once when AMD offloading was enabled by default previously.

We need to check the eligibility before including corresponding directories in CMake. That's the ultimate solution. We know our device runtime is using X, Y, Z features. We check whether the compiler can work properly. That's how `autoconfig` works. If in the future new features are being used, we simply update the checker.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D101663