diff --git a/docs/ReleaseNotes.md b/docs/ReleaseNotes.md index f1d765b9fce..14788221b7a 100644 --- a/docs/ReleaseNotes.md +++ b/docs/ReleaseNotes.md @@ -101,6 +101,9 @@ line upon naming the release. Refer to previous for appropriate section names. after bit-tracking dead code elimination. - Fixed an optimizer miscompile where selects of bitwise-not values guarded by signed less-than or unsigned compares were treated as a signed minimum. +- Fixed non-bool `SV_CullPrimitive` mesh shader outputs producing invalid + SPIR-V. They are now a front-end error for both DXIL and SPIR-V, instead of a + DXIL validation error. ### Upcoming Preview Release diff --git a/tools/clang/include/clang/Basic/DiagnosticSemaKinds.td b/tools/clang/include/clang/Basic/DiagnosticSemaKinds.td index 90323226fe7..e3767dc9795 100644 --- a/tools/clang/include/clang/Basic/DiagnosticSemaKinds.td +++ b/tools/clang/include/clang/Basic/DiagnosticSemaKinds.td @@ -7915,6 +7915,8 @@ def err_hlsl_missing_in_attr: Error< "%0 object must be an in parameter">; def err_hlsl_load_from_mesh_out_arrays: Error< "output arrays of a mesh shader can not be read from">; +def err_hlsl_incorrect_sv_type : Error< + "%0 must be applied to a %1, but %2 has type %3">; def err_hlsl_out_indices_array_incorrect_access: Error< "a vector in out indices array must be accessed as a whole">; def err_hlsl_unsupported_long_vector diff --git a/tools/clang/include/clang/Sema/Sema.h b/tools/clang/include/clang/Sema/Sema.h index 6eb0aba8018..e742fa0032f 100644 --- a/tools/clang/include/clang/Sema/Sema.h +++ b/tools/clang/include/clang/Sema/Sema.h @@ -46,6 +46,7 @@ #include "llvm/ADT/Optional.h" #include "llvm/ADT/SetVector.h" #include "llvm/ADT/SmallPtrSet.h" +#include "llvm/ADT/SmallSet.h" // HLSL Change #include "llvm/ADT/SmallVector.h" #include "llvm/ADT/TinyPtrVector.h" #include @@ -4089,6 +4090,10 @@ class Sema { /// cbuffer/tbuffer llvm::SmallVector HLSLBuffers; + /// (semantic decl, leaf) pairs already given an SV_CullPrimitive type error, + /// so entry points that share a struct report it once. + llvm::SmallSet, 4> + HLSLDiagnosedCullPrimitives; Decl* ActOnStartHLSLBuffer(Scope* bufferScope, bool cbuffer, SourceLocation KwLoc, IdentifierInfo *Ident, SourceLocation IdentLoc, std::vector& BufferAttributes, diff --git a/tools/clang/lib/Sema/SemaHLSL.cpp b/tools/clang/lib/Sema/SemaHLSL.cpp index 1d6b56b2693..15007151772 100644 --- a/tools/clang/lib/Sema/SemaHLSL.cpp +++ b/tools/clang/lib/Sema/SemaHLSL.cpp @@ -17272,6 +17272,85 @@ void DiagnoseVertexEntry(Sema &S, FunctionDecl *FD, llvm::StringRef StageName) { return; } +// SV_CullPrimitive on a mesh shader primitive output must be a scalar bool +// (bool1 and bool1x1 count). Checked here so DXIL and SPIR-V give the same +// error. Only primitive outputs are checked. Both backends already reject +// SV_CullPrimitive on vertex outputs whatever its type, and pixel shader +// inputs are odd: DXIL accepts any type there and replaces the input with 0. + +// Returns the definition of a struct type, or null for anything else. An +// incomplete struct has already been diagnosed and has nothing to walk. +static const RecordDecl *GetCullPrimitiveRecord(QualType Ty) { + const RecordType *RT = Ty->getAs(); + if (!RT || hlsl::IsHLSLVecMatType(Ty)) + return nullptr; + return RT->getDecl()->getDefinition(); +} + +// The backends apply a semantic on a struct to each of its leaf fields, so +// check every leaf of Ty. Errors point at SemDecl, which holds the semantic, +// and name the leaf. +static void DiagnoseCullPrimitiveLeaves(Sema &S, const NamedDecl *SemDecl, + llvm::StringRef SemName, + const NamedDecl *Leaf, QualType Ty) { + Ty = Ty.getUnqualifiedType(); + if (Ty->isIncompleteType()) + return; + if (const RecordDecl *RD = GetCullPrimitiveRecord(Ty)) { + if (const auto *CRD = dyn_cast(RD)) + for (const CXXBaseSpecifier &Base : CRD->bases()) + DiagnoseCullPrimitiveLeaves(S, SemDecl, SemName, Leaf, Base.getType()); + for (const FieldDecl *FD : RD->fields()) + DiagnoseCullPrimitiveLeaves(S, SemDecl, SemName, FD, FD->getType()); + return; + } + bool IsScalarBool = hlsl::IsHLSLVecMatType(Ty) + ? hlsl::GetElementCount(Ty) == 1 && + hlsl::GetElementTypeOrType(Ty)->isBooleanType() + : Ty->isBooleanType(); + if (IsScalarBool) + return; + // Entry points that share a struct would otherwise report it once each. + if (!S.HLSLDiagnosedCullPrimitives.insert({SemDecl, Leaf}).second) + return; + S.Diags.Report(SemDecl->getLocation(), diag::err_hlsl_incorrect_sv_type) + << SemName << "scalar bool" << Leaf << Ty; +} + +static void DiagnoseCullPrimitiveRecord(Sema &S, QualType Ty); + +// A semantic on a field or parameter overrides any semantics inside its type, +// so only look for them inside when D has none. +static void DiagnoseCullPrimitiveDecl(Sema &S, const NamedDecl *D, + QualType Ty) { + for (const hlsl::UnusualAnnotation *UA : D->getUnusualAnnotations()) { + const auto *SD = dyn_cast(UA); + if (!SD) + continue; + llvm::StringRef BaseName; + uint32_t Index; + hlsl::Semantic::DecomposeNameAndIndex(SD->SemanticName, &BaseName, &Index); + if (hlsl::Semantic::GetByName(BaseName)->GetKind() == + hlsl::Semantic::Kind::CullPrimitive) + DiagnoseCullPrimitiveLeaves(S, D, SD->SemanticName, D, Ty); + return; + } + DiagnoseCullPrimitiveRecord(S, Ty); +} + +static void DiagnoseCullPrimitiveRecord(Sema &S, QualType Ty) { + while (const ArrayType *AT = S.Context.getAsArrayType(Ty)) + Ty = AT->getElementType(); + const RecordDecl *RD = GetCullPrimitiveRecord(Ty); + if (!RD) + return; + if (const auto *CRD = dyn_cast(RD)) + for (const CXXBaseSpecifier &Base : CRD->bases()) + DiagnoseCullPrimitiveRecord(S, Base.getType()); + for (const FieldDecl *FD : RD->fields()) + DiagnoseCullPrimitiveDecl(S, FD, FD->getType()); +} + void DiagnoseMeshEntry(Sema &S, FunctionDecl *FD, llvm::StringRef StageName) { if (!(FD->getAttr())) @@ -17280,6 +17359,15 @@ void DiagnoseMeshEntry(Sema &S, FunctionDecl *FD, llvm::StringRef StageName) { if (!(FD->getAttr())) S.Diags.Report(FD->getLocation(), diag::err_hlsl_missing_attr) << StageName << "outputtopology"; + // The semantic applies to each primitive, so check the element type. + for (const ParmVarDecl *Param : FD->params()) { + if (!Param->hasAttr()) + continue; + QualType Ty = Param->getType(); + if (const ArrayType *AT = S.Context.getAsArrayType(Ty)) + Ty = AT->getElementType(); + DiagnoseCullPrimitiveDecl(S, Param, Ty); + } return; } diff --git a/tools/clang/test/CodeGenSPIRV/meshshading.ext.cullprimitive.bool1.hlsl b/tools/clang/test/CodeGenSPIRV/meshshading.ext.cullprimitive.bool1.hlsl new file mode 100644 index 00000000000..62ead1eda67 --- /dev/null +++ b/tools/clang/test/CodeGenSPIRV/meshshading.ext.cullprimitive.bool1.hlsl @@ -0,0 +1,26 @@ +// RUN: %dxc -T ms_6_5 -fspv-target-env=vulkan1.3 -E main -fcgl %s -spirv -DCULL_TYPE=bool1 | FileCheck %s +// RUN: %dxc -T ms_6_5 -fspv-target-env=vulkan1.3 -E main -fcgl %s -spirv -DCULL_TYPE=bool1x1 | FileCheck %s + +// CHECK: OpDecorate [[cullprim:%[0-9]+]] BuiltIn CullPrimitiveEXT +// CHECK: [[cullprim]] = OpVariable %_ptr_Output__arr_bool_uint_1 Output + +struct MeshPerVertex { + float4 position : SV_Position; +}; + +struct MeshPerPrimitive { + CULL_TYPE cull : SV_CullPrimitive; +}; + +[outputtopology("triangle")] +[numthreads(1, 1, 1)] +void main(out vertices MeshPerVertex verts[3], + out indices uint3 tris[1], + out primitives MeshPerPrimitive prims[1]) { + SetMeshOutputCounts(3, 1); + verts[0].position = 0; + verts[1].position = 0; + verts[2].position = 0; + tris[0] = uint3(0, 1, 2); + prims[0].cull = true; +} diff --git a/tools/clang/test/CodeGenSPIRV/meshshading.ext.triangle.mesh.hlsl b/tools/clang/test/CodeGenSPIRV/meshshading.ext.triangle.mesh.hlsl index 904c45c69fe..7651fedb90e 100644 --- a/tools/clang/test/CodeGenSPIRV/meshshading.ext.triangle.mesh.hlsl +++ b/tools/clang/test/CodeGenSPIRV/meshshading.ext.triangle.mesh.hlsl @@ -49,7 +49,7 @@ // CHECK: %gl_PrimitiveID = OpVariable %_ptr_Output__arr_int_uint_81 Output // CHECK: %gl_Layer = OpVariable %_ptr_Output__arr_int_uint_81 Output // CHECK: %gl_ViewportIndex = OpVariable %_ptr_Output__arr_int_uint_81 Output -// CHECK: [[cullprim]] = OpVariable %_ptr_Output__arr_int_uint_81 Output +// CHECK: [[cullprim]] = OpVariable %_ptr_Output__arr_bool_uint_81 Output // CHECK: [[primshadingrate]] = OpVariable %_ptr_Output__arr_uint_uint_81 Output // CHECK: %out_var_PRIM_USER = OpVariable %_ptr_Output__arr_v3float_uint_81 Output // CHECK: %out_var_PRIM_USER_ARR = OpVariable %_ptr_Output__arr__arr_v4float_uint_2_uint_81 Output @@ -70,7 +70,7 @@ struct MeshPerPrimitive { int primId : SV_PrimitiveID; // -> Builtin PrimitiveId int layer : SV_RenderTargetArrayIndex; // -> Builtin Layer int vpIdx : SV_ViewportArrayIndex; // -> Builtin ViewportIndex - int cullPrim : SV_CullPrimitive; // -> BUiltin CullPrimitiveEXT + bool cullPrim : SV_CullPrimitive; // -> Builtin CullPrimitiveEXT uint shadingRate : SV_ShadingRate; // -> Builtin PrimitiveShadingRateEXT float3 userPrimAttr : PRIM_USER; float4 userPrimAttrArr[2] : PRIM_USER_ARR; @@ -198,9 +198,9 @@ void main( // CHECK: OpAccessChain %_ptr_Output_int %gl_ViewportIndex {{%[0-9]+}} // CHECK: OpStore {{%[0-9]+}} %int_12 prims[tig].vpIdx = 12; -// CHECK: OpAccessChain %_ptr_Output_int [[cullprim]] {{%[0-9]+}} -// CHECK: OpStore {{%[0-9]+}} %int_13 - prims[tig].cullPrim = 13; +// CHECK: OpAccessChain %_ptr_Output_bool [[cullprim]] {{%[0-9]+}} +// CHECK: OpStore {{%[0-9]+}} %true + prims[tig].cullPrim = true; // CHECK: OpAccessChain %_ptr_Output_uint [[primshadingrate]] {{%[0-9]+}} // CHECK: OpStore {{%[0-9]+}} %uint_14 prims[tig].shadingRate = 14; @@ -220,7 +220,7 @@ void main( prim.primId = 10; prim.layer = 11; prim.vpIdx = 12; - prim.cullPrim = 1; + prim.cullPrim = true; prim.shadingRate = 0xa; prim.userPrimAttrArr[0] = float4(4.0,5.0,6.0,7.0); prim.userPrimAttrArr[1] = float4(8.0,9.0,10.0,11.0); diff --git a/tools/clang/test/SemaHLSL/cullprimitive-incomplete.hlsl b/tools/clang/test/SemaHLSL/cullprimitive-incomplete.hlsl new file mode 100644 index 00000000000..aa082943f44 --- /dev/null +++ b/tools/clang/test/SemaHLSL/cullprimitive-incomplete.hlsl @@ -0,0 +1,42 @@ +// RUN: %dxc -T lib_6_8 -verify %s + +// The SV_CullPrimitive check must not crash on incomplete primitive types. + +struct Vert { + float4 pos : SV_Position; +}; + +struct DefinedLater; +struct NeverDefined; +struct Incomplete; +struct HasIncompleteField { + Incomplete inner; // expected-error {{field has incomplete type 'Incomplete'}} expected-note@-2 {{forward declaration of 'Incomplete'}} +}; + +[shader("mesh")] [numthreads(1, 1, 1)] [outputtopology("triangle")] +void ms_later(out vertices Vert verts[3], out indices uint3 tris[1], + out primitives DefinedLater prims[1]) { // expected-error {{variable has incomplete type 'DefinedLater __restrict[1]'}} + SetMeshOutputCounts(0, 0); +} + +[shader("mesh")] [numthreads(1, 1, 1)] [outputtopology("triangle")] +void ms_never(out vertices Vert verts[3], out indices uint3 tris[1], + out primitives NeverDefined prims[1]) { // expected-error {{variable has incomplete type 'NeverDefined __restrict[1]'}} + SetMeshOutputCounts(0, 0); +} + +[shader("mesh")] [numthreads(1, 1, 1)] [outputtopology("triangle")] +void ms_param(out vertices Vert verts[3], out indices uint3 tris[1], + out primitives NeverDefined prims[1] : SV_CullPrimitive) { // expected-error {{variable has incomplete type 'NeverDefined __restrict[1]'}} + SetMeshOutputCounts(0, 0); +} + +[shader("mesh")] [numthreads(1, 1, 1)] [outputtopology("triangle")] +void ms_field(out vertices Vert verts[3], out indices uint3 tris[1], + out primitives HasIncompleteField prims[1]) { + SetMeshOutputCounts(0, 0); +} + +struct DefinedLater { + bool cull : SV_CullPrimitive; +}; diff --git a/tools/clang/test/SemaHLSL/cullprimitive-type-entry.hlsl b/tools/clang/test/SemaHLSL/cullprimitive-type-entry.hlsl new file mode 100644 index 00000000000..ed92993d330 --- /dev/null +++ b/tools/clang/test/SemaHLSL/cullprimitive-type-entry.hlsl @@ -0,0 +1,24 @@ +// RUN: %dxc -T ms_6_5 -E main -verify %s + +// The SV_CullPrimitive check also runs for a profile entry point, which +// reaches DiagnoseEntry by a different path than a library shader. + +struct Vert { + float4 pos : SV_Position; +}; +struct Prim { + bool ok : SV_CullPrimitive; +}; +struct BadPrim { + uint cull : SV_CullPrimitive; // expected-error {{SV_CullPrimitive must be applied to a scalar bool, but 'cull' has type 'uint'}} +}; +struct Prims { + Prim good; + BadPrim bad; +}; + +[numthreads(1, 1, 1)] [outputtopology("triangle")] +void main(out vertices Vert verts[3], out indices uint3 tris[1], + out primitives Prims prims[1]) { + SetMeshOutputCounts(0, 0); +} diff --git a/tools/clang/test/SemaHLSL/cullprimitive-type.hlsl b/tools/clang/test/SemaHLSL/cullprimitive-type.hlsl new file mode 100644 index 00000000000..bd5ae7a0cd7 --- /dev/null +++ b/tools/clang/test/SemaHLSL/cullprimitive-type.hlsl @@ -0,0 +1,144 @@ +// RUN: %dxc -T lib_6_8 -verify %s + +// SV_CullPrimitive on a mesh shader primitive output must be a scalar bool. +// A semantic on a struct applies to each of its leaf fields. Pixel shader +// inputs are replaced with 0, so any type is accepted there. + +struct Vert { + float4 pos : SV_Position; +}; + +struct PrimBool { + bool cull : SV_CullPrimitive; +}; +struct PrimBool1 { + bool1 cull : SV_CullPrimitive; +}; +struct PrimBool1x1 { + bool1x1 cull : SV_CullPrimitive; +}; +struct PrimUint { + uint cull : SV_CullPrimitive; // expected-error {{SV_CullPrimitive must be applied to a scalar bool, but 'cull' has type 'uint'}} +}; +struct PrimBool2 { + bool2 cull : SV_CullPrimitive; // expected-error {{SV_CullPrimitive must be applied to a scalar bool, but 'cull' has type 'bool2'}} +}; +struct PrimArray { + bool cull[2] : SV_CullPrimitive; // expected-error {{SV_CullPrimitive must be applied to a scalar bool, but 'cull' has type 'bool [2]'}} +}; +struct PrimBool1x2 { + bool1x2 cull : SV_CullPrimitive; // expected-error {{SV_CullPrimitive must be applied to a scalar bool, but 'cull' has type 'bool1x2'}} +}; +struct PrimIndexed { + uint cull : SV_CullPrimitive1; // expected-error {{SV_CullPrimitive1 must be applied to a scalar bool, but 'cull' has type 'uint'}} +}; +struct PrimLowercase { + uint cull : sv_cullprimitive; // expected-error {{sv_cullprimitive must be applied to a scalar bool, but 'cull' has type 'uint'}} +}; +enum CullEnum { CullOff, CullOn }; +struct PrimEnum { + CullEnum cull : SV_CullPrimitive; // expected-error {{SV_CullPrimitive must be applied to a scalar bool, but 'cull' has type 'CullEnum'}} +}; +// Inner is reached by two entry points and through an array, but its error +// is reported once. +struct Inner { + uint cull : SV_CullPrimitive; // expected-error {{SV_CullPrimitive must be applied to a scalar bool, but 'cull' has type 'uint'}} +}; +struct PrimNested { + float4 col : COLOR0; + Inner inner; +}; +struct PrimArrayOfStructs { + Inner inner[2]; +}; +struct Base { + uint cull : SV_CullPrimitive; // expected-error {{SV_CullPrimitive must be applied to a scalar bool, but 'cull' has type 'uint'}} +}; +struct PrimDerived : Base {}; +// The semantic on 'data' overrides the one inside Overridden. +struct Overridden { + uint cull : SV_CullPrimitive; +}; +struct PrimOverride { + Overridden data : TEXCOORD0; +}; +// A semantic on a struct field applies to each leaf inside it. +struct WrapBool { + bool value; +}; +struct WrapUint { + uint value; +}; +struct WrapMixed { + bool ok; + uint bad; +}; +struct WrapDerived : WrapUint {}; +struct PrimWrapBool { + WrapBool data : SV_CullPrimitive; +}; +struct PrimWrapUint { + WrapUint data : SV_CullPrimitive; // expected-error {{SV_CullPrimitive must be applied to a scalar bool, but 'value' has type 'uint'}} +}; +struct PrimWrapMixed { + WrapMixed data : SV_CullPrimitive; // expected-error {{SV_CullPrimitive must be applied to a scalar bool, but 'bad' has type 'uint'}} +}; +struct PrimWrapDerived { + WrapDerived data : SV_CullPrimitive; // expected-error {{SV_CullPrimitive must be applied to a scalar bool, but 'value' has type 'uint'}} +}; + +#define MESH(name, Prim) \ + [shader("mesh")] [numthreads(1, 1, 1)] [outputtopology("triangle")] \ + void name(out vertices Vert verts[3], out indices uint3 tris[1], \ + out primitives Prim prims[1]) { \ + SetMeshOutputCounts(0, 0); \ + } + +MESH(ms_bool, PrimBool) +MESH(ms_bool1, PrimBool1) +MESH(ms_bool1x1, PrimBool1x1) +MESH(ms_uint, PrimUint) +MESH(ms_bool2, PrimBool2) +MESH(ms_array, PrimArray) +MESH(ms_bool1x2, PrimBool1x2) +MESH(ms_indexed, PrimIndexed) +MESH(ms_lowercase, PrimLowercase) +MESH(ms_enum, PrimEnum) +MESH(ms_nested, PrimNested) +MESH(ms_array_of_structs, PrimArrayOfStructs) +MESH(ms_derived, PrimDerived) +MESH(ms_override, PrimOverride) +MESH(ms_wrap_bool, PrimWrapBool) +MESH(ms_wrap_uint, PrimWrapUint) +MESH(ms_wrap_mixed, PrimWrapMixed) +MESH(ms_wrap_derived, PrimWrapDerived) + +// SV_CullPrimitive directly on the parameter applies to each primitive. +[shader("mesh")] [numthreads(1, 1, 1)] [outputtopology("triangle")] +void ms_param_bool(out vertices Vert verts[3], out indices uint3 tris[1], + out primitives bool prims[1] : SV_CullPrimitive) { + SetMeshOutputCounts(0, 0); +} +[shader("mesh")] [numthreads(1, 1, 1)] [outputtopology("triangle")] +void ms_param_bool1(out vertices Vert verts[3], out indices uint3 tris[1], + out primitives bool1 prims[1] : SV_CullPrimitive) { + SetMeshOutputCounts(0, 0); +} +[shader("mesh")] [numthreads(1, 1, 1)] [outputtopology("triangle")] +void ms_param_wrap_bool(out vertices Vert verts[3], out indices uint3 tris[1], + out primitives WrapBool prims[1] : SV_CullPrimitive) { + SetMeshOutputCounts(0, 0); +} +[shader("mesh")] [numthreads(1, 1, 1)] [outputtopology("triangle")] +void ms_param_wrap_uint(out vertices Vert verts[3], out indices uint3 tris[1], + out primitives WrapUint prims[1] : SV_CullPrimitive) { // expected-error {{SV_CullPrimitive must be applied to a scalar bool, but 'value' has type 'uint'}} + SetMeshOutputCounts(0, 0); +} +[shader("mesh")] [numthreads(1, 1, 1)] [outputtopology("triangle")] +void ms_param_uint(out vertices Vert verts[3], out indices uint3 tris[1], + out primitives uint prims[1] : SV_CullPrimitive) { // expected-error {{SV_CullPrimitive must be applied to a scalar bool, but 'prims' has type 'uint'}} + SetMeshOutputCounts(0, 0); +} + +[shader("pixel")] +float4 ps_uint(uint cull : SV_CullPrimitive) : SV_Target { return cull; }