Diagnose non-bool SV_CullPrimitive mesh outputs in Sema

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
 

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

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 <deque>
@@ -4089,6 +4090,10 @@ class Sema {
 
   /// cbuffer/tbuffer
   llvm::SmallVector<Decl*, 1> HLSLBuffers;
+  /// (semantic decl, leaf) pairs already given an SV_CullPrimitive type error,
+  /// so entry points that share a struct report it once.
+  llvm::SmallSet<std::pair<const Decl *, const Decl *>, 4>
+      HLSLDiagnosedCullPrimitives;
   Decl* ActOnStartHLSLBuffer(Scope* bufferScope, bool cbuffer, SourceLocation KwLoc,
     IdentifierInfo *Ident, SourceLocation IdentLoc,
     std::vector<hlsl::UnusualAnnotation *>& BufferAttributes,

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<RecordType>();
+  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<CXXRecordDecl>(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<hlsl::SemanticDecl>(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<CXXRecordDecl>(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<HLSLNumThreadsAttr>()))
@@ -17280,6 +17359,15 @@ void DiagnoseMeshEntry(Sema &S, FunctionDecl *FD, llvm::StringRef StageName) {
   if (!(FD->getAttr<HLSLOutputTopologyAttr>()))
     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<HLSLPrimitivesAttr>())
+      continue;
+    QualType Ty = Param->getType();
+    if (const ArrayType *AT = S.Context.getAsArrayType(Ty))
+      Ty = AT->getElementType();
+    DiagnoseCullPrimitiveDecl(S, Param, Ty);
+  }
   return;
 }
 

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;
+}

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);

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;
+};

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);
+}