[Openmp-commits] [PATCH] D46842: [OpenMP][libomptarget] Make bitcode library building depend on clang and llvm-linker being available

[Jonas Hahnfeld via Phabricator via Openmp-commits]

Hahnfeld added a comment.

In https://reviews.llvm.org/D46842#1099392, @guansong wrote:

> Can we double check this LLVM IR backward compatible thing? My understanding is that it is not that good, at least for the major versions.




In https://reviews.llvm.org/D46842#1099415, @guansong wrote:

> I just want to comment this last point, my understanding is that it will not work even you try to inline older LLVM IR into a newer one.


I did just that: I built libomptarget-nvptx-bc with Clang 3.9.1 (which is actually the only released version that you can currently build with) and inlined the result with LLVM/Clang trunk which will become 7.0. It worked without problems for all simple test cases I tried. Do you have a scenario that fails for you?

In https://reviews.llvm.org/D46842#1099393, @gtbercea wrote:

> I don't think that works, when trying to inline newer LLVM code into older LLVM code I get an error saying that a different LLVM version is used or something along those lines.


Well, that's ... obvious, isn't it? How is an older version of LLVM supposed to know what to do with a newer format and "downgrade" that? The other direction is generally no problem: Newer LLVM versions usually auto-upgrade older bitcode files, see above.

Actually, I think that's an argument for my point of view: Building with an older compiler will result in libraries that are compatible with a larger number (more recent versions) of compilers. And by definition, `CMAKE_C_COMPILER` is older than the just-built compiler.

In https://reviews.llvm.org/D46842#1099393, @gtbercea wrote:

> > 1. The just-built compiler doesn't exist by definition when CMake is invoked. This will prevent all kind of dynamic flag checking (Does `${LIBOMPTARGET_NVPTX_SELECTED_CUDA_COMPILER}` support `-fflag`?). This is how I propose to reliably implement a solution for https://reviews.llvm.org/D44992.
>
> The compiler not existing can also happen if the user provides a path to a non-existing compiler. Remember, this is a default value which can always be overwritten by the user.
>  Flag checking will work in the same way as if the user had provided the path to a different compiler.


If the user supplies junk, we can just error out. That's what CMake will also do for invalid `CMAKE_C_COMPILER`s and the like.

>> 2. See the quote in my previous comment: This patch will result in rebuilding the entire library whenever there is a change in LLVM or Clang. While this doesn't seem to problematic at first, `clang` is usually the binary that finishes linking last because it's rather large. So the compilation of `libomptarget-nvptx-bc` can only start after almost all other steps of the build are done. I've found this really annoying in the past.
> 
> That's right, when Clang is available that's when the bclib is built. There is no problem if the bclib is built at the end of the build process actually. The BCLIB builds instantly so there's no perceptible overhead. Actually it's what the developer wants, if they make changes that affect the CUDA compilation then they want to see them reflected in the BCLIB.

And it's the least thing developers expect if they are hacking on an entirely different region of the compiler...

> An overall comment which I think applies to all your argument above: the choice of using the just-built compiler is a reasonable default which in an overwhelming number of cases is "what the user wants". This choice can always be overwritten using cmake flags. Those cmake flags are checked first and if a valid clang compiler is found then the building of the bclib will proceed there and then without depending on the just-built compiler. So for all your arguments above,  user can always just provide a different clang and it will work as you want.

My statement would be that the user should just build the `openmp` repo with the "new" compiler, even though I don't see an advantage (see my points above).


Repository:
  rOMP OpenMP

https://reviews.llvm.org/D46842