[PATCH] D24513: [AMDGPU] Expose flat work group size, register and wave control attributes

[Aaron Ballman via cfe-commits]

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

================
Comment at: include/clang/Basic/Attr.td:1054-1056
@@ -1058,3 +1053,5 @@
+//
 // FIXME: This 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.
+
----------------
This FIXME is now detached from what "this" is referring to. Can you clarify by mentioning that the Subjects list is what should be modified?

================
Comment at: include/clang/Basic/Attr.td:1067
@@ +1066,3 @@
+  let Spellings = [GNU<"amdgpu_waves_per_eu">];
+  let Args = [UnsignedArgument<"Min">, VariadicUnsignedArgument<"Max">];
+  let Documentation = [AMDGPUWavesPerEUDocs];
----------------
Looking at the documentation, are you sure this should be a `VariadicUnsignedArgument`? It seems like this should be an `UnsignedArgument` with the optional bit set. Or can you pass multiple Max values?

================
Comment at: include/clang/Basic/AttrDocs.td:1138
@@ +1137,3 @@
+``<max>`` wavefronts are able to fit within the resources of an EU. Requesting
+more wavefronts can  hide memory latency but limits available registers which
+can result in spilling. Requesting fewer wavefronts can help reduce cache
----------------
There's an extra space between "can" and "hide".

================
Comment at: include/clang/Basic/AttrDocs.td:1144
@@ +1143,3 @@
+to ensure it is capable of meeting the requested values. However, when the
+kernel is executed there may be other reasons that prevent meeting the request,
+for example, there may be wavefronts from other kernels executing on the EU.
----------------
Comma after "executed".

================
Comment at: include/clang/Basic/AttrDocs.td:1147
@@ +1146,3 @@
+
+The error will be given if:
+  - Specified values violate subtarget specifications;
----------------
An error instead of The error.

================
Comment at: include/clang/Basic/DiagnosticSemaKinds.td:2385
@@ -2384,1 +2384,3 @@
+def err_attribute_argument_invalid : Error<
+  "%0 attribute parameter is invalid: %1">;
 def err_attribute_argument_is_zero : Error<
----------------
I think this should say the attribute argument is invalid, rather than the attribute parameter.

================
Comment at: lib/CodeGen/TargetInfo.cpp:6958
@@ +6957,3 @@
+
+  if (const auto Attr = FD->getAttr<AMDGPUFlatWorkGroupSizeAttr>()) {
+    unsigned Min = Attr->getMin();
----------------
Should be `const auto *Attr`.

================
Comment at: lib/CodeGen/TargetInfo.cpp:6967-6969
@@ +6966,5 @@
+      F->addFnAttr("amdgpu-flat-work-group-size", AttrVal);
+    } else {
+      assert(Max == 0 && "Max must be zero");
+    }
+  }
----------------
Elide braces?

================
Comment at: lib/CodeGen/TargetInfo.cpp:6972
@@ +6971,3 @@
+
+  if (const auto Attr = FD->getAttr<AMDGPUWavesPerEUAttr>()) {
+    unsigned Min = Attr->getMin();
----------------
`const auto *` here as well.

================
Comment at: lib/CodeGen/TargetInfo.cpp:6983-6985
@@ -6961,1 +6982,5 @@
+      F->addFnAttr("amdgpu-waves-per-eu", AttrVal);
+    } else {
+      assert(Max == 0 && "Max must be zero");
+    }
   }
----------------
Elide braces?

================
Comment at: lib/CodeGen/TargetInfo.cpp:6995
@@ +6994,3 @@
+
+  if (const auto Attr = FD->getAttr<AMDGPUNumVGPRAttr>()) {
+    uint32_t NumVGPR = Attr->getNumVGPR();
----------------
`const auto *` here as well.

================
Comment at: lib/Sema/SemaDeclAttr.cpp:4947
@@ +4946,3 @@
+  uint32_t Min = 0;
+  Expr *MinExpr = static_cast<Expr*>(Attr.getArgAsExpr(0));
+  if (!checkUInt32Argument(S, Attr, MinExpr, Min))
----------------
I don't think this case is required (here, or elsewhere). `getArgAsExpr()` already returns an `Expr *`.

================
Comment at: lib/Sema/SemaDeclAttr.cpp:4959-4960
@@ +4958,4 @@
+      << Attr.getName()
+      << "maximum flat work-group size must be zero since minimum flat "
+         "work-group size is zero";
+    return;
----------------
Please include this text in the diagnostic rather than hard-coding it here. You can use `%select` to differentiate between different text snippets in the diagnostic, and the diagnostic doesn't need to continue to try to be generic. Same comment applies elsewhere.

================
Comment at: lib/Sema/SemaDeclAttr.cpp:4988
@@ +4987,3 @@
+      return;
+  }
+
----------------
This will change if you use the optional bit rather than a variadic argument, but this silently eats attributes that have more than two arguments, which is not a good user experience.

================
Comment at: lib/Sema/SemaDeclAttr.cpp:6048
@@ -5976,2 +6047,3 @@
       D->setInvalidDecl();
-    } else if (Attr *A = D->getAttr<AMDGPUNumVGPRAttr>()) {
+    } else if (Attr *A = D->getAttr<AMDGPUFlatWorkGroupSizeAttr>()) {
+      Diag(D->getLocation(), diag::err_attribute_wrong_decl_type)
----------------
This list is getting to the point where we really need to start handling this in Attr.td soon. Are you planning to work on more AMDGPU attributes in the near future?

================
Comment at: test/SemaOpenCL/amdgpu-attrs.cl:52
@@ +51,3 @@
+__attribute__((amdgpu_flat_work_group_size(64, 32))) kernel void kernel_flat_work_group_size_64_32() {} // expected-error {{'amdgpu_flat_work_group_size' attribute parameter is invalid: minimum flat work-group size must not be greater than maximum flat work-group size}}
+__attribute__((amdgpu_waves_per_eu(4, 2))) kernel void kernel_waves_per_eu_4_2() {} // expected-error {{'amdgpu_waves_per_eu' attribute parameter is invalid: minimum number of waves per execution unit must not be greater than maximum number of waves per execution unit}}
+
----------------
I'd like to see a test that shows amdgpu_waves_per_eu with > 2 arguments to make sure it's handled appropriately.


https://reviews.llvm.org/D24513