[Openmp-commits] [PATCH] D85742: [libomptarget] Implement host plugin for amdgpu

[Jon Chesterfield via Phabricator via Openmp-commits]

JonChesterfield added inline comments.


================
Comment at: openmp/libomptarget/plugins/amdgpu/src/rtl.cpp:39-97
+// Get static gpu grid values from clang target-specific constants managed
+// in the header file llvm/Frontend/OpenMP/OMPGridValues.h
+// Copied verbatim to meet the requirement that libomptarget builds without
+// a copy of llvm checked out nearby
+namespace llvm {
+namespace omp {
+enum GVIDX {
----------------
ronlieb wrote:
> please revisit this inclusion of the file content from llvm/Frontend/OpenMP/OMPGridValues.h
> 
> seems like we have a maintenance issue going forward.
There is an ongoing discussion about sharing constants between the compiler and runtime. 'Copy and paste' is the only presently acceptable solution.

Multiple developers are certain that libomp must compile without a local copy of llvm available. That is, these files can't include any files from within LLVM.

Other developers are certain that LLVM must compile without a local copy of openmp available. That is, LLVM can't include any files from within openmp.

This results in a bunch of duplication between the two projects. We can bring the cost down by writing a test that checks the two projects agree on the values. That test can't live in either llvm or openmp, but provided we put it somewhere else and run it periodically we'll get prompt feedback on the files diverging.

Alternatives are periodically proposed - such as copying a file between the two during cmake, or having a third repo, or asking clang to print out the values to a header file that openmp includes - none of which have been accepted thus far.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D85742