[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

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">];
   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];
 }
 
 // pass_object_size(N) indicates that the parameter should have
 // __builtin_object_size with Type=N evaluated on the parameter at the callsite.
 def PassObjectSize : InheritableParamAttr {
   let Spellings = [GNU<"pass_object_size">];
   let Args = [IntArgument<"Type">];

include/clang/Basic/AttrDocs.td

 Clang supports the
 ``__attribute__((amdgpu_flat_work_group_size(<min>, <max>)))`` attribute for the
 AMDGPU target. This attribute may be attached to a kernel function definition
 and is an optimization hint.
 
 ``<min>`` parameter specifies the minimum flat work-group size, and ``<max>``
 parameter specifies the maximum flat work-group size (must be greater than
 ``<min>``) to which all dispatches of the kernel will conform. Passing ``0, 0``
-as ``<min>, <max>`` implies the default behavior (``128, 256``).
+as ``<min>, <max>`` implies the default behavior (``128, 256``).  Either
+parameter can be:
+  - An integral literal value e.g. ``1, 64, 128`` etc.;
+  - An integral value returned by a ``constexpr`` function which can be
+    evaluated at compile-time;
+  - A type or non-type dependent integral value, e.g. ``sizeof(T)`` within the
+    scope of ``template<typename T> struct S;``.
 
 If specified, the AMDGPU target backend might be able to produce better machine
 code for barriers and perform scratch promotion by estimating available group
 segment size.
 
 An error will be given if:
   - Specified values violate subtarget specifications;
   - Specified values are not compatible with values provided through other

 attribute for the AMDGPU target. This attribute may be attached to a kernel
 function definition and is an optimization hint.
 
 ``<min>`` parameter specifies the requested minimum number of waves per EU, and
 *optional* ``<max>`` parameter specifies the requested maximum number of waves
 per EU (must be greater than ``<min>`` if specified). If ``<max>`` is omitted,
 then there is no restriction on the maximum number of waves per EU other than
 the one dictated by the hardware for which the kernel is compiled. Passing
-``0, 0`` as ``<min>, <max>`` implies the default behavior (no limits).
+``0, 0`` as ``<min>, <max>`` implies the default behavior (no limits). Either
+parameter can be:
+  - An integral literal value e.g. ``1, 64, 128`` etc.;
+  - An integral value returned by a ``constexpr`` function which can be
+    evaluated at compile-time;
+  - A type or non-type dependent integral value, e.g. ``sizeof(T)`` within the
+    scope of ``template<typename T> struct S;``.
 
 If specified, this attribute allows an advanced developer to tune the number of
 wavefronts that are capable of fitting within the resources of an EU. The AMDGPU
 target backend can use this information to limit resources, such as number of
 SGPRs, number of VGPRs, size of available group and private memory segments, in
 such a way that guarantees that at least ``<min>`` wavefronts and at most
 ``<max>`` wavefronts are able to fit within the resources of an EU. Requesting
 more wavefronts can hide memory latency but limits available registers which

 ``__attribute__((amdgpu_num_vgpr(<num_vgpr>)))`` attributes for the AMDGPU
 target. These attributes may be attached to a kernel function definition and are
 an optimization hint.
 
 If these attributes are specified, then the AMDGPU target backend will attempt
 to limit the number of SGPRs and/or VGPRs used to the specified value(s). The
 number of used SGPRs and/or VGPRs may further be rounded up to satisfy the
 allocation requirements or constraints of the subtarget. Passing ``0`` as
-``num_sgpr`` and/or ``num_vgpr`` implies the default behavior (no limits).
+``num_sgpr`` and/or ``num_vgpr`` implies the default behavior (no limits). For
+both attributes, the parameter can be:
+  - An integral literal value e.g. ``1, 64, 128`` etc.;
+  - An integral value returned by a ``constexpr`` function which can be
+    evaluated at compile-time;
+  - A type or non-type dependent integral value, e.g. ``sizeof(T)`` within the
+    scope of ``template<typename T> struct S;``.
 
 These attributes can be used to test the AMDGPU target backend. It is
 recommended that the ``amdgpu_waves_per_eu`` attribute be used to control
 resources such as SGPRs and VGPRs since it is aware of the limits for different
 subtargets.
 
 An error will be given if:
   - Specified values violate subtarget specifications;

lib/CodeGen/TargetInfo.cpp

                                          llvm::LLVMContext &C) const override;
   llvm::Function *
   createEnqueuedBlockKernel(CodeGenFunction &CGF,
                             llvm::Function *BlockInvokeFunc,
                             llvm::Value *BlockLiteral) const override;
 };
 }
 
+static llvm::APSInt getConstexprInt(const Expr *E, const ASTContext& Ctx) {
+  assert(E);
+
+  llvm::APSInt Tmp{32, 0};
+  E->EvaluateAsInt(Tmp, Ctx);
+
+  return Tmp;
+}
+
 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 KMin = getConstexprInt(FlatWGS->getMin(), FD->getASTContext());
+    llvm::APSInt KMax = getConstexprInt(FlatWGS->getMax(), FD->getASTContext());
+
+    unsigned Min = KMin.getZExtValue();
+    unsigned Max = std::max(KMin, KMax).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 KMin = getConstexprInt(Attr->getMin(), FD->getASTContext());
+    llvm::APSInt KMax = getConstexprInt(Attr->getMax(), FD->getASTContext());
+
+    unsigned Min = KMin.getZExtValue();
+    unsigned Max = std::max(KMin, KMax).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;
   }
 }
 
+static bool checkAllAreIntegral(Sema &S, const AttributeList &Attr) {
+  for (unsigned i = 0u; i != Attr.getNumArgs(); ++i) {
+    Expr *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;
+  uint32_t Max = Min;
   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(S, Attr))
+    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;
+  uint32_t Max = Min;
+  Expr *MaxExpr = MinExpr;
   if (Attr.getNumArgs() == 2) {
     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 (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(S, Attr))
+    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(S, Attr))
+    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;
   // Also don't warn on function pointer typedefs.
   TypedefNameDecl *TD = dyn_cast<TypedefNameDecl>(D);
   if (TD && (TD->getUnderlyingType()->isFunctionPointerType() ||