[Openmp-commits] [PATCH] D104710: [OpenMP] Introduce an CMake find module for OpenMP Target support

[Joseph Huber via Phabricator via Openmp-commits]

jhuber6 added a comment.

Other compilers, Clang is the only one with a separate library AFAIK. The others just put them inside of the existing ones like libgomp so I don't search for them.



================
Comment at: openmp/tools/Modules/FindOpenMPTarget.cmake:62
+
+The module will also try to provide the OpenMP version variables:
+
----------------
ye-luo wrote:
> jhuber6 wrote:
> > ye-luo wrote:
> > > I don't quite follow what is `OpenMP version` especially the device specific one.
> > > As vendors usually only implement a subset, users should not rely on OpenMP spec version anyway.
> > > If they really need it, just invoke FindOpenMP themselves. RIght now it is an illusion there is a separate OpenMPTarget_VERSION
> > Since `FindOpenMP` returns a version, I think it's reasonable to return the same version for this module because it's intended to be a superset. It should include all the varialbes `FindOpenMP` offers, plus the ones needed for offloading. (I may need to check if I set the include dirs and libs correctly in addition).
> I mean it is better not to add another version variable if the OpenMP_VERSION is sufficient and has been included already.
I could get rid of it, or just unset them manually. I mostly just wanted the versions to make the find_package_handle_standard_args work more in line with OpenMP.


================
Comment at: openmp/tools/Modules/FindOpenMPTarget.cmake:99
+    set(OMPTarget_FLAG_XL "-qoffload")
+    set(OMPTarget_FLAG_PGI "-mp=${DEVICE}")
+
----------------
ye-luo wrote:
> jhuber6 wrote:
> > ye-luo wrote:
> > > FLAGS, plural because you might need multiple flags.
> > > 
> > > Use NVHPC instead of PGI, the correct flag is -mp=gpu
> > > does -mp=${DEVICE} work
> > > 
> > > GNU offload to NVIDIA is -foffload=nvptx-none
> > > does -foffload=${DEVICE} work?
> > > 
> > PGI what the `COMPILDER_ID` returns when I ran with the NVHPC compiler last I checked. I think it only supports nvptx so there's no option to configure its target.
> > 
> > GNU allows this flag, technically linking math and atomic on the device are optional, but they are needed in most applications so it's much easier to do it by default.
> > 
> > I'll make it plural.
> 3.20 changed PGI to NVHPC
Will probably need to provide both since this should work for older versions as well.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104710