[Openmp-commits] [PATCH] D128130: [libomptarget] Make libomptarget.devicertl.a built in all cases.

Sat Jun 18 20:43:24 PDT 2022

ye-luo added a comment.

In D128130#3594408 <https://reviews.llvm.org/D128130#3594408>, @jhuber6 wrote:

> So this changes the compilation to just use the Clang binary found in the install directory and hope it works like we did previously.

We don't do "hope it works" but we are sure it works and I don't think we found undesired clang. If undesired clang gets picked up. Please report issues.
Actually in the case of ENABLE_PROJECTS=openmp, there is no clang binary searching but directly referencing the clang cmake target. The just-built clang is guaranteed to be picked up.
In the case of ENABLE_RUNTIMES=openmp, LLVM_DIR is passed in, I would consider it closer to "hope it works". Even relying on CMAKE_CXX_COMPILER in runtime builds, you actually hope the RUNTIMES setup passes in the desired compiler, it is just a different scripting magic managed by ENABLE_RUNTIMES.

> It's not ideaI was planning on deleting all the code above and deprecating the old bitcode library.

I've no problem depercating old bitcode library only if the devicertl static library can be built when OpenMP project can be built standalone or as ENABLE_PROJECTS with GCC as the host compiler for the host library part. `RUNTIMES` has the downside of getting contaminated by clang development. It doesn't cover my needs. The intention of this PR is to make my requirement to work. It is not "ideal" but on the other hand "ideal" solution only exits when it has actually been made. To make the deprecation and transition fast. We should consider intermediate solutions before an "ideal" solution is found.

> As I understand the reason we'd want to do this is to support compiling the OpenMP runtime with a different compiler than the `clang` that's necessary to build the device runtime. I wonder if instead it's possible to make the `RUNTIMES` portion of the build only apply to the device runtime, since realistically that's all we need it for.

Correct, building libomptarget host part with GCC and only the DeviceRTL with clang. I'm requesting this compilation mode to work. 
A potentially better solution you may try is to spin off DeviceRTL as a separate RUNTIME project. The ENABLE_PROJECTS=openmp and ENABLE_RUNTIMES=openmp-devicertl. However, I'm not sure how soon this can be made working and how drastic changes are acceptable.

> Also with this method, we no longer inherit the `-DCMAKE_BUILD_TYPE=Release` and instead use the old `clang-opt-flags` which only did `-O1`. This removes a lot of the benefits users will see using this library for LTO over the traditional bitcode library. We don't do O3 <https://reviews.llvm.org/owners/package/3/> on the bitcode library because last I checked it broke everything.

I just made an initial patch and chose to use existing `clang-opt-flags` and thus inherit '-O1',  no problem just make a another `clang-opt-flags-for-static` with '-O3'. There are anyway many flags being hard-coded, I don't see the issue of hard-coding more flags. For my understanding, RUNTIMES itself is not necessary a perfect feature. It is a compromise. RUNTIME doesn't allow precise control of libraries neither. RUNTIME="libcxx;openmp" I don't see easy way of configuring two runtime libraries compiled with different compile type. RUNTIME only brings in some benefit but it also carries limitations.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128130