This is an archive of the discontinued LLVM Phabricator instance.

[HIP][Clang][docs][RFC] Add documentation for C++ Parallel Algorithm Offload
AcceptedPublic

Authored by AlexVlx on Jul 19 2023, 6:59 PM.

Details

Summary

This patch adds the documentation for 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. It is the parent of a series of patches that make up the implementation.

Diff Detail

Event Timeline

AlexVlx created this revision.Jul 19 2023, 6:59 PM
Herald added a project: Restricted Project. · View Herald TranscriptJul 19 2023, 6:59 PM
Herald added a subscriber: arphaman. · View Herald Transcript
AlexVlx requested review of this revision.Jul 19 2023, 6:59 PM

Interesting.

clang/docs/StdParSupport.rst
350
355
361–363
367–368

Another way could be to hide somehow a way to select the device in the policy like in https://github.com/KhronosGroup/SyclParallelSTL, which might be something included in your point "4." of "Open Questions / Future Developments".
Perhaps better than opening the TLS Pandora box?

AlexVlx updated this revision to Diff 547572.Aug 6 2023, 6:19 AM

Remove confusing guidance around mGPU.

AlexVlx marked an inline comment as done.Aug 6 2023, 6:21 AM
AlexVlx added inline comments.
clang/docs/StdParSupport.rst
367–368

In hindsight, this was needlessly confusing and relied on an implementation detail, therefore the reference was removed. Thank you for pointing that out.

AlexVlx updated this revision to Diff 552550.Aug 22 2023, 6:03 PM
AlexVlx marked an inline comment as done.
AlexVlx retitled this revision from [Clang][docs][RFC] Add documentation for C++ Parallel Algorithm Offload to [HIP][Clang][docs][RFC] Add documentation for C++ Parallel Algorithm Offload.
AlexVlx removed reviewers: bader, Anastasia, erichkeane.

Updating this to reflect the outcome of the RFC, which is that this shall be a HIP only extension. As such, documentation lives within the HIP Support master document; the other patches in the series will be updated accordingly.

yaxunl added inline comments.Aug 23 2023, 8:16 AM
clang/docs/HIPSupport.rst
232 ↗(On Diff #552550)

typo j -> h

549 ↗(On Diff #552550)

can we add an item explaining whether relocatable object files compiled from --hipstdpar and HIP be linked together?

AlexVlx updated this revision to Diff 552754.Aug 23 2023, 9:26 AM

Update per review comments.

AlexVlx marked 2 inline comments as done.Aug 23 2023, 9:27 AM

seems some irrelevant change got into this patch

AlexVlx updated this revision to Diff 552763.Aug 23 2023, 9:43 AM

Remove noise.

AlexVlx updated this revision to Diff 553799.Aug 27 2023, 9:56 AM
AlexVlx edited reviewers, added: jhuber6; removed: tra, jdoerfert.

Fix typo, add release notes reflecting the HIP-only nature.

AlexVlx updated this revision to Diff 553800.Aug 27 2023, 10:00 AM

Add back clarification about RDC.

AlexVlx updated this revision to Diff 555450.Sep 1 2023, 11:56 AM
AlexVlx added a reviewer: JonChesterfield.

Correctly reflect AMDGPU-only nature of the extension.

keryell added inline comments.Sep 1 2023, 2:17 PM
clang/docs/HIPSupport.rst
216 ↗(On Diff #555450)

Since this does not sounds like an official wording and this is not a recommended practice https://isocpp.org/wiki/faq/coding-standards#using-namespace-std, I suggest just adding std:: everywhere since this is an end-user document.
Further more it makes clear that your extension can work with the standard library instead of something that would be declared in a namespace from your extension.

AlexVlx updated this revision to Diff 556369.Sep 10 2023, 8:03 AM

Fix some wording to further clarify this is an HIP + AMDGPU exclusive extension.

yaxunl accepted this revision.Sep 11 2023, 6:34 AM

LGTM. Please address Ronan's comments. Thanks.

This revision is now accepted and ready to land.Sep 11 2023, 6:34 AM
AlexVlx updated this revision to Diff 556618.Sep 12 2023, 5:06 PM

Address review feedback around explicit use of std::.

AlexVlx marked an inline comment as done.Sep 12 2023, 5:06 PM
AlexVlx added inline comments.
clang/docs/HIPSupport.rst
216 ↗(On Diff #555450)

Addressed, thanks!

keryell accepted this revision.Sep 12 2023, 5:57 PM

Thanks for the clarifications.

AlexVlx updated this revision to Diff 557057.Sep 19 2023, 10:05 AM
AlexVlx marked an inline comment as done.

Rebase.

MaskRay added inline comments.
clang/docs/HIPSupport.rst
266 ↗(On Diff #557057)

Newer long options that don't use the common prefix like -f are preferred to only support --foo, not -foo.
Many --hip* options don't support -hip*. Why add -hipstdpar?

AlexVlx added inline comments.Sep 25 2023, 2:02 PM
clang/docs/HIPSupport.rst
266 ↗(On Diff #557057)

Thanks for the review - to answer the question, it was only done for (apparent) symmetry, there's no strong incentive to have the single dash flavour; I will update both this and the driver patch to retain only the double dash flavours.

AlexVlx updated this revision to Diff 557368.Sep 26 2023, 11:17 AM

Use double dash flags exclusively.

AlexVlx marked an inline comment as done.Sep 26 2023, 11:17 AM