[PATCH] D102134: [docs]Updated the AMD GPU Attributes documentation

[PoojaYadav via Phabricator via cfe-commits]

pooja2299 added inline comments.



Comment at: clang/include/clang/Basic/AttrDocs.td:2244-2247
+and is an optimization hint. It is mandatory to use this attribute in some 
+situations. Because when the attribute is absent, the compiler assumes the 
+default maximum workgroup size of 256 but nowadays the workgroup size can 
legally go 
+to 1024. 

arsenm wrote:
> aaron.ballman wrote:
> > 
> 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
Ohh. Thanks for your feedback. Will update it 


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D102134/new/

https://reviews.llvm.org/D102134

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D102134: [docs]Updated the AMD GPU Attributes documentation

[PoojaYadav via Phabricator via cfe-commits]

pooja2299 abandoned this revision.
pooja2299 added a comment.

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


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D102134/new/

https://reviews.llvm.org/D102134

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D102134: [docs]Updated the AMD GPU Attributes documentation

[PoojaYadav via Phabricator via cfe-commits]

pooja2299 created this revision.
pooja2299 added a reviewer: xgupta.
Herald added a subscriber: tpr.
Herald added a reviewer: aaron.ballman.
pooja2299 requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

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.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D102134

Files:
  clang/include/clang/Basic/AttrDocs.td


Index: clang/include/clang/Basic/AttrDocs.td
===
--- clang/include/clang/Basic/AttrDocs.td
+++ clang/include/clang/Basic/AttrDocs.td
@@ -2240,8 +2240,8 @@
 
 Clang supports the
 ``__attribute__((amdgpu_flat_work_group_size(, )))`` attribute for 
the
-AMDGPU target. This attribute may be attached to a kernel function definition
-and is an optimization hint.
+AMDGPU target. This attribute can be attached to a kernel function definition
+and is mandatory in some situations.
 
  parameter specifies the minimum flat work-group size, and 
 parameter specifies the maximum flat work-group size (must be greater than


Index: clang/include/clang/Basic/AttrDocs.td
===
--- clang/include/clang/Basic/AttrDocs.td
+++ clang/include/clang/Basic/AttrDocs.td
@@ -2240,8 +2240,8 @@
 
 Clang supports the
 ``__attribute__((amdgpu_flat_work_group_size(, )))`` attribute for the
-AMDGPU target. This attribute may be attached to a kernel function definition
-and is an optimization hint.
+AMDGPU target. This attribute can be attached to a kernel function definition
+and is mandatory in some situations.
 
  parameter specifies the minimum flat work-group size, and 
 parameter specifies the maximum flat work-group size (must be greater than
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D102134: [docs]Updated the AMD GPU Attributes documentation

[PoojaYadav via Phabricator via cfe-commits]

pooja2299 updated this revision to Diff 343919.
pooja2299 added a comment.

Amended the flat_work_group_size section.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D102134/new/

https://reviews.llvm.org/D102134

Files:
  clang/include/clang/Basic/AttrDocs.td


Index: clang/include/clang/Basic/AttrDocs.td
===
--- clang/include/clang/Basic/AttrDocs.td
+++ clang/include/clang/Basic/AttrDocs.td
@@ -2241,7 +2241,10 @@
 Clang supports the
 ``__attribute__((amdgpu_flat_work_group_size(, )))`` attribute for 
the
 AMDGPU target. This attribute may be attached to a kernel function definition
-and is an optimization hint.
+and is an optimization hint. It is mandatory to use this attribute in some 
+situations. Because when the attribute is absent, the compiler assumes the 
+default maximum workgroup size of 256 but currently the workgroup size can 
legally go 
+to 1024. 
 
  parameter specifies the minimum flat work-group size, and 
 parameter specifies the maximum flat work-group size (must be greater than


Index: clang/include/clang/Basic/AttrDocs.td
===
--- clang/include/clang/Basic/AttrDocs.td
+++ clang/include/clang/Basic/AttrDocs.td
@@ -2241,7 +2241,10 @@
 Clang supports the
 ``__attribute__((amdgpu_flat_work_group_size(, )))`` attribute for the
 AMDGPU target. This attribute may be attached to a kernel function definition
-and is an optimization hint.
+and is an optimization hint. It is mandatory to use this attribute in some 
+situations. Because when the attribute is absent, the compiler assumes the 
+default maximum workgroup size of 256 but currently the workgroup size can legally go 
+to 1024. 
 
  parameter specifies the minimum flat work-group size, and 
 parameter specifies the maximum flat work-group size (must be greater than
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D102134: [docs]Updated the AMD GPU Attributes documentation

[PoojaYadav via Phabricator via cfe-commits]

pooja2299 updated this revision to Diff 343920.
pooja2299 added a comment.

Made some corrections


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D102134/new/

https://reviews.llvm.org/D102134

Files:
  clang/include/clang/Basic/AttrDocs.td


Index: clang/include/clang/Basic/AttrDocs.td
===
--- clang/include/clang/Basic/AttrDocs.td
+++ clang/include/clang/Basic/AttrDocs.td
@@ -2241,7 +2241,10 @@
 Clang supports the
 ``__attribute__((amdgpu_flat_work_group_size(, )))`` attribute for 
the
 AMDGPU target. This attribute may be attached to a kernel function definition
-and is an optimization hint.
+and is an optimization hint. It is mandatory to use this attribute in some 
+situations. Because when the attribute is absent, the compiler assumes the 
+default maximum workgroup size of 256 but nowadays the workgroup size can 
legally go 
+to 1024. 
 
  parameter specifies the minimum flat work-group size, and 
 parameter specifies the maximum flat work-group size (must be greater than


Index: clang/include/clang/Basic/AttrDocs.td
===
--- clang/include/clang/Basic/AttrDocs.td
+++ clang/include/clang/Basic/AttrDocs.td
@@ -2241,7 +2241,10 @@
 Clang supports the
 ``__attribute__((amdgpu_flat_work_group_size(, )))`` attribute for the
 AMDGPU target. This attribute may be attached to a kernel function definition
-and is an optimization hint.
+and is an optimization hint. It is mandatory to use this attribute in some 
+situations. Because when the attribute is absent, the compiler assumes the 
+default maximum workgroup size of 256 but nowadays the workgroup size can legally go 
+to 1024. 
 
  parameter specifies the minimum flat work-group size, and 
 parameter specifies the maximum flat work-group size (must be greater than
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D102134: [docs]Updated the AMD GPU Attributes documentation

[PoojaYadav via Phabricator via cfe-commits]

pooja2299 added a comment.

In D102134#2747649 , @aaron.ballman 
wrote:

> 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 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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D102134/new/

https://reviews.llvm.org/D102134

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits