This is an archive of the discontinued LLVM Phabricator instance.

[OpenMP] Introduce an CMake find module for OpenMP Target support
ClosedPublic

Authored by jhuber6 on Jun 22 2021, 6:41 AM.

Details

Summary

This introduces a CMake find module for detecting target offloading support in
a compiler. The goal is to make it easier to incorporate target offloading into
a cmake project.

Diff Detail

Event Timeline

jhuber6 created this revision.Jun 22 2021, 6:41 AM
jhuber6 requested review of this revision.Jun 22 2021, 6:41 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 22 2021, 6:41 AM
tianshilei1992 accepted this revision.Jun 22 2021, 9:37 AM

LGTM. Some nits.

openmp/tools/Modules/FindOpenMPTarget.cmake
2

Is it better to put these files into libomptarget?

88

Not sure if it is too strict. LLVM and OpenMP both require CMake 3.13.4 and above.

This revision is now accepted and ready to land.Jun 22 2021, 9:37 AM
jhuber6 added inline comments.Jun 22 2021, 9:54 AM
openmp/tools/Modules/FindOpenMPTarget.cmake
2

We don't have a tools directory under that, I figured it's fine in the same way we put the libomptarget documentation under openmp/docs.

88

I think the only things preventing me from using 3.13.4 is that the OpenMP module doesn't seem to return the version correctly, and it doesn't support the "NAME_MISMATCHED" which means that newer CMake will emit a warning for finding a package with a different name than the one specified.

openmp/tools/Modules/FindOpenMPTarget.cmake
88

The feature is good for sure, but since we require only 3.13+ to build OpenMP, I think it's better to keep it consistent; otherwise it might be confusing.

Is this intended for libomptarget only or even to take care other offload runtime libraries?
if it is the former case, It needs to be renamed as FindLibOMPTarget to be specific about libomptarget.
If it is the later case, please upstream it to CMake directly.

User code may want to defined either own OpenMPTarget target and alias it to the target created by this module if they use libomptarget or define others if other vendor OpenMP offload runtime library is used.

OpenMP is both a language extension and library, creating a cmake module is a challenge.

openmp/tools/Modules/FindOpenMPTarget.cmake
63

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

100

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?

jhuber6 updated this revision to Diff 353703.Jun 22 2021, 10:37 AM

Changing the minimum version requirements.

Is this intended for libomptarget only or even to take care other offload runtime libraries?
if it is the former case, It needs to be renamed as FindLibOMPTarget to be specific about libomptarget.
If it is the later case, please upstream it to CMake directly.

I intend for this to work for other compilers, but I don't have as much experience with the other compilers so I can't guarantee its effectiveness. Upstreaming to CMake could be a goal, but I think for now it's easier to keep it here until some more people have has a chance to test it.

User code may want to defined either own OpenMPTarget target and alias it to the target created by this module if they use libomptarget or define others if other vendor OpenMP offload runtime library is used.

You should be able to manually set the flags by setting the OpenMPTarget_<Device>_FLAGS or OpenMPTarget_<Device>_DEVICE since it will forward those if they're already set.

OpenMP is both a language extension and library, creating a cmake module is a challenge.

Yeah, I primarily copied the method used for the existing FindOpenMP.cmake module, fortunately I can reuse a lot of features.

openmp/tools/Modules/FindOpenMPTarget.cmake
63

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

100

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.

Is this intended for libomptarget only or even to take care other offload runtime libraries?
if it is the former case, It needs to be renamed as FindLibOMPTarget to be specific about libomptarget.
If it is the later case, please upstream it to CMake directly.

I intend for this to work for other compilers, but I don't have as much experience with the other compilers so I can't guarantee its effectiveness. Upstreaming to CMake could be a goal, but I think for now it's easier to keep it here until some more people have has a chance to test it.

other compiler with libomp+libomptarget or other compilers with their own OpenMP runtime libraries?

openmp/tools/Modules/FindOpenMPTarget.cmake
63

I mean it is better not to add another version variable if the OpenMP_VERSION is sufficient and has been included already.

100

3.20 changed PGI to NVHPC

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.

openmp/tools/Modules/FindOpenMPTarget.cmake
63

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.

100

Will probably need to provide both since this should work for older versions as well.

jhuber6 updated this revision to Diff 353764.Jun 22 2021, 1:21 PM

Addressing comments.

jhuber6 updated this revision to Diff 353837.Jun 22 2021, 6:26 PM

Fixing small error. I've checked gcc, clang, xlc, and nvc offloading to NVPTX
and only clang offloading to AMDGCN and it seems to work for those.