This is an archive of the discontinued LLVM Phabricator instance.

[libomptarget] Build a minimal deviceRTL for amdgcn
ClosedPublic

Authored by JonChesterfield on Nov 18 2019, 5:12 PM.

Details

Summary

[libomptarget] Build a minimal deviceRTL for amdgcn

The CMakeLists.txt file is functionally identical to the one used in the aomp fork.
Whitespace changes were made based on nvptx/CMakeLists.txt, plus the
copyright notice updated to match (Greg was the original author so would
like his sign off on that here).

This change will build a small subset of the deviceRTL if an appropriate toolchain is
available, e.g. a local install of rocm. Support.h is moved from nvptx as a dependency
of debug.h.

Diff Detail

Event Timeline

Herald added a project: Restricted Project. · View Herald TranscriptNov 18 2019, 5:12 PM
JonChesterfield marked 2 inline comments as done.Nov 18 2019, 5:19 PM
JonChesterfield added inline comments.
openmp/libomptarget/deviceRTLs/amdgcn/CMakeLists.txt
57

Glob has the advantage that this file will keep working as other source is added to amdgcn or common.

This will break as soon as common contains a file that amdgcn doesn't want to compile. At that point, this can become a whitelist instead.

I'm happy to list out the sources explicitly if that's strongly preferred.

74

Builds a specialised version of the library for various amdgcn architectures

Random drive-by comments to parts of the patch, I won't review in full.

openmp/libomptarget/deviceRTLs/amdgcn/CMakeLists.txt
57

As far as I know globs have the disadvantage that they need an explicit invocation of CMake to get an updated list of source files. That's why LLVM usually avoids them.

71–72

I don't think it's a good idea to honor random environment variables. I'd suggest to make this a proper configuration variable.

133

OPENMP_INSTALL_LIBDIR

  • address review comments
JonChesterfield marked 6 inline comments as done.EditedNov 19 2019, 10:33 AM

This is actually quite close to compiling under Release, at least for cancel.cu and critical.cu which don't include omptarget-nvptx.h. Debug.h unconditionally includes support.h (looks unintentional) and amdgcn's target_impl currently doesn't work without the cuda shim.

openmp/libomptarget/deviceRTLs/amdgcn/CMakeLists.txt
57

Fair enough. Explicit lists it is.

71–72

Agreed. Added a variable loosely based on the equivalent one for nvptx and checked I can still plumb it into the out of tree build scripts.

133

Nice, thanks.

JonChesterfield marked 3 inline comments as done.
  • Drop device_environment from this patch
  • Import support.h, update target_impl such that the deviceRTL builds successfully
JonChesterfield edited the summary of this revision. (Show Details)Nov 19 2019, 4:23 PM
JonChesterfield marked an inline comment as done.Nov 19 2019, 4:25 PM
JonChesterfield added inline comments.
openmp/libomptarget/deviceRTLs/amdgcn/src/device_environment.h
17

This file is correct for the not yet upstream hsa plugin. For the purposes of this change the num_devices and device_num fields could be dropped.

JonChesterfield retitled this revision from [libomptarget] Implement a CMakeLists.txt for amdgcn to [libomptarget] Build a minimal deviceRTL for amdgcn.Nov 19 2019, 4:26 PM
JonChesterfield edited the summary of this revision. (Show Details)

Thanks for pushing this. Two comments below, nothing major.

openmp/libomptarget/deviceRTLs/amdgcn/CMakeLists.txt
137

Nit: I'm not a cmake person but I am surprised to see "cuda" above so many times, e.g. in file names?

openmp/libomptarget/deviceRTLs/amdgcn/src/target_impl.h
114

Where did these implementations go?

JonChesterfield marked 2 inline comments as done.Nov 21 2019, 11:07 AM
JonChesterfield added inline comments.
openmp/libomptarget/deviceRTLs/amdgcn/CMakeLists.txt
137

The source is (currently still) cuda so it's not too bad that variables refer to cuda. I didn't rename anything from Greg's original script. Happy to rename the variables before or after commit.

I'd like to put the remaining cuda intrinsics behind an API and rename the files to .cpp, but the renaming caused problems with nvcc that I am not yet sure how to solve.

openmp/libomptarget/deviceRTLs/amdgcn/src/target_impl.h
114

The previous implementation called through to functions in a support library with fairly ugly prototypes.

I'm planning to implement these in target_impl.hip, which doesn't exist yet. Until then they're just another couple of missing symbols.

This revision is now accepted and ready to land.Nov 25 2019, 9:48 AM
This revision was automatically updated to reflect the committed changes.
ABataev added a comment.EditedDec 3 2019, 9:12 AM

Seems to me, it causes some problems at the compile-time:

/build_llvm/llvm-project/openmp/libomptarget/deviceRTLs/nvptx/src/omptarget-nvptx.h:27:21: fatal error: support.h: No such file or directory
 #include "support.h"
                     ^
compilation terminated.

Seems to me, it causes some problems at the compile-time:

Ah. Sorry about this, a clean build would show the error locally for me. The fix is trivial, will do it now (plus the 20mins or so for a clean build to sanity check)

Fair enough. The patch is literally s/"support.h"/"common/support.h" in two, optionally three files.

@ABataev I reproduced locally, fixed, posted D70971. Would you mind checking it builds for you as a redundant check? Thanks