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@+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@+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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
