Page MenuHomePhabricator

[openmp][libomptarget] Include header from LLVM source tree
ClosedPublic

Authored by JonChesterfield on Sep 17 2020, 10:16 AM.

Details

Summary

[openmp][libomptarget] Include header from LLVM source tree

The change is to the amdgpu plugin so is unlikely to break anything.

The point of contention is whether libomptarget can depend on LLVM.
A community discussion was cautiously not opposed yesterday.

This introduces a compile time dependency on the LLVM source tree, in this case
expressed as skipping the building of the plugin if LLVM_MAIN_INCLUDE_DIR is not
set. One the source files will #include llvm/Frontend/OpenMP/OMPGridValues.h,
instead of copy&pasting the numbers across.

For users that download the monorepo, the llvm tree is already on disk. This will
inconvenience users who download only the openmp source as a tar, as they would
now also have to download (at least a file or two) from the llvm source, if they want
to build the parts of the openmp project that (post this patch) depend on llvm.

There was interest expressed in going further - using llvm tools as part of
building libomp, or linking against llvm libraries. That seems less clear cut
an improvement and worthy of further discussion. This patch seeks only to change
policy to support openmp depending on the llvm source tree. Including in the
other direction, or using libraries / tools etc, are purposefully out of scope.

Reviewers are a best guess at interested parties, please feel free to add others

Diff Detail

Event Timeline

Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptSep 17 2020, 10:16 AM
JonChesterfield requested review of this revision.Sep 17 2020, 10:16 AM
JonChesterfield added inline comments.
llvm/include/llvm/Frontend/OpenMP/OMPGridValues.h
3 ↗(On Diff #292552)

Somewhat unfortunately for this change, this header was added with the old license. Correcting here. Introduced by @saiislam and written by @gregrodgers from my team at AMD.

openmp/libomptarget/plugins/amdgpu/src/rtl.cpp
41

Lines on the left are byte for byte identical to the middle of this header, as that's where they were copied from. At some point in the future they're likely to accidentally diverge and I don't want to find that as the root cause while debugging.

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)

Hahnfeld resigned from this revision.Sep 17 2020, 10:45 AM

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)

Thanks! I was not aware of that, will correct the description.

JonChesterfield edited the summary of this revision. (Show Details)Sep 17 2020, 11:01 AM
saiislam added a comment.EditedSep 17 2020, 11:08 AM

AFAIK, all files which are required/should be available in libomptarget reside in llvm/Frontend/OpenMP/* , so may be an option can be provided for only-tarball users to get files only from this directory.
This kind of approach will also enforce people exporting stuff from llvm to libomptarget, to put everything in one place.

saiislam added inline comments.Sep 17 2020, 11:28 AM
llvm/include/llvm/Frontend/OpenMP/OMPGridValues.h
33 ↗(On Diff #292552)

You might want to update these comments also. I have a NFC patch under review for exactly this D86232. Feel free to bring it in here.

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.

llvm/include/llvm/Frontend/OpenMP/OMPGridValues.h
33 ↗(On Diff #292552)

Thanks for the cross reference. I want to keep this review focused on accepting the new dependency edge.

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.

  1. 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.

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.

  1. 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.

*shrudder* Not that please.

  1. 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.

Most of our bigger machine have no internet. Download after the fact might also confuse people. I think telling the user to download and point to instead, as we do with other dependences, is in the grant scheme of things less surprising.

  1. 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.

Most of our bigger machine have no internet. Download after the fact might also confuse people. I think telling the user to download and point to instead, as we do with other dependences, is in the grant scheme of things less surprising.

I think it might be better to provide both options: if users don't specify anything, using online download by default. If users specify the path to the headers, it then does not download.

I strongly dislike build scripts pulling files off the internet. It's bad for machines that aren't connected to the web and upsets people in security

I have no idea if this has been considered before or not, or if there is some technical/organizational difficulties against this option, but I think an option would be to create somewhere in the LLVM umbrella project for common Headers/Def.s between different LLVM subprojects (directory called "common" maybe?), which should help maintain independence between the projects without duplication, package maintainers will know to always include that in any package without having to worry about looking for shared files on a file by file bases, and users should always download that with whichever llvm package they are going to use.

FWIW.

fghanim removed a subscriber: fghanim.Sep 18 2020, 10:34 AM

I have no idea if this has been considered before or not, or if there is some technical/organizational difficulties against this option, but I think an option would be to create somewhere in the LLVM umbrella project for common Headers/Def.s between different LLVM subprojects (directory called "common" maybe?), which should help maintain independence between the projects without duplication, package maintainers will know to always include that in any package without having to worry about looking for shared files on a file by file bases, and users should always download that with whichever llvm package they are going to use.

FWIW.

+1 for this solution.

JonChesterfield added a comment.EditedSep 18 2020, 11:31 AM

An additional repo which llvm, openmp, clang etc implicitly depend on would also work. It's roughly equivalent to treating llvm itself as that root dependency.

I suppose I view this set of repositories (now merged as the monorepo) as llvm + stuff, so it seems strange to require components in stuff to work wholly independently of llvm.

edit: A strong advantage to including from within llvm, instead of setting up a new top level directory to put shared things in, is that we can fix this maintenance hazard in openmp without waiting to reach consensus from the wider community on what to name, and what to put in, said new directory.

Ping. Can we have the include from llvm now, and postpone choosing a new top level directory until we have a longer list of things we'd like in it?

Can we move the cmake stuff in the main openmptarget logic so we can use llvm in all of openmp target.

JonChesterfield added a comment.EditedWed, Oct 14, 12:44 PM

Can we move the cmake stuff in the main openmptarget logic so we can use llvm in all of openmp target.

Sounds great. Proposed as D89426, with the intent of landing that one first.

jdoerfert accepted this revision.Wed, Oct 14, 3:30 PM

LGTM, one comment.

llvm/include/llvm/Frontend/OpenMP/OMPGridValues.h
6 ↗(On Diff #298147)

Commit this separately as NFC

This revision is now accepted and ready to land.Wed, Oct 14, 3:30 PM
saiislam added inline comments.Thu, Oct 15, 3:36 AM
llvm/include/llvm/Frontend/OpenMP/OMPGridValues.h
6 ↗(On Diff #298147)

I will take care of it in my related NFC patch D86232.

JonChesterfield marked an inline comment as done.Thu, Oct 15, 7:45 AM
JonChesterfield added inline comments.
llvm/include/llvm/Frontend/OpenMP/OMPGridValues.h
6 ↗(On Diff #298147)

Done

This revision was landed with ongoing or failed builds.Thu, Oct 15, 7:46 AM
This revision was automatically updated to reflect the committed changes.