[PATCH] D17764: Add attributes for AMD GPU Tools

[Konstantin Zhuravlyov via cfe-commits]

kzhuravl-AMD marked 10 inline comments as done.
kzhuravl-AMD added a comment.

Review Feedback


================
Comment at: include/clang/Basic/Attr.td:993-998
@@ +992,8 @@
+
+def AMDGPUToolsInsertNops : InheritableAttr {
+  let Spellings = [GNU<"amdgpu_tools_insert_nops">];
+  let Documentation = [AMDGPUToolsInsertNopsDocs];
+  let Subjects = SubjectList<[Function], ErrorDiag,
+                             "ExpectedKernelFunction">;
+}
+
----------------
arsenm wrote:
> I didn't envision these as being user code facing attributes, and only the function emission would add them. Is there a use for these being used manually?
Tools team expressed a desire to have the ability to specify them manually as well.

================
Comment at: include/clang/Driver/Options.td:355-365
@@ -354,2 +354,13 @@
 def allowable__client : Separate<["-"], "allowable_client">;
+def amdgpu_tools_insert_nops :
+  Flag<["--"], "amdgpu-tools-insert-nops">, Flags<[CC1Option, HelpHidden]>,
+  HelpText<"Insert two nop instructions for each high level source statement">;
+def amdgpu_tools_num_reserved_vgpr :
+  Joined<["--"], "amdgpu-tools-num-reserved-vgpr=">,
+  Flags<[CC1Option, HelpHidden]>,
+  HelpText<"Reserve <num> vector registers">, MetaVarName<"<num>">;
+def amdgpu_tools_num_reserved_sgpr :
+  Joined<["--"], "amdgpu-tools-num-reserved-sgpr=">,
+  Flags<[CC1Option, HelpHidden]>,
+  HelpText<"Reserve <num> scalar registers">, MetaVarName<"<num>">;
 def ansi : Flag<["-", "--"], "ansi">;
----------------
arsenm wrote:
> These are user facing options, not cc1 flags? I wouldn't expect these to be exposed to users, and they would just be implied by -g. Is there a need for this? Even if there is some need for these, I don't see anything testing for these with only -g
These should be independent of -g, user facing options. One of the use cases is profiler, where debug information is needed (-g), but no registers need to be reserved, no nops inserted.

================
Comment at: lib/CodeGen/CGCall.cpp:1601
@@ +1600,3 @@
+    if (CodeGenOpts.AMDGPUToolsInsertNopsOpt)
+      FuncAttrs.addAttribute("amdgpu_tools_insert_nops");
+    if (CodeGenOpts.AMDGPUToolsNumReservedVGPROpt)
----------------
arsenm wrote:
> I've been trying to switch to consistently using the '-' separates words conventions most function attributes use.
Do you mean use '-' instead of '_' in attribute spellings?


http://reviews.llvm.org/D17764