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

[Hal Finkel via Phabricator via Openmp-commits]

hfinkel added a comment.

In https://reviews.llvm.org/D46842#1104559, @Hahnfeld wrote:

> In https://reviews.llvm.org/D46842#1103727, @gtbercea wrote:
>
> > Your argument that an older version is better is actually wrong. Using an older version to build the BCLIB is always a BAD idea. The reason why you want to build the BCLIB is so that the runtime functions actually get inlined. If you use an older compiler the functions will not be inlined which defeats the purpose of having the BCLIB in the first place. I stumbled again upon this problem today after updating to the latest changes.
>
>
> To be precise, I didn't say that "an older version is better". My statement was that it works and that older versions of the compiler generate bitcode files that are compatible with a larger number of (newer) compilers.
>
> But you are right, inlining of old bitcode files broke last month with https://reviews.llvm.org/rC329829. Luckily, this can be fixed, see https://reviews.llvm.org/D47070.
>
> > I do not understand why you are opposing this, so far I haven't heard a single strong argument against this.
>
> "strong" is certainly subjective and may imply different things for you and me. If I wanted to put my point to the extreme, I'd say that there is none of your arguments left because I sat down and fixed all the problems you have been describing so far.
>
> Coming back to my opinion: I don't like using the just-built compiler because that's not how CMake usually works when you build a project.


You're correct, this is not how CMake normally works. However, CMake is also not normally being used to build a compiler. Compiler builds often self host to some degree or another, and the compiler provided to the build system is generally through of only as something necessary to bootstrap the build process. LLVM/Clang, being both a self-hosting compiler and also a library component, has a split personality in this regard. However, in this case, as I believe we do with compiler-rt components, etc., it makes sense to default to using the just-built clang to build the bitcode library (which is intended to be used by that just-built compiler). Honestly, I think we should carefully consider whether we want to officially support a non-version-locked configuration, because if we do, then we need to set parameters on that and test it on the buildbots. My inclination is to officially support on the version-locked configuration between the compiler building the application code, the compiler used to build the bitcode library, and the version of the bitcode library itself.

> The build system takes a single (version of a) compiler that can't change afterwards, determines its abilities through dynamic feature checking and compiles the source code. The last two step can be repeated as often as the project or its prerequisites change.
> 
> In https://reviews.llvm.org/D46842#1104142, @guansong wrote:
> 
>> I second this opinion. Besides, I think when the runtime is in-tree built, the compiler to compile the bc file HAS be the just-built clang compiler.
> 
> 
> I don't see why that has to be the case. As I explained throughout the comments, you can build the bitcode library with an older compiler and still inline it with a newer version.

I agree, I don't think it must be the case, technically it should work with other compilers. I think that using the just-built clang is a better default, however.

> 
> 
>> After that said, I am not sure if we consider in-tree runtime built for the moment. It sounds we do not?
> 
> You can build the bclib in-tree, it just won't use the just-built compiler but `CMAKE_C_COMPILER` by default.
> 
>> I may missed some earlier discussions. To me, I am not sure how to exactly set the dependence to make that happen. Our old code seems did that and then get changed?
> 
> I'll happily link to the previous discussion once more: The "support" for using the just-built compiler was removed during review of https://reviews.llvm.org/D14254. CC'ing Hal who chimed in last time, maybe he can add his opinion and convince one of the sides (I'm also fine with being convinced myself if I do still miss an important point!)

Thanks ;)


Repository:
  rOMP OpenMP

https://reviews.llvm.org/D46842