[PATCH] D39857: [AMDGPU] Late parsed / dependent arguments for AMDGPU kernel attributes

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.


================
Comment at: include/clang/Basic/Attr.td:1329
               StringArgument<"ISA", 1>];
   let Documentation = [AMDGPUFlatWorkGroupSizeDocs];
   let Subjects = SubjectList<[Function], ErrorDiag, "ExpectedKernelFunction">;
----------------
Please ensure that the attribute documentation properly reflects the changes you've made here. At the very least, I would expect the docs to mention that these can now be in dependent contexts. 

The same applies for the other attributes.


================
Comment at: include/clang/Basic/Attr.td:1368
   let Subjects = SubjectList<[Function], ErrorDiag>;
   let Documentation = [Undocumented];
+  let TemplateDependent = 1;
----------------
Since you're changing the definition of the attribute, it would also be nice to add some documentation to AttrDocs.td for this.


================
Comment at: lib/CodeGen/TargetInfo.cpp:7665-7666
 
+namespace
+{
+  inline
----------------
This should be a static function rather than in an inline namespace.


================
Comment at: lib/CodeGen/TargetInfo.cpp:7671
+    llvm::APSInt r{32, 0};
+    if (E) E->EvaluateAsInt(r, Ctx);
+
----------------
If there is no expression given, why should this return an `APSInt` for 0? This seems more like something you would assert.


================
Comment at: lib/Sema/SemaDeclAttr.cpp:5583-5584
 
+namespace
+{
+  inline
----------------
static function instead, please.


================
Comment at: lib/Sema/SemaDeclAttr.cpp:5586
+  inline
+  bool checkAllAreIntegral(const AttributeList &Attr, Sema &S) {
+    for (auto i = 0u; i != Attr.getNumArgs(); ++i) {
----------------
We usually put the `Sema` object first in the parameter list in this file, don't we?


================
Comment at: lib/Sema/SemaDeclAttr.cpp:5587-5588
+  bool checkAllAreIntegral(const AttributeList &Attr, Sema &S) {
+    for (auto i = 0u; i != Attr.getNumArgs(); ++i) {
+      auto e = Attr.getArgAsExpr(i);
+      if (e && !e->getType()->isIntegralOrEnumerationType()) {
----------------
These variables should not be lower-case per our usual coding conventions.


================
Comment at: lib/Sema/SemaDeclAttr.cpp:5588
+    for (auto i = 0u; i != Attr.getNumArgs(); ++i) {
+      auto e = Attr.getArgAsExpr(i);
+      if (e && !e->getType()->isIntegralOrEnumerationType()) {
----------------
Please spell the type out explicitly.


https://reviews.llvm.org/D39857