This is an archive of the discontinued LLVM Phabricator instance.

[HIP] Support offloading by linker script
ClosedPublic

Authored by yaxunl on May 4 2018, 2:09 PM.

Details

Summary

To support linking device code in different source files, it is necessary to
embed fat binary at host linking stage.

This patch emits an external symbol for fat binary in host codegen, then
embed the fat binary by lld through a linker script.

Diff Detail

Repository
rL LLVM

Event Timeline

yaxunl created this revision.May 4 2018, 2:09 PM

The IRGen aspects of this seem reasonable, modulo a comment change. I don't feel qualified to judge the driver change.

lib/CodeGen/CGCUDANV.cpp
317 ↗(On Diff #145291)

Is this filename string only used for CUDA? If so, please rename it to make that clear, or at least add a comment.

yaxunl marked an inline comment as done.May 7 2018, 8:57 AM
yaxunl added inline comments.
lib/CodeGen/CGCUDANV.cpp
317 ↗(On Diff #145291)

Yes. Will do.

yaxunl updated this revision to Diff 145480.May 7 2018, 9:25 AM
yaxunl marked 2 inline comments as done.

Rename variables used for CUDA only.

yaxunl retitled this revision from [HIP] Support offloading by linkscript to [HIP] Support offloading by linker script.May 15 2018, 9:12 AM
yaxunl edited the summary of this revision. (Show Details)
yaxunl added reviewers: Hahnfeld, hfinkel.
t-tye accepted this revision.May 17 2018, 1:17 PM

LGTM except for minor suggestions.

lib/CodeGen/CGCUDANV.cpp
361–373 ↗(On Diff #145480)

Should this all be inside an if (IsHip) so the names can be different for HIP and NV CUDA? Seems HIP should not be using names with VV_CUDA in them. In fact maybe the following IF should be included as well to consilidate the NV CUDA code and HIP code together.

394–398 ↗(On Diff #145480)

Should HIP use the same magic value and version number? Perhaps this should also be moved into the IsHip IF.

438 ↗(On Diff #145480)

Update comment for HIP

lib/Driver/ToolChains/CommonArgs.cpp
150 ↗(On Diff #145480)

OpenMP -> OpenMP or HIP

1321–1322 ↗(On Diff #145480)

for for -> for

This revision is now accepted and ready to land.May 17 2018, 1:17 PM

LGTM except for minor suggestions.

Thanks. Will make changes when committing.

This revision was automatically updated to reflect the committed changes.
tra added inline comments.May 18 2018, 4:26 PM
cfe/trunk/lib/Driver/ToolChains/CommonArgs.cpp
1371–1388

Using this linker script may present a problem.

INSERT BEFORE is not going to work with ld.gold.
https://sourceware.org/bugzilla/show_bug.cgi?id=15373

LLD also does not handle it particularly well -- INSERT BEFORE can only be used to override explicitly specified external linker script and virtually nobody uses linker scripts with LLD.
See tests in https://reviews.llvm.org/D44380

pcc added a subscriber: pcc.May 18 2018, 4:38 PM
pcc added inline comments.
cfe/trunk/lib/Driver/ToolChains/CommonArgs.cpp
1371–1388

If you're just trying to embed a file it may be better to use MC to write an ELF with something like:

.section .hip_fatbin
.align 16
.globl __hip_fatbin
__hip_fatbin:
.incbin "BundleFileName"

and add that to the link.

yaxunl added inline comments.May 21 2018, 6:57 AM
cfe/trunk/lib/Driver/ToolChains/CommonArgs.cpp
1371–1388

In https://reviews.llvm.org/D45212 we specified to use lld for linking. Since this is an explicit linker script, it works. We have tested this approach with many HIP applications that it works well.

Similar approach has also been used by OpenMP for embedding device binary.

1371–1388

Thanks for your suggestion. We will consider using MC if we see issues arise due to linker script.

JonChesterfield added inline comments.
cfe/trunk/lib/Driver/ToolChains/CommonArgs.cpp
1329

Should this be toolchains::HipToolChain?

yaxunl marked 2 inline comments as done.Apr 23 2020, 8:27 AM
yaxunl added inline comments.
cfe/trunk/lib/Driver/ToolChains/CommonArgs.cpp
1329

will fix. thanks