[SPIR-V] Fix invalid codegen for empty cbuffers (microsoft#7717)

Each variable in the Uniform/Uniform Constant storage class must be
decorated with DescriptorSet + Binding.
-> VUID-StandaloneSpirv-UniformConstant-06677

When a cbuffer was empty (or only contained resources), an empty struct
and variable in the uniform storage class was created. Because the
cbuffer was empty, no binding/descriptor set was assigned, hence no
decoration was emitted.

This was fine as long as we compiled with optimizations on, as DCE would
get rid of this unused cbuffer. But compiling with -O0 would yield a
validation error.

Fixes microsoft#7681

Author: Keenuts

tools/clang/lib/SPIRV/DeclResultIdMapper.cpp

@@ -1381,7 +1381,6 @@ SpirvVariable *DeclResultIdMapper::createStructOrStructArrayVarOfExplicitLayout(
       }
 
       if (isResourceType(varType)) {
-        createExternVar(fieldVar);
         continue;
       }
     }
@@ -1473,7 +1472,28 @@ void DeclResultIdMapper::createEnumConstant(const EnumConstantDecl *decl) {
   astDecls[valueDecl] = createDeclSpirvInfo(varInstr);
 }
 
-SpirvVariable *DeclResultIdMapper::createCTBuffer(const HLSLBufferDecl *decl) {
+void DeclResultIdMapper::createCTBuffer(const HLSLBufferDecl *decl) {
+
+  SmallVector<const VarDecl *, 4> variablesToDeclare;
+  for (const auto *subDecl : decl->decls()) {
+    if (shouldSkipInStructLayout(subDecl))
+      continue;
+
+    // If the subDecl is a resource, it is lowered as a standalone variable.
+    const auto *varDecl = cast<VarDecl>(subDecl);
+    if (isResourceType(varDecl->getType())) {
+      createExternVar(varDecl);
+      continue;
+    }
+
+    variablesToDeclare.push_back(varDecl);
+  }
+
+  // If the cbuffer is empty or only contains resources, skip the variable
+  // creation.
+  if (variablesToDeclare.size() == 0)
+    return;
+
   // This function handles creation of cbuffer or tbuffer.
   const auto usageKind =
       decl->isCBuffer() ? ContextUsageKind::CBuffer : ContextUsageKind::TBuffer;
@@ -1486,30 +1506,16 @@ SpirvVariable *DeclResultIdMapper::createCTBuffer(const HLSLBufferDecl *decl) {
   // mapped to the <result-id> of the buffer object, which means when querying
   // querying the <result-id> for a certain VarDecl, we need to do an extra
   // OpAccessChain.
-  int index = 0;
-  for (const auto *subDecl : decl->decls()) {
-    if (shouldSkipInStructLayout(subDecl))
-      continue;
-
-    // If subDecl is a variable with resource type, we already added a separate
-    // OpVariable for it in createStructOrStructArrayVarOfExplicitLayout().
-    const auto *varDecl = cast<VarDecl>(subDecl);
-    if (isResourceType(varDecl->getType()))
-      continue;
+  for (unsigned I = 0; I < variablesToDeclare.size(); ++I)
+    registerVariableForDecl(variablesToDeclare[I],
+                            createDeclSpirvInfo(bufferVar, I));
 
-    registerVariableForDecl(varDecl, createDeclSpirvInfo(bufferVar, index++));
-  }
-  // If it does not contains a member with non-resource type, we do not want to
-  // set a dedicated binding number.
-  if (index != 0) {
-    resourceVars.emplace_back(
-        bufferVar, decl, decl->getLocation(), getResourceBinding(decl),
-        decl->getAttr<VKBindingAttr>(), decl->getAttr<VKCounterBindingAttr>());
-  }
+  resourceVars.emplace_back(
+      bufferVar, decl, decl->getLocation(), getResourceBinding(decl),
+      decl->getAttr<VKBindingAttr>(), decl->getAttr<VKCounterBindingAttr>());
 
-  if (!spirvOptions.debugInfoRich) {
-    return bufferVar;
-  }
+  if (!spirvOptions.debugInfoRich)
+    return;
 
   auto *dbgGlobalVar = createDebugGlobalVariable(
       bufferVar, QualType(), decl->getLocation(), decl->getName());
@@ -1529,7 +1535,7 @@ SpirvVariable *DeclResultIdMapper::createCTBuffer(const HLSLBufferDecl *decl) {
   // mapping.
   spvContext.registerStructDeclForSpirvType(resultType, decl);
 
-  return bufferVar;
+  return;
 }
 
 SpirvVariable *DeclResultIdMapper::createPushConstant(const VarDecl *decl) {

tools/clang/lib/SPIRV/DeclResultIdMapper.h

@@ -326,7 +326,7 @@ class DeclResultIdMapper {
   /// for the whole buffer. When we refer to the field VarDecl later, we need
   /// to do an extra OpAccessChain to get its pointer from the SPIR-V variable
   /// standing for the whole buffer.
-  SpirvVariable *createCTBuffer(const HLSLBufferDecl *decl);
+  void createCTBuffer(const HLSLBufferDecl *decl);
 
   /// \brief Creates a PushConstant block from the given decl.
   SpirvVariable *createPushConstant(const VarDecl *decl);

tools/clang/lib/SPIRV/SpirvEmitter.cpp

@@ -1942,7 +1942,7 @@ void SpirvEmitter::doHLSLBufferDecl(const HLSLBufferDecl *bufferDecl) {
         bufferDecl,
         DeclResultIdMapper::ContextUsageKind::ShaderRecordBufferKHR);
   } else {
-    (void)declIdMapper.createCTBuffer(bufferDecl);
+    declIdMapper.createCTBuffer(bufferDecl);
   }
 }
 

tools/clang/test/CodeGenSPIRV/type.cbuffer.empty.hlsl

@@ -0,0 +1,9 @@
+// RUN: %dxc -T ps_6_0 -E main -O0 %s -spirv | FileCheck %s
+
+// CHECK-NOT: %empty = OpVariable %_ptr_Uniform_type_empty Uniform
+cbuffer empty {
+};
+
+float4 main(float2 uv : TEXCOORD) : SV_TARGET {
+  return uv.xyxy;
+}

tools/clang/test/CodeGenSPIRV/type.cbuffer.including.resource.hlsl

@@ -1,5 +1,4 @@
-// RUN: %dxc -T ps_6_0 -E main -fcgl -Vd %s -spirv | FileCheck %s
-// FIXME(7681): Enable validation again once codegen is fixed.
+// RUN: %dxc -T ps_6_0 -E main -fcgl %s -spirv | FileCheck %s
 
 // CHECK-NOT: OpDecorate %buf3
 
@@ -16,28 +15,32 @@
 // CHECK: OpDecorate %w DescriptorSet 0
 // CHECK: OpDecorate %w Binding 5
 
-// CHECK: %type_buf0 = OpTypeStruct %v4float
+// CHECK:     %type_buf0 = OpTypeStruct %v4float
+// CHECK:     %type_buf1 = OpTypeStruct %v4float
+// CHECK:     %type_buf2 = OpTypeStruct %v4float
+// CHECK-NOT: %type_buf3 = OpTypeStruct
+
+// CHECK: %buf0 = OpVariable %_ptr_Uniform_type_buf0 Uniform
 cbuffer buf0 : register(b0) {
   float4 foo;
 };
 
-// CHECK: %type_buf1 = OpTypeStruct %v4float
+// CHECK: %buf1 = OpVariable %_ptr_Uniform_type_buf1 Uniform
 cbuffer buf1 : register(b4) {
   float4 bar;
 };
 
-// CHECK: %type_buf2 = OpTypeStruct %v4float
-// CHECK: %type_buf3 = OpTypeStruct
-
 // CHECK: %y = OpVariable %_ptr_UniformConstant_type_2d_image UniformConstant
 // CHECK: %z = OpVariable %_ptr_UniformConstant_type_sampler UniformConstant
+// CHECK: %buf2 = OpVariable %_ptr_Uniform_type_buf2 Uniform
 cbuffer buf2 {
   float4 x;
   Texture2D y;
   SamplerState z;
 };
 
 // CHECK: %w = OpVariable %_ptr_UniformConstant_type_sampler UniformConstant
+// CHECK-NOT: %buf3 = OpVariable %_ptr_Uniform_type_buf3 Uniform
 cbuffer buf3 : register(b2) {
   SamplerState w;
 }