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

[Alex Voicu via Phabricator via cfe-commits]

AlexVlx marked 2 inline comments as done.
AlexVlx added inline comments.


================
Comment at: include/clang/Basic/Attr.td:1328
+              ExprArgument<"Max", 1>,
               StringArgument<"ISA", 1>];
   let Documentation = [AMDGPUFlatWorkGroupSizeDocs];
----------------
kzhuravl wrote:
> Not in trunk.
Done (hopefully). Apologies for that.


================
Comment at: include/clang/Basic/Attr.td:1339
+              ExprArgument<"Max", 1>,
               StringArgument<"ISA", 1>];
   let Documentation = [AMDGPUWavesPerEUDocs];
----------------
kzhuravl wrote:
> Not in trunk.
Done (hopefully). Apologies for that.


================
Comment at: include/clang/Basic/Attr.td:1361-1370
+def AMDGPUMaxWorkGroupDim : InheritableAttr {
   let Spellings = [CXX11<"","hc_max_workgroup_dim", 201511>];
-  let Args = [IntArgument<"X">,
-              IntArgument<"Y", 1>,
-              IntArgument<"Z", 1>,
+  let Args = [ExprArgument<"X">,
+              ExprArgument<"Y">,
+              ExprArgument<"Z">,
               StringArgument<"ISA", 1>];
   let Subjects = SubjectList<[Function], ErrorDiag>;
----------------
kzhuravl wrote:
> Not in trunk.
Done (hopefully). Apologies for that.


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


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


https://reviews.llvm.org/D39857