This is an archive of the discontinued LLVM Phabricator instance.

[docs]Updated the AMD GPU Attributes documentation
AbandonedPublic

Authored by pooja2299 on May 9 2021, 10:08 AM.

Details

Summary

Changed the documentation of amdgpu_flat_work_group_size under AMD GPU Attributes which suggested that attribute is an optimization hint. But as suggested in the bug https://bugs.llvm.org/show_bug.cgi?id=42989, it should be made mandatory.

Diff Detail

Unit TestsFailed

Event Timeline

pooja2299 created this revision.May 9 2021, 10:08 AM
pooja2299 requested review of this revision.May 9 2021, 10:08 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 9 2021, 10:08 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
pooja2299 edited reviewers, added: gandhi21299; removed: aaron.ballman.May 9 2021, 10:14 AM
pooja2299 updated this revision to Diff 343919.May 9 2021, 10:32 AM

Amended the flat_work_group_size section.

pooja2299 edited reviewers, added: arsenm; removed: aaron.ballman.May 9 2021, 10:35 AM
pooja2299 updated this revision to Diff 343920.May 9 2021, 10:45 AM

Made some corrections

aaron.ballman requested changes to this revision.May 10 2021, 5:39 AM

Minor wordsmithing on the documentation changes, but more importantly: why is the correct fix to the documentation as opposed to changing the default max working group size?

clang/include/clang/Basic/AttrDocs.td
2244–2247
This revision now requires changes to proceed.May 10 2021, 5:39 AM
pooja2299 added a comment.EditedMay 11 2021, 10:15 AM

Minor wordsmithing on the documentation changes, but more importantly: why is the correct fix to the documentation as opposed to changing the default max working group size?

Hi @aaron.ballman. Thanks for your feedback! I am an outreachy applicant and totally new to this project. I am currently trying to understand the code base. So thought to update the documentation meanwhile. Later on we can change the default max working group size with your suggestion. What do you say, should we directly change the default max working group size and not the documentation?

Minor wordsmithing on the documentation changes, but more importantly: why is the correct fix to the documentation as opposed to changing the default max working group size?

Hi @aaron.ballman. Thanks for your feedback! I am an outreachy applicant and totally new to this project. I am currently trying to understand the code base.

Welcome!

So thought to update the documentation meanwhile. Later on we can change the default max working group size with your suggestion. What do you say, should we directly change the default max working group size and not the documentation?

I'm not an AMD person and so I'm not certain I'm the *best* person to answer this, but my feeling is that this is a case where the implementation should be updated rather than the docs. Otherwise, we're effectively encouraging users to churn their code (add the attribute to places they didn't use it before) with the intention of undoing that in the future. However, I'm hoping someone more familiar with AMDGPU can pipe up with their opinions. @arsenm?

arsenm added inline comments.May 11 2021, 12:02 PM
clang/include/clang/Basic/AttrDocs.td
2244–2247

You're updating this with outdated information. In general functions should be conservatively correct by default with no attribute specified. This was broken at one point in the past. The default assumed workgroup size is now 1024, but for opencl clang will always default to a max of 256

xgupta resigned from this revision.EditedMay 12 2021, 9:48 AM

I am really not an idol reviewer for this patch -:) don't know anything about AMDGPU.

xgupta added a subscriber: xgupta.May 12 2021, 9:52 AM
pooja2299 added inline comments.May 19 2021, 8:31 AM
clang/include/clang/Basic/AttrDocs.td
2244–2247

Ohh. Thanks for your feedback. Will update it

xgupta removed a subscriber: xgupta.May 24 2021, 3:09 AM
pooja2299 abandoned this revision.Jun 23 2021, 1:26 AM

Closing this issue because the default workgroup size is 1024 now, so no changes are required.