This is an archive of the discontinued LLVM Phabricator instance.

[HIPSPV][1/4] Refactor HIP tool chain
ClosedPublic

Authored by linjamaki on Sep 27 2021, 6:24 AM.

Details

Summary

This patch refactors the HIP tool chain for new HIP tool chain, HIPSPV
tool chain, which is added in the follow up patch part 2.

  • Rename HIPToolChain to HIPAMDToolChain and Renames HIP.* files to HIPAMD.*.
  • Introduce HIPUtility.* file where common HIP utilities, shared among HIP tool chain implementations, are placed in.
  • Move constructHIPFatbinCommand() and constructGenerateObjFileFromHIPFatBinary() to HIPUtility. HIPSPV tool chain is going to use them.
  • Tweak bundle target ID in constructHIPFatbinCommand(): extra dashes are dropped if the Target ID is empty and 'hip' offload kind is made default for non-AMD targets.

Diff Detail

Event Timeline

linjamaki created this revision.Sep 27 2021, 6:24 AM
linjamaki edited the summary of this revision. (Show Details)Sep 28 2021, 1:04 AM
linjamaki updated this revision to Diff 375488.Sep 28 2021, 1:24 AM

Style fixes.

linjamaki published this revision for review.Sep 28 2021, 1:29 AM
linjamaki edited the summary of this revision. (Show Details)
linjamaki added a reviewer: yaxunl.
Herald added a project: Restricted Project. · View Herald TranscriptSep 28 2021, 1:29 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
yaxunl accepted this revision.Nov 18 2021, 7:26 AM

LGTM. Thanks.

This revision is now accepted and ready to land.Nov 18 2021, 7:26 AM
tra added a subscriber: echristo.Nov 22 2021, 1:11 PM
tra added inline comments.
clang/lib/Driver/ToolChains/HIPUtility.cpp
120–134

Using MC for just wrapping a blob into an object file strikes me as something MC is not intended for.

@echristo -- is it OK to use MC as a tool in the standard compilation pipeline? I vaguely recall we had a conversaion about using objcopy for similar purposes during early days of CUDA and the conclusion was that we generally don't want that. I might be wrong, too, it's been too long ago.

echristo added inline comments.Nov 22 2021, 1:55 PM
clang/lib/Driver/ToolChains/HIPUtility.cpp
120–134

Using llvm-mc the binary? No. That's definitely not something that should be done. And as far as objcopy we spent quite a bit of time pulling it out for split dwarf and, if possible, it shouldn't be used there either.

yaxunl added inline comments.Nov 22 2021, 3:27 PM
clang/lib/Driver/ToolChains/HIPUtility.cpp
120–134

HIP toolchain has used llvm-mc to embed fat binary in a host object for a long time.

This is for supporting -fgpu-rdc since separated compilation/link will result in host object and device bit code. After device bit code are linked and fat binary are generated, fat binary needs to be embedded in some host object and linked with other host objects.

In the beginning, we used linker script. That works until we need to support windows. Then we adopted this suggestion (https://reviews.llvm.org/D46472#inline-412796) and use llvm-mc to generate a host object containing the fat binary, which works for both Linux and Windows.

I am wondering what is wrong with this approach and what is a better approach.

Thanks for the review.

Assuming this patch is ready to land. @yaxunl, Could you please commit this patch to the LLVM for us. Thanks.

Assuming this patch is ready to land. @yaxunl, Could you please commit this patch to the LLVM for us. Thanks.

I will test it with our internal CI, then commit it.

This revision was landed with ongoing or failed builds.Dec 13 2021, 7:50 AM
This revision was automatically updated to reflect the committed changes.