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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
