[Openmp-commits] [PATCH] D101265: [OpenMP][CMake] Use in-project clang as CUDA->IR compiler.

Wed Apr 28 17:02:28 PDT 2021

tianshilei1992 added a comment.

In D101265#2718317 <https://reviews.llvm.org/D101265#2718317>, @Meinersbur wrote:

> In D101265#2715759 <https://reviews.llvm.org/D101265#2715759>, @tianshilei1992 wrote:
>
>> In D101265#2715749 <https://reviews.llvm.org/D101265#2715749>, @Meinersbur wrote:
>>
>>> In D101265#2715714 <https://reviews.llvm.org/D101265#2715714>, @tianshilei1992 wrote:
>>>
>>>> If OpenMP is built via `LLVM_ENABLE_PROJECTS`, it is *intentional* to build OpenMP w/o using the clang in recent build.
>>>
>>> Is this documented somewhere?
>>
>> You could refer to https://llvm.org/docs/CMake.html for `LLVM_ENABLE_PROJECTS` and https://llvm.org/docs/BuildingADistribution.html for `LLVM_ENABLE_RUNTIMES`.
>
> The first link does not mention what is intended to build with what compiler.
> The second link is for creating a distributable package, which does not apply here.

The two links show the difference between `LLVM_ENABLE_RUNTIMES` and `LLVM_ENABLE_PROJECTS`. It does mention that projects in `LLVM_ENABLE_RUNTIMES` are built by the recent built clang.

>>> Would you require the buildbot to do a stage1 build first for a LLVM_ENABLE_PROJECTS=openmp build?
>>
>> It depends. If you want offloading feature, then it requires. If you don't want that, it doesn't.
>
> It is not required, it works without a stage1 build and this patch.

If you need offloading feature, it is required because OpenMP offloading features requires clang to be the compiler. Yes, `libomptarget.so` will be generated, but no `deviceRTLs` will be generated so the offloading is still unusable.

>> For example, for `LLVM_ENABLE_PROJECTS=clang`, we don't expect to build clang with the "recent built" clang, right?
>
> We expect `.td` files to be built with recent built tablegen.

It's chicken egg problem. Let's say we have `LLVM_ENABLE_PROJECTS=clang;openmp`, and we are using GCC to build the whole LLVM. Let's call the clang generated here "the clang". If you expect OpenMP to be built by "the clang", what do you expect to build "the clang"? If GCC, why is OpenMP built by "the clang"? If "the clang", where is "the clang" at the first place? And even if we only have``LLVM_ENABLE_PROJECTS=openmp`, why is OpenMP in this case built by GCC?

>From my perspective, to keep the semantics consistent is important, and that's the only reason that I think all projects in `LLVM_ENABLE_PROJECTS` should be built by the same compiler as the one to build LLVM.

> There are valid use cases for using a LLVM_ENABLE_PROJECT configuration. One is that it is a single build dir for using in an IDE or generating a single CMAKE_EXPORT_COMPILE_COMMANDS for use with tools such as clangd. Ninja can also better track dependencies. libomp(target?) is also used by non-Clang compilers such as icc and msvc.

I don't doubt that. To have `LLVM_ENABLE_PROJECTS` for IDE is totally fine, and I think even now w/o your patch, `libomp` and `libomptarget` can work with `clangd` except those plugins and `deviceRTLs`. Plugins and `deviceRTLs` will still be missing in the compilation database even with your patch if I understand correctly.

>> I suppose you are building LLVM with GCC. If you're building LLVM with LLVM, offloading support will also be enabled. The idea here is, the OpenMP is built using the same compiler to build LLVM. Basically the OpenMP project will be built in a same way as others.
>
> The OpenMP host code, but CMake does not configure a device RTL compiler. I would not expect it to use the host compiler for cross-compiling to the GPU. That's what a cross-compiler is for.

Sure, you can, but by default it will use what CMake detects. If you don't want it, just specify the one.

> Another difference is while for assembly we have a well-defined ABI that ensures that outputs from different compilers are compatible, mixing BC files from different versions of LLVM might not be such a good idea. Sure, there is the AutoUpgrader, but it's probably one of the least tested components in LLVM. Then there are also future versions or third-party forks of clang who's BC files might not at all compatible with our clang, e.g. Apple's clang.

That's unrelated to the problem we discuss here. The key point is, you propose to let `LLVM_ENABLE_PROJECTS` behave same as `LLVM_ENABLE_RUNTIMES`, and solely for OpenMP. If so, why do we even have this two arguments at the first place? Or let me put it in this way, is there any other project (such as `lld`) that when it is in `LLVM_ENABLE_PROJECTS` along with `clang`, it is actually built by the clang just built? If yes, I'll be totally fine with your change.

And BTW, using `LLVM_ENABLE_RUNTIMES` can already avoid all problems you mentioned here.

> LLVM_ENABLE_RUNTIMES also seems badly documented. When it was first introduced, it did not work for a couple of years and I only found out relatively recently that it is supposed to work. And I am not the only one <https://lists.llvm.org/pipermail/llvm-dev/2021-March/148980.html>. A newcomer will first try `LLVM_ENABLE_PROJECT=clang;openmp` and be frustrated to see that it will not work with a cryptic error message
>
>   No library 'libomptarget-nvptx-sm_61.bc' found in the default clang lib directory or in LIBRARY_PATH. Please use --libomptarget-nvptx-bc-path to specify nvptx bitcode library.
>
> These problems can all be avoided by using the "recent built" clang as a sensible default. You can still use another by explicitly setting LIBOMPTARGET_NVPTX_CUDA_COMPILER.

I agree with you that `LLVM_ENABLE_RUNTIMES` was broken before, but now it works, and it is in OpenMP Q&A (https://openmp.llvm.org/docs/SupportAndFAQ.html).

FWIW, the only problem for `LLVM_ENABLE_RUNTIMES` now is, if we run `check-all`, and if any test case in `libomptarget` fails, other checks will not be run. Since currently offloading `x86-64` is broken, test cases for `libomptarget` never all pass, but that's another story and we need to fix the lit for `libomptarget`.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D101265