Page MenuHomePhabricator

AMDGPU: Teach toolchain to link rocm device libs
ClosedPublic

Authored by arsenm on Mar 13 2019, 1:22 PM.

Details

Summary

Currently the library is separately linked, but this isn't correct to
implement fast math flags correctly. Each module should get the
version of the library appropriate for its combination of fast math
and related flags, with the attributes propagated into its functions
and internalized.

HIP already maintains the list of libraries, but this is not used for
OpenCL. Unfortunately, HIP uses a separate --hip-device-lib argument,
despite both languages using the same bitcode library. Eventually
these two searches need to be merged.

An additional problem is there are 3 different locations the libraries
are installed, depending on which build is used. This also needs to be
consolidated (or at least the search logic needs to deal with this
unnecessary complexity).

Diff Detail

Event Timeline

arsenm created this revision.Mar 13 2019, 1:22 PM
yaxunl added inline comments.Apr 3 2019, 11:34 AM
lib/Driver/ToolChains/AMDGPU.h
25 ↗(On Diff #190487)

I don't think we should detect ROCm installation here. We are compiling code for amdgpu not only on ROCm, but also on amdgpu-pro and windows. Many cases, people want to compile code for amdgpu on systems without ROCm installed.

Compiilng code for amdgpu does not really depend on ROCm. We only depend on device library.

It makes more sense to have a AMDGPUDeviceLibDetector which works on ROCm, amdgpu-pro, and windows. Also we need an option -amdgpu-device-lib-path to override the default amdgpu device lib path.

arsenm marked an inline comment as done.Apr 3 2019, 1:28 PM
arsenm added inline comments.
lib/Driver/ToolChains/AMDGPU.h
25 ↗(On Diff #190487)

Being able to cross compile without a rocm installation is one of my primary goals here. I don't know what the install paths look like for those other drivers. It would be best if everything could agree on a single default install location for these to search, which is mostly a naming problem. In the patch as-is, the rocm-path is serving as -amdgpu-device-lib-path, but this is a naming question

arsenm marked an inline comment as done.Nov 19 2019, 11:00 PM
arsenm added inline comments.
lib/Driver/ToolChains/AMDGPU.h
25 ↗(On Diff #190487)

Except we don't have an omni-purpose bitcode library set. It's the ROCm-Device-Libs. rocm is the only semi-coherently defined platform at the moment

Add Scott since this may affect comgr. Probably need to add -nodefaultlibs in comgr after this change.

yaxunl added inline comments.Nov 20 2019, 1:51 PM
clang/lib/Driver/ToolChains/AMDGPU.cpp
93

debugging code needs to be removed

arsenm updated this revision to Diff 252692.Mar 25 2020, 3:25 PM

Use -nogpulib instead of -nodefaultlibs

arsenm updated this revision to Diff 252708.Mar 25 2020, 4:53 PM

Cleanup a bit. This should eventually merge with the HIP library handling when it switches to using clang cc1 instead of llc

arsenm updated this revision to Diff 252715.Mar 25 2020, 5:48 PM

Handle wave64 library

arsenm updated this revision to Diff 252978.Mar 26 2020, 2:18 PM
arsenm retitled this revision from WIP: AMDGPU: Teach toolchain to link rocm device libs to AMDGPU: Teach toolchain to link rocm device libs.
arsenm edited the summary of this revision. (Show Details)
arsenm updated this revision to Diff 253156.Mar 27 2020, 9:54 AM

Cleanup wave64 check

arsenm updated this revision to Diff 253194.Mar 27 2020, 12:34 PM

Fix negating backwards logic for default FTZ mode

yaxunl added inline comments.Mar 31 2020, 8:05 PM
clang/include/clang/Basic/DiagnosticDriverKinds.td
264

could you please rebase your patch?

The patch seems to contain irrelevant diffs.

arsenm updated this revision to Diff 254229.Apr 1 2020, 9:39 AM
arsenm marked an inline comment as done.

Rebase again

clang/include/clang/Basic/DiagnosticDriverKinds.td
264

You're probably looking at the wrong history diff

hliao added a comment.Apr 1 2020, 11:41 AM

Do we have a better way to avoid adding those empty bitcode files?

clang/lib/Driver/ToolChains/AMDGPU.h
29

may need to add llvm namespace prefix just like all other LLVM stuffs used in clang in case the same name is introduced later by accident.

44–46

sounds to me that both Version and BinPath should be added. They will be used eventually.

clang/lib/Driver/ToolChains/HIP.h
76

Do you miss the change in HIP.cpp? That constructor needs revising as the base class is changed.

arsenm marked 2 inline comments as done.Apr 1 2020, 1:50 PM

Do we have a better way to avoid adding those empty bitcode files?

No, we need the files to exist for tests. This is what existing bitcode link tests do

clang/lib/Driver/ToolChains/AMDGPU.h
44–46

It's easy to add when needded

clang/lib/Driver/ToolChains/HIP.h
76

Yes, I fixed this locally already

yaxunl added inline comments.Apr 1 2020, 2:42 PM
clang/include/clang/Driver/Options.td
611

HIP toolchain will also use it to add include path for HIP header files, therefore this is more like --cuda-path=. i_Group is more proper.

hliao added a comment.Apr 2 2020, 7:27 AM

Do we have a better way to avoid adding those empty bitcode files?

No, we need the files to exist for tests. This is what existing bitcode link tests do

could you update this patch?

arsenm updated this revision to Diff 254591.Apr 2 2020, 12:16 PM

Correct group

yaxunl accepted this revision.Apr 8 2020, 12:03 PM

LGTM. Thanks!

This revision is now accepted and ready to land.Apr 8 2020, 12:03 PM