[Openmp-commits] [PATCH] D87841: [openmp][libomptarget] Include header from LLVM source tree

Fri Sep 18 04:23:00 PDT 2020

grokos added a comment.

In D87841#2280009 <https://reviews.llvm.org/D87841#2280009>, @jdoerfert wrote:

> In D87841#2279743 <https://reviews.llvm.org/D87841#2279743>, @lebedev.ri wrote:
>
>>> As openmp and llvm are now developed in the same monorepo, the only uses this
>>> dependency can break are those that download the monorepo, then delete the llvm
>>> directory before calling cmake. That is not a compelling use case.
>>
>> That is factually not accurate.
>> There are standalone tarballs, including one for omp:
>> https://releases.llvm.org/download.html#10.0.1 `OpenMP Source code (.sig)`
>
> Right. You can download the source for the sub-projects.
> Though, others, e.g., clang, are already not compileable in isolation.
>
> I see three options here:
>
> 1. Continue to copy code from llvm-core to openmp and back. This is "good" for people that compile libomp in isolation and "bad" for developers. Arguably, it is also "bad" for people that use libomp with clang because there is an increased risk we have an unnoticed mismatch between the two. This is not theoretical but already happened.
> 2. Copy the code from llvm-core to openmp during packaging time. This is a (small?) burden on the package maintainers but has all the advantages for now. However, it will get more complicated the more llvm-core features we want to use, e.g., tablegen.
> 3. Require users to download llvm-core and point to it. This is a small burden on the users but has all the advantages now and in the future.
>
> This patch, on its own, picks 3). I'm happy with that.

4. At CMake time, check whether llvm-core is there; if yes use `OMPGridValues.h` from llvm-core, if no then download the header from some suitable location automatically. This eliminates the need for (2) and makes both proponents of (1) and proponents of (3) happy.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D87841