[AMDGPU] Late parsed / dependent arguments for AMDGPU kernel attributes

[AlexVlx]

Summary

This generalises AMDGPU kernel attributes by changing their
definition to accept possibly late-parsed / dependent expressions, and
not only literal values of integral type. This enables one to use, for example,
template meta-programming or constexpr functions to generate an
attribute value, based on compile-time information such as the size of a
type or a nested compiled-time constant property. This brings parity with
launch_bounds as implemented in CUDA.

[kzhuravl]

Hi Alex, can you rebase on top of trunk (I think you brought in some extra changes from hcc branch) and upload a full diff?

include/clang/Basic/Attr.td

1328

Not in trunk.

1339

Not in trunk.

1361–1370

Not in trunk.

[aaron.ballman]

include/clang/Basic/Attr.td

1329

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.

1368

Since you're changing the definition of the attribute, it would also be nice to add some documentation to AttrDocs.td for this.

lib/CodeGen/TargetInfo.cpp

7665–7666

This should be a static function rather than in an inline namespace.

7671

If there is no expression given, why should this return an APSInt for 0? This seems more like something you would assert.

lib/Sema/SemaDeclAttr.cpp

5583–5584

static function instead, please.

5586

We usually put the Sema object first in the parameter list in this file, don't we?

5587–5588

These variables should not be lower-case per our usual coding conventions.

5588

Please spell the type out explicitly.

[AlexVlx]

include/clang/Basic/Attr.td

1328

Done (hopefully). Apologies for that.

1339

Done (hopefully). Apologies for that.

1361–1370

Done (hopefully). Apologies for that.

lib/CodeGen/TargetInfo.cpp

7665–7666

Done.

7671

Hello Aaron, thank you for your comment. The initial premise was that it could be possible for a pass null, for cases where an user would not have specified an optional argument (e.g. for MaxWavesPerEU). This would've put the actual check further away from the actual attribute handling logic, making it somewhat tidier. However, thinking about it some more, from the specification of the attributes where this would have applied this can be done upstream in such a way as to ensure that all expressions describing the arguments passed to an attribute are non-null, so I've switched it over to follow your suggestion.

[AlexVlx]

In D39857#920814, @kzhuravl wrote:

Hi Alex, can you rebase on top of trunk (I think you brought in some extra changes from hcc branch) and upload a full diff?

Hello Konstantine - apologies about that, hopefully it's correct now. Thank you.

[aaron.ballman]

include/clang/Basic/AttrDocs.td

1502–1503 ↗

(On Diff #122327)

Slight wording tweak:

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

drop the "be they type or non-type"

Same comment applies below.

lib/CodeGen/TargetInfo.cpp

7665

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.

7669

Are you expecting this to always succeed, or are you relying on the initialized value in the case where this fails?

lib/Sema/SemaDeclAttr.cpp

5473

Don't use auto here either, just use unsigned. Also i doesn't match our coding standard.

5474

• binds to E not to Expr

5484

This still returns true even if E is null; is that intended?

[AlexVlx]

Apologies for the delayed update. The new version tries to deal with two issues:

1. the non-compliant style identified by Aaron in various spots, hopefully this is correct now;
2. there was an issue with the existing logic, which only came up with additional testing:
   • for AMDGPUFlatWorkGroupSizeAttr and AMDGPUWavesPerEU, it is valid for the user to have passed an integer literal for the minimum and a dependent expression for the maximum;
   • in this case, iff the expression for the maximum could not be evaluated in Sema, then a spurious error case would obtain in which the minimum would signalled as greater than the maximum, which would have been pegged at 0;
   • this is fixed by having the maximum be at least equal to the minimum before any attempt of evaluating it is made - iff the actual user passed arguments to the attribute violate its invariants the error is signalled.
   • the relevant changes are on lines 5495 and 5529 in SemaDeclAttr.cpp.

[aaron.ballman]

Please add test cases for this new functionality.

lib/CodeGen/TargetInfo.cpp

7665

This is still a bit off -- the & should bind to Ctx.

7665

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.

7669

This question still applies.

lib/Sema/SemaDeclAttr.cpp

5473

Only partially fixed; the naming convention is still not being followed.

include/clang/Basic/Attr.td

 // use this in CUDA, since CUDA does not use the same terminology.
 //
 // FIXME: SubjectList should be for OpenCLKernelFunction, but is not to
 // workaround needing to see kernel attribute before others to know if
 // this should be rejected on non-kernels.
 
 def AMDGPUFlatWorkGroupSize : InheritableAttr {
   let Spellings = [GNU<"amdgpu_flat_work_group_size">];
-  let Args = [UnsignedArgument<"Min">, UnsignedArgument<"Max">];
+  let Args = [ExprArgument<"Min">, ExprArgument<"Max", 1>];
   let Documentation = [AMDGPUFlatWorkGroupSizeDocs];
   let Subjects = SubjectList<[Function], ErrorDiag, "ExpectedKernelFunction">;
+  let TemplateDependent = 1;
 }
 
 def AMDGPUWavesPerEU : InheritableAttr {
   let Spellings = [GNU<"amdgpu_waves_per_eu">];
-  let Args = [UnsignedArgument<"Min">, UnsignedArgument<"Max", 1>];
+  let Args = [ExprArgument<"Min">, ExprArgument<"Max", 1>];
   let Documentation = [AMDGPUWavesPerEUDocs];
   let Subjects = SubjectList<[Function], ErrorDiag, "ExpectedKernelFunction">;
+  let TemplateDependent = 1;
 }
 
 def AMDGPUNumSGPR : InheritableAttr {
   let Spellings = [GNU<"amdgpu_num_sgpr">];
-  let Args = [UnsignedArgument<"NumSGPR">];
+  let Args = [ExprArgument<"NumSGPR">];
   let Documentation = [AMDGPUNumSGPRNumVGPRDocs];
   let Subjects = SubjectList<[Function], ErrorDiag, "ExpectedKernelFunction">;
+  let TemplateDependent = 1;
 }
 
 def AMDGPUNumVGPR : InheritableAttr {
   let Spellings = [GNU<"amdgpu_num_vgpr">];
-  let Args = [UnsignedArgument<"NumVGPR">];
+  let Args = [ExprArgument<"NumVGPR">];
   let Documentation = [AMDGPUNumSGPRNumVGPRDocs];
   let Subjects = SubjectList<[Function], ErrorDiag, "ExpectedKernelFunction">;
+  let TemplateDependent = 1;
 }
 
 def NoSplitStack : InheritableAttr {
   let Spellings = [GCC<"no_split_stack">];
   let Subjects = SubjectList<[Function], ErrorDiag>;
   let Documentation = [NoSplitStackDocs];
 }
 
 def NonNull : InheritableParamAttr {
   let Spellings = [GCC<"nonnull">];
   let Subjects = SubjectList<[ObjCMethod, HasFunctionProto, ParmVar], WarnDiag,
                              "ExpectedFunctionMethodOrParameter">;
   let Args = [VariadicUnsignedArgument<"Args">];
   let AdditionalMembers =
 [{bool isNonNull(unsigned idx) const {
     if (!args_size())
       return true;
     for (const auto &V : args())
       if (V == idx)
         return true;
     return false;
   } }];
   // FIXME: We should merge duplicates into a single nonnull attribute.
   let DuplicatesAllowedWhileMerging = 1;
   let Documentation = [NonNullDocs];
 }
 
 def ReturnsNonNull : InheritableAttr {
   let Spellings = [GCC<"returns_nonnull">];
   let Subjects = SubjectList<[ObjCMethod, Function], WarnDiag,
                              "ExpectedFunctionOrMethod">;
   let Documentation = [ReturnsNonNullDocs];

lib/CodeGen/TargetInfo.cpp

                                          llvm::LLVMContext &C) const override;
   llvm::Function *
   createEnqueuedBlockKernel(CodeGenFunction &CGF,
                             llvm::Function *BlockInvokeFunc,
                             llvm::Value *BlockLiteral) const override;
 };
 }
 
+namespace
+{
+  inline
+  llvm::APSInt getConstexprInt(const Expr *E, const ASTContext& Ctx)
+  {
+    llvm::APSInt r{32, 0};
+    if (E) E->EvaluateAsInt(r, Ctx);
+
+    return r;
+  }
+}
+
 void AMDGPUTargetCodeGenInfo::setTargetAttributes(
     const Decl *D, llvm::GlobalValue *GV, CodeGen::CodeGenModule &M,
     ForDefinition_t IsForDefinition) const {
   if (!IsForDefinition)
     return;
   const FunctionDecl *FD = dyn_cast_or_null<FunctionDecl>(D);
   if (!FD)
     return;
 
   llvm::Function *F = cast<llvm::Function>(GV);
 
   const auto *ReqdWGS = M.getLangOpts().OpenCL ?
     FD->getAttr<ReqdWorkGroupSizeAttr>() : nullptr;
   const auto *FlatWGS = FD->getAttr<AMDGPUFlatWorkGroupSizeAttr>();
   if (ReqdWGS || FlatWGS) {
-    unsigned Min = FlatWGS ? FlatWGS->getMin() : 0;
-    unsigned Max = FlatWGS ? FlatWGS->getMax() : 0;
+    llvm::APSInt min = getConstexprInt(FlatWGS->getMin(), FD->getASTContext());
+    llvm::APSInt max = getConstexprInt(FlatWGS->getMax(), FD->getASTContext());
+
+    unsigned Min = min.getZExtValue();
+    unsigned Max = std::max(min, max).getZExtValue();
     if (ReqdWGS && Min == 0 && Max == 0)
       Min = Max = ReqdWGS->getXDim() * ReqdWGS->getYDim() * ReqdWGS->getZDim();
 
     if (Min != 0) {
       assert(Min <= Max && "Min must be less than or equal Max");
 
       std::string AttrVal = llvm::utostr(Min) + "," + llvm::utostr(Max);
       F->addFnAttr("amdgpu-flat-work-group-size", AttrVal);
     } else
       assert(Max == 0 && "Max must be zero");
   }
 
   if (const auto *Attr = FD->getAttr<AMDGPUWavesPerEUAttr>()) {
-    unsigned Min = Attr->getMin();
-    unsigned Max = Attr->getMax();
+    llvm::APSInt min = getConstexprInt(Attr->getMin(), FD->getASTContext());
+    llvm::APSInt max = getConstexprInt(Attr->getMax(), FD->getASTContext());
+
+    Attr->getMin()->dump();
+    Attr->getMax()->dump();
+    unsigned Min = min.getZExtValue();
+    unsigned Max = std::max(min, max).getZExtValue();
 
     if (Min != 0) {
       assert((Max == 0 || Min <= Max) && "Min must be less than or equal Max");
 
       std::string AttrVal = llvm::utostr(Min);
       if (Max != 0)
         AttrVal = AttrVal + "," + llvm::utostr(Max);
       F->addFnAttr("amdgpu-waves-per-eu", AttrVal);
     } else
       assert(Max == 0 && "Max must be zero");
   }
 
   if (const auto *Attr = FD->getAttr<AMDGPUNumSGPRAttr>()) {
-    unsigned NumSGPR = Attr->getNumSGPR();
+    llvm::APSInt sgprs =
+      getConstexprInt(Attr->getNumSGPR(), FD->getASTContext());
+    unsigned NumSGPR = sgprs.getZExtValue();
 
     if (NumSGPR != 0)
       F->addFnAttr("amdgpu-num-sgpr", llvm::utostr(NumSGPR));
   }
 
   if (const auto *Attr = FD->getAttr<AMDGPUNumVGPRAttr>()) {
-    uint32_t NumVGPR = Attr->getNumVGPR();
+    llvm::APSInt vgprs =
+      getConstexprInt(Attr->getNumVGPR(), FD->getASTContext());
+    unsigned NumVGPR = vgprs.getZExtValue();
 
     if (NumVGPR != 0)
       F->addFnAttr("amdgpu-num-vgpr", llvm::utostr(NumVGPR));
   }
 }
 
 unsigned AMDGPUTargetCodeGenInfo::getOpenCLKernelCallingConv() const {
   return llvm::CallingConv::AMDGPU_KERNEL;

lib/Sema/SemaDeclAttr.cpp

     handleAVRInterruptAttr(S, D, Attr);
     break;
   default:
     handleARMInterruptAttr(S, D, Attr);
     break;
   }
 }
 
+namespace
+{
+  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()) {
+        S.Diag(getAttrLoc(Attr), diag::err_attribute_argument_n_type)
+          << getAttrName(Attr) << i << AANT_ArgumentIntegerConstant
+          << e->getSourceRange();
+
+        return false;
+      }
+    }
+
+    return true;
+  }
+}
+
 static void handleAMDGPUFlatWorkGroupSizeAttr(Sema &S, Decl *D,
                                               const AttributeList &Attr) {
   uint32_t Min = 0;
   Expr *MinExpr = Attr.getArgAsExpr(0);
-  if (!checkUInt32Argument(S, Attr, MinExpr, Min))
+  if (MinExpr->isEvaluatable(S.Context) &&
+      !checkUInt32Argument(S, Attr, MinExpr, Min))
     return;
 
   uint32_t Max = 0;
   Expr *MaxExpr = Attr.getArgAsExpr(1);
-  if (!checkUInt32Argument(S, Attr, MaxExpr, Max))
+  if (MaxExpr->isEvaluatable(S.Context) &&
+      !checkUInt32Argument(S, Attr, MaxExpr, Max))
     return;
 
   if (Min == 0 && Max != 0) {
     S.Diag(Attr.getLoc(), diag::err_attribute_argument_invalid)
       << Attr.getName() << 0;
     return;
   }
   if (Min > Max) {
     S.Diag(Attr.getLoc(), diag::err_attribute_argument_invalid)
       << Attr.getName() << 1;
     return;
   }
 
   D->addAttr(::new (S.Context)
-             AMDGPUFlatWorkGroupSizeAttr(Attr.getLoc(), S.Context, Min, Max,
-                                         Attr.getAttributeSpellingListIndex()));
+             AMDGPUFlatWorkGroupSizeAttr(
+               Attr.getLoc(), S.Context, MinExpr, MaxExpr,
+               Attr.getAttributeSpellingListIndex()));
 }
 
 static void handleAMDGPUWavesPerEUAttr(Sema &S, Decl *D,
                                        const AttributeList &Attr) {
+  if (!checkAllAreIntegral(Attr, S))
+    return;
+
   uint32_t Min = 0;
   Expr *MinExpr = Attr.getArgAsExpr(0);
-  if (!checkUInt32Argument(S, Attr, MinExpr, Min))
+  if (MinExpr->isEvaluatable(S.Context) &&
+      !checkUInt32Argument(S, Attr, MinExpr, Min))
     return;
 
   uint32_t Max = 0;
+  Expr *MaxExpr = MinExpr;
   if (Attr.getNumArgs() == 2) {
-    Expr *MaxExpr = Attr.getArgAsExpr(1);
-    if (!checkUInt32Argument(S, Attr, MaxExpr, Max))
+    MaxExpr = Attr.getArgAsExpr(1);
+    if (MaxExpr->isEvaluatable(S.Context) &&
+        !checkUInt32Argument(S, Attr, MaxExpr, Max))
       return;
   }
 
   if (Min == 0 && Max != 0) {
     S.Diag(Attr.getLoc(), diag::err_attribute_argument_invalid)
       << Attr.getName() << 0;
     return;
   }
   if (Max != 0 && Min > Max) {
     S.Diag(Attr.getLoc(), diag::err_attribute_argument_invalid)
       << Attr.getName() << 1;
     return;
   }
 
   D->addAttr(::new (S.Context)
-             AMDGPUWavesPerEUAttr(Attr.getLoc(), S.Context, Min, Max,
+             AMDGPUWavesPerEUAttr(Attr.getLoc(), S.Context, MinExpr, MaxExpr,
                                   Attr.getAttributeSpellingListIndex()));
 }
 
 static void handleAMDGPUNumSGPRAttr(Sema &S, Decl *D,
                                     const AttributeList &Attr) {
+  if (!checkAllAreIntegral(Attr, S))
+    return;
+
   uint32_t NumSGPR = 0;
   Expr *NumSGPRExpr = Attr.getArgAsExpr(0);
-  if (!checkUInt32Argument(S, Attr, NumSGPRExpr, NumSGPR))
+  if (NumSGPRExpr->isEvaluatable(S.Context) &&
+      !checkUInt32Argument(S, Attr, NumSGPRExpr, NumSGPR))
     return;
 
   D->addAttr(::new (S.Context)
-             AMDGPUNumSGPRAttr(Attr.getLoc(), S.Context, NumSGPR,
+             AMDGPUNumSGPRAttr(Attr.getLoc(), S.Context, NumSGPRExpr,
                                Attr.getAttributeSpellingListIndex()));
 }
 
 static void handleAMDGPUNumVGPRAttr(Sema &S, Decl *D,
                                     const AttributeList &Attr) {
+  if (!checkAllAreIntegral(Attr, S))
+    return;
+
   uint32_t NumVGPR = 0;
   Expr *NumVGPRExpr = Attr.getArgAsExpr(0);
-  if (!checkUInt32Argument(S, Attr, NumVGPRExpr, NumVGPR))
+  if (NumVGPRExpr->isEvaluatable(S.Context) &&
+      !checkUInt32Argument(S, Attr, NumVGPRExpr, NumVGPR))
     return;
 
   D->addAttr(::new (S.Context)
-             AMDGPUNumVGPRAttr(Attr.getLoc(), S.Context, NumVGPR,
+             AMDGPUNumVGPRAttr(Attr.getLoc(), S.Context, NumVGPRExpr,
                                Attr.getAttributeSpellingListIndex()));
 }
 
 static void handleX86ForceAlignArgPointerAttr(Sema &S, Decl *D,
                                               const AttributeList& Attr) {
   // If we try to apply it to a function pointer, don't warn, but don't
   // do anything, either. It doesn't matter anyway, because there's nothing
   // special about calling a force_align_arg_pointer function.
   ValueDecl *VD = dyn_cast<ValueDecl>(D);
   if (VD && VD->getType()->isFunctionPointerType())
     return;