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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman requested changes to this revision.
aaron.ballman added a comment.
This revision now requires changes to proceed.

Please add test cases for this new functionality.



================
Comment at: lib/CodeGen/TargetInfo.cpp:7665
+static llvm::APSInt getConstexprInt(const Expr *E, const ASTContext& Ctx) {
+  assert(E);
+
----------------
You should add a message to your assertion in case it is triggered. e.g., `assert(E && "attempting to get a constexpr int out of a null Expr");` or something along those lines.


================
Comment at: lib/CodeGen/TargetInfo.cpp:7665
+static llvm::APSInt getConstexprInt(const Expr *E, const ASTContext& Ctx)
+{
+  assert(E);
----------------
aaron.ballman wrote:
> 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.
This is still a bit off -- the `&` should bind to `Ctx`.


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


================
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);
----------------
aaron.ballman wrote:
> Don't use `auto` here either, just use `unsigned`. Also `i` doesn't match our coding standard.
Only partially fixed; the naming convention is still not being followed.


https://reviews.llvm.org/D39857