This is an archive of the discontinued LLVM Phabricator instance.

[libomptarget] Implement host plugin for amdgpu
ClosedPublic

Authored by JonChesterfield on Aug 11 2020, 8:50 AM.

Details

Summary

[libomptarget] Implement host plugin for amdgpu

Replacement for D71384. Primary difference is inlining the dependency on atmi
followed by extensive simplification and bugfixes. This is the latest version
from https://github.com/ROCm-Developer-Tools/amd-llvm-project/tree/aomp12 with
minor patches and a rename from hsa to amdgpu, on the basis that this can't be
used by other implementations of hsa without additional work.

This will not build unless the ROCM_DIR variable is passed so won't break other
builds. That variable is used to locate two amdgpu specific libraries that ship
as part of rocm:
libhsakmt at https://github.com/RadeonOpenCompute/ROCT-Thunk-Interface
libhsa-runtime64 at https://github.com/RadeonOpenCompute/ROCR-Runtime
These libraries build from source. The build scripts in those repos are for
shared libraries, but can be adapted to statically link both into this plugin.

There are caveats.

  • This works well enough to run various tests and benchmarks, and will be used to support the current clang bring up
  • It is adequately thread safe for the above but there will be races remaining
  • It is not stylistically correct for llvm, though has had clang-format run
  • It has suboptimal memory management and locking strategies
  • The debug printing / error handling is inconsistent

I would like to contribute this pretty much as-is and then improve it in-tree.
This would be advantagous because the aomp12 branch that was in use for fixing
this codebase has just been joined with the amd internal rocm dev process.

Diff Detail

Event Timeline

JonChesterfield requested review of this revision.Aug 11 2020, 8:50 AM
JonChesterfield edited reviewers, added: ronlieb; removed: ronl.Aug 11 2020, 8:52 AM
openmp/libomptarget/plugins/amdgpu/impl/msgpack.h
7

This is a from-scratch implementation of a msgpack parser, written by me in order to keep libomptarget from depending on the msgpack parse that already exists in llvm. Hopefully something that can go after the license changes finish up.

  • Insert plugin in chronological order
  • Fix typo in comment
openmp/libomptarget/src/rtl.cpp
32

Discussed in call that these should be in the order in which they were added to the codebase. Thus AMDGPU is at the end as the most recent addition.

Someone who knows this chronological order should reorder them separate to this patch.

Harbormaster completed remote builds in B68109: Diff 285080.
jdoerfert accepted this revision.EditedAug 15 2020, 2:32 PM

I have a really hard time understanding why all this code is necessary. Browsing it it seems to duplicate a lot of things that are in the OpenMP target runtime or I assumed would be in the underlying driver. Anyway, it is what it is.

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

There is no linker script anymore, IIRC.

openmp/libomptarget/src/rtl.cpp
32

Yes.

This revision is now accepted and ready to land.Aug 15 2020, 2:32 PM

There are some parts of this that I'm hoping to push down into the hsa/rocr layer. And other parts that can be deleted without replacement. I will make this better. Good to have clang dev unblocked in the meantime.

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

Indeed not. And Guansong is no longer with AMD, so may be difficult to check with anyway. Will drop this comment in the near future.

This revision was landed with ongoing or failed builds.Aug 15 2020, 3:58 PM
This revision was automatically updated to reflect the committed changes.
ronlieb added inline comments.Aug 16 2020, 10:58 AM
openmp/libomptarget/plugins/amdgpu/src/rtl.cpp
40–98

please revisit this inclusion of the file content from llvm/Frontend/OpenMP/OMPGridValues.h

seems like we have a maintenance issue going forward.

openmp/libomptarget/plugins/amdgpu/src/rtl.cpp
40–98

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.