[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/AttrDocs.td:1502-1503
+    evaluated at compile-time;
+  - A dependent integral value which depends on one or more template arguments,
+    be they type or non-type e.g. ``sizeof(T)`` within the scope of
+    ``template<typename T> struct S;``.
----------------
Slight wording tweak:

"A type or non-type dependent integral value..."

drop the "be they type or non-type"

Same comment applies below.


================
Comment at: lib/CodeGen/TargetInfo.cpp:7665
+static llvm::APSInt getConstexprInt(const Expr *E, const ASTContext& Ctx)
+{
+  assert(E);
----------------
The curly brace should bind to the end of the function parameter list. I'd just run the whole patch through clang-format and then fix up anything that goes wrong in the .td files.


================
Comment at: lib/CodeGen/TargetInfo.cpp:7669
+  llvm::APSInt Tmp{32, 0};
+  E->EvaluateAsInt(Tmp, Ctx);
+
----------------
Are you expecting this to always succeed, or are you relying on the initialized value in the case where this fails?


================
Comment at: lib/Sema/SemaDeclAttr.cpp:5473
+static bool checkAllAreIntegral(Sema &S, const AttributeList &Attr) {
+  for (auto i = 0u; i != Attr.getNumArgs(); ++i) {
+    Expr* E = Attr.getArgAsExpr(i);
----------------
Don't use `auto` here either, just use `unsigned`. Also `i` doesn't match our coding standard.


================
Comment at: lib/Sema/SemaDeclAttr.cpp:5474
+  for (auto i = 0u; i != Attr.getNumArgs(); ++i) {
+    Expr* E = Attr.getArgAsExpr(i);
+    if (E && !E->getType()->isIntegralOrEnumerationType()) {
----------------
* binds to `E` not to `Expr`


================
Comment at: lib/Sema/SemaDeclAttr.cpp:5484
+
+  return true;
+}
----------------
This still returns `true` even if `E` is null; is that intended?


https://reviews.llvm.org/D39857