This is an archive of the discontinued LLVM Phabricator instance.

[hip] Fix device-only relocatable code compilation.
ClosedPublic

Authored by hliao on Jun 8 2020, 1:31 PM.

Details

Summary
  • In HIP, just as the regular device-only compilation, the device-only relocatable code compilation should not involve offload bundle.
  • In addition, that device-only relocatable code compilation should have the similar 3 steps, namely preprocessor, compile, and backend, to the regular code generation with -emit-llvm.

Diff Detail

Event Timeline

hliao created this revision.Jun 8 2020, 1:31 PM
Herald added a project: Restricted Project. · View Herald TranscriptJun 8 2020, 1:31 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript
tra added a comment.Jun 8 2020, 2:24 PM

LGTM in general, but I'll let Sam stamp it.

clang/lib/Driver/Driver.cpp
4594–4599

This is rather hard to understand.
Would it be simpler to specify when we shouldn't add .tmp?
At the very least I'd extract the newly added clause into a temporary variable and would add some comments explaining why -fgpu-rdc gets special treatment.

clang/test/Driver/hip-device-only.hip
6

We appear to test -fgpu-rdc only and the test case seems to be fairly specific to the way that option is handled by HIP.
Perhaps the test file should reflect that. hip-rdc-device-only ?

hliao updated this revision to Diff 269555.Jun 9 2020, 8:32 AM

Revise following reviewer's comment.

hliao marked 2 inline comments as done.Jun 9 2020, 8:33 AM
yaxunl accepted this revision.Jun 9 2020, 9:59 AM

LGTM. thanks

This revision is now accepted and ready to land.Jun 9 2020, 9:59 AM
thakis added a subscriber: thakis.Jun 10 2020, 11:21 AM

This doesn't pass tests: http://45.33.8.238/linux/19977/step_7.txt

Please take a look, and please revert for now if fixing takes a while.

This doesn't pass tests: http://45.33.8.238/linux/19977/step_7.txt

Please take a look, and please revert for now if fixing takes a while.

Thanks. I am building with PowerPC enabled to reproduce this issue. There should be a minor fix on that test case.

This revision was automatically updated to reflect the committed changes.

This doesn't pass tests: http://45.33.8.238/linux/19977/step_7.txt

Please take a look, and please revert for now if fixing takes a while.

Fixed in https://github.com/llvm/llvm-project/commit/6dd058083208d58c6a7005bfb092cee085fd9a48