This is an archive of the discontinued LLVM Phabricator instance.

[HIP][Clang][Driver][RFC] Add driver support for C++ Parallel Algorithm Offload
ClosedPublic

Authored by AlexVlx on Jul 19 2023, 8:20 PM.

Details

Summary

This patch adds the Driver changes needed by the standard algorithm offload feature being proposed here: https://discourse.llvm.org/t/rfc-adding-c-parallel-algorithm-offload-support-to-clang-llvm/72159/1. The verbose documentation is included in its parent patch. What this change does can be summed up as follows:

  1. add two flags, one for enabling stdpar compilation, the second enabling the optional allocation interposition mode;
  2. the flags correspond to new LangOpt members;
  3. if we are compiling or linking with -stdpar, we enable HIP; in the compilation case C and C++ inputs are treated as HIP inputs;
  4. the ROCm / AMDGPU driver is augmented to look for and include an implementation detail forwarding header, which is provided here https://github.com/ROCmSoftwarePlatform/roc-stdpar/blob/main/include/stdpar_lib.hpp; we error out if the user requested stdpar but the header or its dependencies cannot be found (it is plausible that in the future we'll move the check for the dependencies to the header itself in order to reduce the compiler footprint).

Tests for the behaviour described above are also added.

Diff Detail

Event Timeline

AlexVlx created this revision.Jul 19 2023, 8:20 PM
Herald added a project: Restricted Project. · View Herald Transcript
AlexVlx requested review of this revision.Jul 19 2023, 8:20 PM
Herald added a project: Restricted Project. · View Herald TranscriptJul 19 2023, 8:20 PM
AlexVlx updated this revision to Diff 542287.Jul 19 2023, 8:23 PM

Removed some accidental noise.

AlexVlx updated this revision to Diff 544974.Jul 27 2023, 4:03 PM

Exploit the fact that ROCm/AMDGPU does LTCG at the moment and for the foreseeable future by moving the accelerator code selection pass to later.

AlexVlx updated this revision to Diff 547570.Aug 6 2023, 6:08 AM
AlexVlx updated this revision to Diff 552556.Aug 22 2023, 6:28 PM
AlexVlx retitled this revision from [Clang][Driver][RFC] Add driver support for C++ Parallel Algorithm Offload to [HIP][Clang][Driver][RFC] Add driver support for C++ Parallel Algorithm Offload.

Updating this to reflect the outcome of the RFC, which is that this will be added as a HIP extension exclusively.

MaskRay added inline comments.Aug 23 2023, 12:11 AM
clang/test/Driver/hipstdpar.c
2

Better to use --hipstdpar instead of -hipstdpar.

In GCC, -h is a Joined Separate option that accepts a value.

AlexVlx updated this revision to Diff 552738.Aug 23 2023, 8:45 AM

Use --hipstdpar instead of -hipstdpar in the test.

AlexVlx marked an inline comment as done.Aug 23 2023, 8:46 AM

Gentle ping.

yaxunl added inline comments.Sep 19 2023, 8:49 AM
clang/include/clang/Basic/DiagnosticDriverKinds.td
75

needs to update the option name, same as below

AlexVlx updated this revision to Diff 557071.Sep 19 2023, 12:59 PM

Use correct flag naming.

AlexVlx marked an inline comment as done.Sep 19 2023, 12:59 PM
yaxunl accepted this revision.Sep 25 2023, 6:11 AM

LGTM. Thanks

This revision is now accepted and ready to land.Sep 25 2023, 6:11 AM
MaskRay added inline comments.Sep 25 2023, 9:55 AM
clang/include/clang/Driver/Options.td
1261

Accept just -- for new options. See --hip-path=.

MaskRay added inline comments.Sep 25 2023, 10:02 AM
clang/lib/Driver/ToolChains/Clang.cpp
6585

This can use AddLastArg.

AlexVlx updated this revision to Diff 557328.Sep 25 2023, 2:08 PM

Use double dash compiler flags only.

AlexVlx marked 2 inline comments as done.Sep 25 2023, 2:08 PM

@MaskRay do you think there's anything else that needs adjusting? Thanks.

MaskRay accepted this revision.Sep 28 2023, 9:57 AM
MaskRay added inline comments.
clang/include/clang/Driver/Options.td
1281

The convention is to omit the trailing dot.

clang/lib/Driver/ToolChains/AMDGPU.cpp
340

Prefer concat so that if HIPRocThrustPathArg ends with /, we will not get ...//thrust.

clang/lib/Driver/ToolChains/Clang.cpp
6583

Note that I have refactored the code to use addOptInFlag. You need to remove the repeated OPT_fgpu_allow_device_init checking.

AlexVlx updated this revision to Diff 557492.Sep 29 2023, 9:30 AM
AlexVlx marked 2 inline comments as done.

Remove noise, add missing stub folders needed by the test.

clang/lib/Driver/ToolChains/AMDGPU.cpp
340

If you don't mind, I'll do this in a subsequent patch since it's just annoying and not an outright error. The issue being that RocmInstallationDetector is not a ToolChain and isn't friendly to one either, so concat is inaccessible. Addressing that would add a bit of unrelated noise (looking through the file we probably want concat in more places).

MaskRay added inline comments.Sep 29 2023, 10:24 AM
clang/lib/Driver/ToolChains/AMDGPU.cpp
340

Don't mind:)

This revision was automatically updated to reflect the committed changes.
thakis added a subscriber: thakis.Oct 3 2023, 8:26 AM

This seems to break tests on Mac and windows, see eg http://45.33.8.238/macm1/70415/step_7.txt

Please take a look and revert for now if it takes a while to fix.

This seems to break tests on Mac and windows, see eg http://45.33.8.238/macm1/70415/step_7.txt

Please take a look and revert for now if it takes a while to fix.

Whoops, thank you for flagging this, my bad. At the moment (and for the foreseeable future) this is Linux only, so I think I'd rather XFAIL it on non Linux targets, if that's OK.

yaxunl added a comment.Oct 3 2023, 8:45 AM

This seems to break tests on Mac and windows, see eg http://45.33.8.238/macm1/70415/step_7.txt

Please take a look and revert for now if it takes a while to fix.

Whoops, thank you for flagging this, my bad. At the moment (and for the foreseeable future) this is Linux only, so I think I'd rather XFAIL it on non Linux targets, if that's OK.

LGTM

https://reviews.llvm.org/rG0701ee69f7ac should fix it, once again apologies for the noise.

dyung added a subscriber: dyung.Oct 3 2023, 10:52 AM

Hi @AlexVlx, the test you added is failing on an linux bot, can you take a look and revert if you need time to investigate? It appears it has been failing for over 6 hours.

https://lab.llvm.org/buildbot/#/builders/139/builds/50940

dyung added a comment.Oct 3 2023, 11:02 AM

https://reviews.llvm.org/rG0701ee69f7ac should fix it, once again apologies for the noise.

FYI, the test is still failing on the Windows PS5 bot after your change: https://lab.llvm.org/buildbot/#/builders/216/builds/28289

Hi @AlexVlx, the test you added is failing on an linux bot, can you take a look and revert if you need time to investigate? It appears it has been failing for over 6 hours.

https://lab.llvm.org/buildbot/#/builders/139/builds/50940

This is another unsupported target (as is hexagon which is also failing), that I'll temporarily disable, but I admit to being actually confused as to why it's failing there, because the sequence it cannot find appears to be there. Thank you for flagging it.

dyung added a comment.Oct 3 2023, 11:11 AM

Hi @AlexVlx, the test you added is failing on an linux bot, can you take a look and revert if you need time to investigate? It appears it has been failing for over 6 hours.

https://lab.llvm.org/buildbot/#/builders/139/builds/50940

This is another unsupported target (as is hexagon which is also failing), that I'll temporarily disable, but I admit to being actually confused as to why it's failing there, because the sequence it cannot find appears to be there. Thank you for flagging it.

Note that this linux bot builds with the ps4 target as the default target.

At this point, would it be easier to add a REQUIRES line for the target the test should support rather than just whack-a-mole for the targets it does not?

Hi @AlexVlx, the test you added is failing on an linux bot, can you take a look and revert if you need time to investigate? It appears it has been failing for over 6 hours.

https://lab.llvm.org/buildbot/#/builders/139/builds/50940

This is another unsupported target (as is hexagon which is also failing), that I'll temporarily disable, but I admit to being actually confused as to why it's failing there, because the sequence it cannot find appears to be there. Thank you for flagging it.

Note that this linux bot builds with the ps4 target as the default target.

At this point, would it be easier to add a REQUIRES line for the target the test should support rather than just whack-a-mole for the targets it does not?

Oh, it definitely would be MUCH easier to add a REQUIRES line, however I'd have had to have thought about that, which I did not, so thank you for the wake up call. I'll update this in a few minutes.

ro added a subscriber: ro.Oct 4 2023, 1:15 AM

At this point, would it be easier to add a REQUIRES line for the target the test should support rather than just whack-a-mole for the targets it does not?

Oh, it definitely would be MUCH easier to add a REQUIRES line, however I'd have had to have thought about that, which I did not, so thank you for the wake up call. I'll update this in a few minutes.

Unfortunately, this is still not done after almost a day, leaving quite a number of buildbots broken. Please fix or revert.

In D155775#4652851, @ro wrote:

At this point, would it be easier to add a REQUIRES line for the target the test should support rather than just whack-a-mole for the targets it does not?

Oh, it definitely would be MUCH easier to add a REQUIRES line, however I'd have had to have thought about that, which I did not, so thank you for the wake up call. I'll update this in a few minutes.

Unfortunately, this is still not done after almost a day, leaving quite a number of buildbots broken. Please fix or revert.

Apologies, do you have an indication of which buildbots are still broken due to this change, after the most recent commit? The ones I was initially aware of, as well as those brought up here appear to pass at the moment, and glancing through the currently failing builds didn’t point to this as being the culprit (I could’ve simply missed it). Thank you!

ro added a comment.Oct 4 2023, 2:53 AM
In D155775#4652851, @ro wrote:

At this point, would it be easier to add a REQUIRES line for the target the test should support rather than just whack-a-mole for the targets it does not?

Oh, it definitely would be MUCH easier to add a REQUIRES line, however I'd have had to have thought about that, which I did not, so thank you for the wake up call. I'll update this in a few minutes.

Unfortunately, this is still not done after almost a day, leaving quite a number of buildbots broken. Please fix or revert.

Apologies, do you have an indication of which buildbots are still broken due to this change, after the most recent commit? The ones I was initially aware of, as well as those brought up here appear to pass at the moment, and glancing through the currently failing builds didn’t point to this as being the culprit (I could’ve simply missed it). Thank you!

At least the Solaris/sparcv9 and Solaris/amd64 buildbots are still affected. However, XFAILing a long list of targets when it's known that particular targets/target properties are required for the test to PASS is fundamentally wrong!