Page MenuHomePhabricator

Add HIP toolchain
ClosedPublic

Authored by yaxunl on Apr 3 2018, 9:27 AM.

Details

Summary

This patch adds HIP toolchain to support HIP language mode. It includes:

Create specific compiler jobs for HIP.

Choose specific libraries for HIP.

With contribution from Greg Rodgers.

Diff Detail

Repository
rC Clang

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
yaxunl added inline comments.Apr 19 2018, 7:25 PM
lib/Driver/ToolChains/Cuda.cpp
498–501 ↗(On Diff #141863)

These are not disabling the passes but adding these passes. They are some optimizations which are usually improving performance for amdgcn.

yaxunl updated this revision to Diff 143218.Apr 19 2018, 7:54 PM
yaxunl marked 14 inline comments as done.

Revised by Artem's comments.

yaxunl updated this revision to Diff 143298.Apr 20 2018, 5:54 AM
yaxunl edited the summary of this revision. (Show Details)

sync to ToT.

yaxunl updated this revision to Diff 143307.Apr 20 2018, 6:27 AM

minor bug fix: do not add CUDA specific link options for HIP.

yaxunl marked an inline comment as done.Apr 20 2018, 8:26 AM
yaxunl added inline comments.
lib/Driver/ToolChains/Cuda.cpp
498–501 ↗(On Diff #141863)

Since opt is now able to adjust passes based on -mtriple and -mcpu, will remove these manually added passes and add -mtriple and -mcpu instead.

yaxunl updated this revision to Diff 143320.Apr 20 2018, 8:29 AM
yaxunl marked an inline comment as done.

Remove manually added passes from opt and add -mtriple.

yaxunl marked 14 inline comments as done.Apr 26 2018, 4:08 AM

ping

yaxunl updated this revision to Diff 145297.May 4 2018, 2:19 PM

clean up code and separate action builder to another review.

Hi Artem,

I've addressed your comments. Any further changes are needed? Thanks.

tra added a comment.May 18 2018, 3:17 PM

Hi,

Sorry about the long silence. I'm back to continue the reviews. I'll handle what I can today and will continue with the rest on Tuesday.

It looks like patch description needs to be updated:

Use clang-offload-bindler to create binary for device ISA.

I don't see anything related to offload-bundler in this patch any more.

include/clang/Driver/Options.td
588

I'm not sure about i_Group? This will cause this option to be passed to all preprocessor jobs. It will also be passed to host and device side compilations, while you probably only want/need it on device side only.

lib/Driver/ToolChains/Cuda.cpp
323 ↗(On Diff #145297)

FullName is already result of Args.MakeArgString. You only need to do it once.

329 ↗(On Diff #145297)

This function is too large to easily see that we're actually constructing sequence of commands.
I'd probably split construction of individual tool's command line into its own function.

336 ↗(On Diff #145297)

No need for the leading space in the message.

344–345 ↗(On Diff #145297)

for (const InputInfo &it : Inputs) ?

350 ↗(On Diff #145297)

All-caps name looks like a macro. Rename to GfxName ?

354–359 ↗(On Diff #145297)
for (path : Args.getAllArgValues(...)) {
   LibraryPaths.push_back(Args.MakeArgString(path));
}
375–378 ↗(On Diff #145297)

This is somewhat unreadable. Perhaps you could construct the name in a temp variable.

384 ↗(On Diff #145297)

You don't need to use c_str() for MakeArgString. It will happily accept std::string.

394 ↗(On Diff #145297)

BitcodeOutputFile?

417 ↗(On Diff #145297)

I think you can get rid of the temp var here without hurting readability.

420 ↗(On Diff #145297)

I wonder if we could derive temp file name from the input's name. This may make it easier to find relevant temp files if/when we need to debug the compilation process.

422 ↗(On Diff #145297)

No need for c_str() here.

439 ↗(On Diff #145297)

c_str(), again.

764 ↗(On Diff #145297)

I'd just add something like this and leave existing if unchanged:

// There's no need for CUDA-specific bitcode linking with HIP.
if( DeviceOffloadingKind == Action::OFK_HIP)
  return;
926–938 ↗(On Diff #145297)

All these should be in the derived toolchain.

lib/Driver/ToolChains/Cuda.h
137–138 ↗(On Diff #145297)

Where does amdgcn-link come from? Does it accept --options-file ?

153–154 ↗(On Diff #145297)

You may want to derive your own HIPToolChain as you're unlikely to reuse NVPTX-specific tools.

172 ↗(On Diff #145297)

In HIPToolchain this one would become an inline function returning true.

tra added a comment.May 18 2018, 3:51 PM

One more thing -- it would be really good to add some tests to make sure your commands are constructed the way you want.

yaxunl marked 19 inline comments as done.May 23 2018, 7:59 AM
In D45212#1105177, @tra wrote:

Hi,

Sorry about the long silence. I'm back to continue the reviews. I'll handle what I can today and will continue with the rest on Tuesday.

It looks like patch description needs to be updated:

Use clang-offload-bindler to create binary for device ISA.

I don't see anything related to offload-bundler in this patch any more.

You are right. Using clang-offload-bundler to create binary for device ISA has been moved to another patch. Will update the description of this patch.

In D45212#1105282, @tra wrote:

One more thing -- it would be really good to add some tests to make sure your commands are constructed the way you want.

will do

include/clang/Driver/Options.td
588

will change to Link_Group

lib/Driver/ToolChains/Cuda.cpp
323 ↗(On Diff #145297)

will fix

329 ↗(On Diff #145297)

will do

336 ↗(On Diff #145297)

will fix.

344–345 ↗(On Diff #145297)

will fix

350 ↗(On Diff #145297)

will fix

354–359 ↗(On Diff #145297)

will fix

375–378 ↗(On Diff #145297)

will do

384 ↗(On Diff #145297)

will fix

394 ↗(On Diff #145297)

will change

417 ↗(On Diff #145297)

will do

420 ↗(On Diff #145297)

will do

422 ↗(On Diff #145297)

will do

439 ↗(On Diff #145297)

will fix

764 ↗(On Diff #145297)

will change

926–938 ↗(On Diff #145297)

will do

lib/Driver/ToolChains/Cuda.h
137–138 ↗(On Diff #145297)

amdgcn-link is the short name of the amdgcn bitcode linker. It is not a real program and does not support response file. Will remove the arguments about response file.

153–154 ↗(On Diff #145297)

Will do

172 ↗(On Diff #145297)

will do

yaxunl updated this revision to Diff 148216.May 23 2018, 8:05 AM
yaxunl marked 19 inline comments as done.
yaxunl retitled this revision from [HIP] Let CUDA toolchain support HIP language mode and amdgpu to Add HIP toolchain.
yaxunl edited the summary of this revision. (Show Details)

Revised by Artem's comments.

tra added inline comments.May 23 2018, 12:13 PM
lib/Driver/ToolChains/HIP.cpp
30–48

FullName may remain uninitialized if LibraryPaths are empty which will probably crash compiler when you attempt to pass it to MakeArgString.
If empty LibraryPaths is not expected there should be an assert.

If the library is not found, we issue an error, but we still proceed to append the FullName to the CmdArgs. I don't think we should do that. FullName will be either NULL or pointing to the last directory in the LibraryPaths.

You seem to be relying on diagnostics to deal with errors and are not using return value of the function. You may as well make it void.

I'd move CmdArgs.push_back(...) under if(::exists(FullName)) and change break to return;
Then you can get rid of FoundLibDevice and just issue the error if we ever reach the end of the function.

80–82

No need for intermediate values here -- just '+' all parts together.

134

Nit: I think explicit llvm::Twine is unnecessary here.

156–161

Nit: THis could be collapsed into ArgStringList LlcArgs({...});

180–182

Same here: ArgStringList LldArgs({"-flavor", "gnu", "--no-undefined", "-shared", "-o"});

213–216

Right now the code is structured as if you're appending to the same TempFile string which is not the case here. I'd give intermediate variables their own names -- OptCommand,LlcCommand.
This would make it easier to see that you are chaining separate commands, each producing its own temp output file.

yaxunl marked 6 inline comments as done.May 23 2018, 1:31 PM
yaxunl added inline comments.
lib/Driver/ToolChains/HIP.cpp
30–48

Will CmdArgs.push_back(...) under if(::exists(FullName)) and change break to return; and change return type to void.

80–82

will do

134

will remove

156–161

will do

180–182

will do

213–216

will do

yaxunl updated this revision to Diff 148277.May 23 2018, 1:34 PM
yaxunl marked 6 inline comments as done.

Revised by Artem's comments.

tra accepted this revision.May 23 2018, 1:42 PM

One small nit. LGTM otherwise.

lib/Driver/ToolChains/HIP.cpp
45

You don't need FoundLibDevice any more as you will always return from inside the loop if it is ever true.

This revision is now accepted and ready to land.May 23 2018, 1:42 PM
yaxunl marked an inline comment as done.May 23 2018, 1:47 PM
yaxunl added inline comments.
lib/Driver/ToolChains/HIP.cpp
45

will remove when committing. Thanks!

This revision was automatically updated to reflect the committed changes.
yaxunl marked an inline comment as done.