This is an archive of the discontinued LLVM Phabricator instance.

[OpenMP] Add support for linking AMDGPU images
ClosedPublic

Authored by jhuber6 on Jan 13 2022, 1:15 PM.

Details

Summary

This patch adds support for linking AMDGPU images using the LLD binary.
AMDGPU files are always bitcode images and will always use the LTO
backend. Additionally we now pass the default architecture found with
the amdgpu-arch tool to the argument list.

Depends on D117156

Diff Detail

Event Timeline

jhuber6 created this revision.Jan 13 2022, 1:15 PM
jhuber6 requested review of this revision.Jan 13 2022, 1:15 PM
Herald added a project: Restricted Project. · View Herald TranscriptJan 13 2022, 1:15 PM

A bit nervous about the automatic appending of host library search paths for the device link but I see that's pre-existing in nvptx. I'll check whether -### changes behaviour as expected and approve if it does

clang/lib/Driver/ToolChains/AMDGPUOpenMP.cpp
297 ↗(On Diff #399760)

This part is valuable as-is and probably independently testable, in that I think -### will have no march= before this change and will contain march=gfx*** after it. Though for that test to work reliably, I think it would have to run on a machine where amdgpu-arch succeeded, which is probably more bother than it's worth. Will check locally.

clang/tools/clang-linker-wrapper/ClangLinkerWrapper.cpp
609

Does this mean to take the library paths the host used to link, and pass them unchanged for the device link as well? Doesn't seem a given that this is the right behaviour. Do we have a mechanism for passing arguments to the device linker? Might be ugly, e.g. -Xopenmp-target=sm_70 -Xopenmp-target=-Wl,whatever

jhuber6 added inline comments.Jan 19 2022, 10:34 AM
clang/lib/Driver/ToolChains/AMDGPUOpenMP.cpp
297 ↗(On Diff #399760)

Yes, I believe with this we could remove the other calls to checkSystemForAMDGPU and just reference the -march in the driver arguments. Not sure about testing because it requires the binary of course.

clang/tools/clang-linker-wrapper/ClangLinkerWrapper.cpp
609

That's a good idea for general control over the arguments to the linker wrapper. I don't think the -L arguments are strictly necessary here. Could probably move to your approach if needed, the -L arguments were already present in the clang-nvlink-wrapper so I just replicated that without really caring if it was necessary.

jhuber6 updated this revision to Diff 401466.Jan 19 2022, 6:04 PM

Updating after upstreaming a portion of this patch.

jhuber6 updated this revision to Diff 402929.Jan 25 2022, 8:27 AM

Update commits.

@JonChesterfield can we just accept the patch and make it in 14? If we later find anything broken, we could have bug fix.

tianshilei1992 accepted this revision.Jan 31 2022, 7:46 PM
This revision is now accepted and ready to land.Jan 31 2022, 7:46 PM
This revision was landed with ongoing or failed builds.Jan 31 2022, 8:12 PM
This revision was automatically updated to reflect the committed changes.