[PATCH] D17764: Add attributes for AMD GPU Tools

[Aaron Ballman via cfe-commits]

aaron.ballman added a comment.

Some quick thoughts below.


================
Comment at: include/clang/Basic/Attr.td:997
@@ +996,3 @@
+  let Subjects = SubjectList<[Function], ErrorDiag,
+                             "ExpectedKernelFunction">;
+}
----------------
I don't see any code that checks whether the function is actually a kernel function, nor sema tests that diagnose if this is attached to a non-kernel function.

It may make sense to make a SubsetSubject that does the checking for you, and use that in all of these attributes.

================
Comment at: include/clang/Basic/AttrDocs.td:943
@@ +942,3 @@
+
+Clang supports following attributes for tools, such as debugger and profiler:
+  }];
----------------
This reads a bit strangely, can it be "debuggers" and "profilers" (plural)?

================
Comment at: include/clang/Basic/AttrDocs.td:951
@@ +950,3 @@
+
+Clang supports ``__attribute__((amdgpu_tools_insert_nops))`` attribute on AMD
+Southern Islands GPUs and later. If specified, causes AMD GPU Backend to insert
----------------
"supports the `__attribute__`" (insert the).

================
Comment at: include/clang/Basic/AttrDocs.td:952
@@ +951,3 @@
+Clang supports ``__attribute__((amdgpu_tools_insert_nops))`` attribute on AMD
+Southern Islands GPUs and later. If specified, causes AMD GPU Backend to insert
+two nop instructions for each high level source statement: one nop instruction
----------------
"it causes" (insert it).

================
Comment at: include/clang/Basic/AttrDocs.td:954
@@ +953,3 @@
+two nop instructions for each high level source statement: one nop instruction
+is inserted before first isa instruction of high level source statement, and one
+nop instruction is inserted after last isa instruction of high level source
----------------
"before the first ISA" (insert the, capitalize acronym).
"of the high level" (insert the).

================
Comment at: include/clang/Basic/AttrDocs.td:955
@@ +954,3 @@
+is inserted before first isa instruction of high level source statement, and one
+nop instruction is inserted after last isa instruction of high level source
+statement.
----------------
"after the last ISA" (insert the, capitalize acronym).
"of the high level" (insert the).

================
Comment at: include/clang/Basic/AttrDocs.td:959
@@ +958,3 @@
+In addition to specifying this attribute manually, clang can add this attribute
+for each kernel function in module if ``--amdgpu-tools-insert-nops`` clang
+command line option is specified.
----------------
"function in module" -> "function in the translation unit"? At least insert "the", but uncertain what you mean by "module", which I assume to be a translation unit.

Also add "the" before the command line option.

================
Comment at: include/clang/Basic/AttrDocs.td:964
@@ +963,3 @@
+
+def AMDGPUToolsNumReservedVGPRDocs : Documentation {
+  let Category = DocCatAMDGPUToolsAttributes;
----------------
Same sentence comments apply to the following attributes as above.

================
Comment at: test/SemaOpenCL/amdgpu-tools-attrs.cl:5
@@ +4,3 @@
+typedef __attribute__((amdgpu_tools_insert_nops)) struct foo0_s { // expected-error {{'amdgpu_tools_insert_nops' attribute only applies to kernel functions}}
+  int x;
+  int y;
----------------
You can make the struct empty in these tests, but more importantly, it would be good to see a test that ensures the following diagnoses:
```
__attribute__((amdgpu_tools_insert_nops)) void not_a_kernel_func() {}
```


http://reviews.llvm.org/D17764