This is an archive of the discontinued LLVM Phabricator instance.

[HIP] Do not use llvm-link/opt/llc for -fgpu-rdc
ClosedPublic

Authored by yaxunl on Jun 15 2020, 11:30 AM.

Details

Summary

This patch is a follow up on https://reviews.llvm.org/D81627.

In addition to default -fno-gpu-rdc case, this patches let
HIP toolchain not use llvm-link/opt/llc to link device code
for -fgpu-rdc case. Instead, uses standard lto.

This will eliminate some redundant optimizations and speed
up the compilation/linking.

Diff Detail

Event Timeline

yaxunl created this revision.Jun 15 2020, 11:30 AM
arsenm added a subscriber: arsenm.Jun 15 2020, 12:00 PM

Thanks for doing this, this has been sorely needed since forever. We also really need to switch opencl over to this

clang/lib/Driver/ToolChains/CommonArgs.cpp
207–209 ↗(On Diff #270808)

Why is this necessary?

yaxunl marked an inline comment as done.Jun 15 2020, 12:12 PM
yaxunl added inline comments.
clang/lib/Driver/ToolChains/CommonArgs.cpp
207–209 ↗(On Diff #270808)

HIP uses -march to pass the processor. I can switch to use -mcpu in HIP toolchain.

Thank you for this patch, it looks good, and I have a few minor questions/comments.

clang/test/Driver/hip-phases.hip
34

should also use device-[[T]]? I think there are several locations here using device-hip instead of device[[T]].

253

[[T]] is defined everytime here, should probably use [[T]] in subsequent checks?

clang/test/Driver/hip-save-temps.hip
58

Should this link test be moved into a separate file? Also to cover without -o? I may also be adding more tests to this case for static lib generation too.

clang/test/Driver/hip-toolchain-rdc.hip
10

What is the difference between this file and hip-toolchain-rdc-separate.hip?

yaxunl updated this revision to Diff 270825.Jun 15 2020, 12:26 PM
yaxunl added a reviewer: arsenm.

use -mcpu instead of march in HIP toolchain.

yaxunl marked 5 inline comments as done.Jun 15 2020, 12:41 PM
yaxunl added inline comments.
clang/test/Driver/hip-phases.hip
34

will fix

253

But those [[T]] are defined through check prefixes not used by this run line, so we have to define it here.

clang/test/Driver/hip-save-temps.hip
58

will do

clang/test/Driver/hip-toolchain-rdc.hip
10

this one do compile and link together. that one do compile and link with separate commands.

yaxunl marked 2 inline comments as done.Jun 15 2020, 12:46 PM
yaxunl added inline comments.
clang/test/Driver/hip-phases.hip
253

sorry I misunderstood. Yes you are right. It only needs to be defined in the first line.

yaxunl updated this revision to Diff 270836.Jun 15 2020, 1:03 PM
yaxunl marked 3 inline comments as done.

fix tests

ashi1 accepted this revision.EditedJun 15 2020, 2:49 PM

LGTM, ready to submit if @tra and @arsenm are okay with this patch.

This revision is now accepted and ready to land.Jun 15 2020, 2:49 PM
tra accepted this revision.Jun 15 2020, 3:24 PM

LGTM.

This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptJun 15 2020, 6:14 PM
hjyamauchi added inline comments.
clang/test/Driver/hip-toolchain-no-rdc.hip
164

Hi, this test seems to fail for me because I happen to have the string "opt" in the "InstallDir" file path to the clang binary in my workstation, like

clang version 11.0.0 (https://github.com/llvm/llvm-project.git 6bc2b042f4a9e8d912901679bd4614d46f6fafed)
Target: x86_64-unknown-linux-gnu
Thread model: posix
InstalledDir: /opt/llvm-project/build/bin

Is there a way for this check pattern to be refined to avoid this issue?

arsenm added inline comments.Jun 16 2020, 11:37 AM
clang/test/Driver/hip-toolchain-no-rdc.hip
164

I think the negative checks aren't really necessary

yaxunl marked an inline comment as done.Jun 16 2020, 12:27 PM
yaxunl added inline comments.
clang/test/Driver/hip-toolchain-no-rdc.hip
164

it can be changed to

// LINK-NOT: {{".*/opt"}}

hjyamauchi added inline comments.Jun 17 2020, 12:42 PM
clang/test/Driver/hip-toolchain-no-rdc.hip
164

Uploaded D82046 for this.