[PATCH] D133983: [HLSL] Add SV_DispatchThreadID

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In D133983#3807284 <https://reviews.llvm.org/D133983#3807284>, @python3kgae wrote:

> In D133983#3805761 <https://reviews.llvm.org/D133983#3805761>, @aaron.ballman wrote:
>
>> There are no tests for applying this to a global variable, so those should be added.
>
> The global variable in the Subjects is wrong.
> It should be Field.

I wondered if that was the case, thanks for the fix. :-)

> Support for semantic on field is a bigger change.
> Created https://github.com/llvm/llvm-project/issues/57889 to track it.

Thanks!

You should add additional test coverage: applying the attribute to a static data member, to a non-static data member, to a variable, to an unnamed parameter, and to a lambda parameter. The tests should demonstrate that we give good diagnostics where appropriate, instead of crashing, asserting, or silently accepting.



================
Comment at: clang/include/clang/Basic/AttrDocs.td:6596-6598
+The ``SV_DispatchThreadID`` semantic, when applied to an input parameter, specifies a
+data binding to map global thread offset within the Dispatch call(per dimension of the group) to the specified parameter.
+When applied to a field of a struct, the data binding is specified to the field when the struct is used as a parameter type.
----------------
You should also wrap the docs to the usual 80-col limit.

Can you apply the attribute to a static data member in a struct, or only fields?


================
Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:11656
+def err_hlsl_attr_invalid_type : Error<
+   "Attribute %0 only applies to fields/parameters that have type %1">;
 def err_hlsl_numthreads_argument_oor : Error<"argument '%select{X|Y|Z}0' to numthreads attribute cannot exceed %1">;
----------------



================
Comment at: clang/lib/Sema/SemaDeclAttr.cpp:6916
+    return false;
+  if (auto *VT = T->getAs<VectorType>())
+    return VT->getNumElements() <= 3;
----------------



================
Comment at: clang/lib/Sema/SemaDeclAttr.cpp:6940
+
+  auto *VD = cast<ValueDecl>(D);
+  if (!isLegalTypeForHLSLSV_DispatchThreadID(VD->getType())) {
----------------
This will cause an assert when the attribute appears on a field -- you need to check for a field decl above and give a diagnostic about it not being supported yet.


================
Comment at: clang/lib/Sema/SemaDeclAttr.cpp:6943
+    S.Diag(AL.getLoc(), diag::err_hlsl_attr_invalid_type)
+        << "SV_DispatchThreadID"
+        << "uint/uint2/uint3";
----------------



================
Comment at: clang/test/SemaHLSL/Semantics/entry_parameter.hlsl:4
 
 [numthreads(8,8, 1)]
+// expected-error at +2 {{attribute 'SV_GroupIndex' is unsupported in Mesh shaders, requires Compute}}
----------------



================
Comment at: clang/test/SemaHLSL/Semantics/invalid_entry_parameter.hlsl:3-13
+[numthreads(8,8, 1)]
+// expected-error at +1 {{Attribute SV_DispatchThreadID only applies to fields/parameters that have type uint/uint2/uint3}}
+void CSMain(float ID : SV_DispatchThreadID) {
+
+}
+
+struct ST {
----------------



================
Comment at: clang/test/SemaHLSL/Semantics/valid_entry_parameter.hlsl:3-16
+[numthreads(8,8, 1)]
+void CSMain(uint ID : SV_DispatchThreadID) {
+// CHECK: FunctionDecl 0x{{[0-9a-fA-F]+}} <{{.*}}> line:[[@LINE-1]]:6 CSMain 'void (uint)'
+// CHECK-NEXT: ParmVarDecl 0x{{[0-9a-fA-F]+}} <{{.*}}> col:18 ID 'uint'
+// CHECK-NEXT: HLSLSV_DispatchThreadIDAttr
+}
+[numthreads(8,8, 1)]
----------------



Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D133983/new/

https://reviews.llvm.org/D133983