[spirv] separate resource from cbuffer (#4040)

The current DXC does not separate the resources from a cbuffer, which
results in a OpTypeStruct including resources. We have to separate the
resources from a cbuffer.

Fixes #4019

docs/SPIR-V.rst

@@ -1643,6 +1643,53 @@ automatically be applied according to ``-fvk-*shift N M``.
       CBUFFER
       CONSTANTBUFFER
 
+Binding number assignment for resources in cbuffer
+~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+
+Basically, we use the same binding assignment rule described above for a
+cbuffer, but when a cbuffer contains one or more resources, it is inevitable
+to use multiple binding numbers for a single cbuffer. For this type of
+cbuffers, we first assign the next available binding number to the resources.
+Based the order of the appearance in the cbuffer, a resource that appears early
+uses a smaller (earlier available) binding number than a resource that appears
+later. After assigning binding numbers to all resource members, if the cbuffer
+contains one or more members with non-resource types, it creates a struct for
+the remaining members and assign the next available binding number to the
+variable with the struct type.
+
+For example, the binding numbers for the following resources and cbuffers
+
+.. code:: hlsl
+  cbuffer buf0 : register(b0) {
+    float4 non_resource0;
+  };
+  cbuffer buf1 : register(b4) {
+    float4 non_resource1;
+  };
+  cbuffer buf2 {
+    float4 non_resource2;
+    Texture2D resource0;
+    SamplerState resource1;
+  };
+  cbuffer buf3 : register(b2) {
+    SamplerState resource2;
+  }
+
+will be
+
+- ``buf0``: 0 because of ``register(b0)``
+
+- ``buf1``: 4 because of ``register(b4)``
+
+- ``resource2``: 2 because of ``register(b2)``. Note that ``buf3`` is empty
+  without ``resource2``. We do not assign a binding number to an empty struct.
+
+- ``resource0``: 1 because it is the next available binding number.
+
+- ``resource1``: 3 because it is the next available binding number.
+
+- ``buf2`` including only ``non_resource2``: 5 because it is the next available
+  binding number.
 
 Summary
 ~~~~~~~

tools/clang/lib/SPIRV/DeclResultIdMapper.cpp

@@ -1099,14 +1099,20 @@ SpirvVariable *DeclResultIdMapper::createStructOrStructArrayVarOfExplicitLayout(
     // HLSLBufferDecls).
     assert(isa<VarDecl>(subDecl) || isa<FieldDecl>(subDecl));
     const auto *declDecl = cast<DeclaratorDecl>(subDecl);
+    auto varType = declDecl->getType();
+    if (const auto *fieldVar = dyn_cast<VarDecl>(subDecl)) {
+      if (isResourceType(varType)) {
+        createExternVar(fieldVar);
+        continue;
+      }
+    }
 
     // In case 'register(c#)' annotation is placed on a global variable.
     const hlsl::RegisterAssignment *registerC =
         forGlobals ? getRegisterCAssignment(declDecl) : nullptr;
 
     // All fields are qualified with const. It will affect the debug name.
     // We don't need it here.
-    auto varType = declDecl->getType();
     varType.removeLocalConst();
     HybridStructType::FieldInfo info(varType, declDecl->getName(),
                                      declDecl->getAttr<VKOffsetAttr>(),
@@ -1185,12 +1191,21 @@ SpirvVariable *DeclResultIdMapper::createCTBuffer(const HLSLBufferDecl *decl) {
     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;
+
     astDecls[varDecl] = createDeclSpirvInfo(bufferVar, index++);
   }
-  resourceVars.emplace_back(
-      bufferVar, decl, decl->getLocation(), getResourceBinding(decl),
-      decl->getAttr<VKBindingAttr>(), decl->getAttr<VKCounterBindingAttr>());
+  // 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>());
+  }
 
   auto *dbgGlobalVar = createDebugGlobalVariable(
       bufferVar, QualType(), decl->getLocation(), decl->getName());
@@ -1350,7 +1365,12 @@ DeclResultIdMapper::createShaderRecordBuffer(const HLSLBufferDecl *decl,
     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;
+
     astDecls[varDecl] = createDeclSpirvInfo(bufferVar, index++);
   }
   return bufferVar;
@@ -1365,10 +1385,6 @@ void DeclResultIdMapper::createGlobalsCBuffer(const VarDecl *var) {
       context, /*arraySize*/ 0, ContextUsageKind::Globals, "type.$Globals",
       "$Globals");
 
-  resourceVars.emplace_back(globals, /*decl*/ nullptr, SourceLocation(),
-                            nullptr, nullptr, nullptr, /*isCounterVar*/ false,
-                            /*isGlobalsCBuffer*/ true);
-
   uint32_t index = 0;
   for (const auto *decl : collectDeclsInDeclContext(context)) {
     if (const auto *varDecl = dyn_cast<VarDecl>(decl)) {
@@ -1387,9 +1403,23 @@ void DeclResultIdMapper::createGlobalsCBuffer(const VarDecl *var) {
         return;
       }
 
+      // If subDecl is a variable with resource type, we already added a
+      // separate OpVariable for it in
+      // createStructOrStructArrayVarOfExplicitLayout().
+      if (isResourceType(varDecl->getType()))
+        continue;
+
       astDecls[varDecl] = createDeclSpirvInfo(globals, 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(globals, /*decl*/ nullptr, SourceLocation(),
+                              nullptr, nullptr, nullptr, /*isCounterVar*/ false,
+                              /*isGlobalsCBuffer*/ true);
+  }
 }
 
 SpirvFunction *DeclResultIdMapper::getOrRegisterFn(const FunctionDecl *fn) {

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

@@ -0,0 +1,60 @@
+// Run: %dxc -T ps_6_0 -E main
+
+// CHECK-NOT: OpDecorate %buf3
+
+// CHECK: OpDecorate %buf0 DescriptorSet 0
+// CHECK: OpDecorate %buf0 Binding 0
+// CHECK: OpDecorate %buf1 DescriptorSet 0
+// CHECK: OpDecorate %buf1 Binding 4
+// CHECK: OpDecorate %y DescriptorSet 0
+// CHECK: OpDecorate %y Binding 1
+// CHECK: OpDecorate %z DescriptorSet 0
+// CHECK: OpDecorate %z Binding 2
+// CHECK: OpDecorate %buf2 DescriptorSet 0
+// CHECK: OpDecorate %buf2 Binding 3
+// CHECK: OpDecorate %w DescriptorSet 0
+// CHECK: OpDecorate %w Binding 5
+
+// CHECK: %type_buf0 = OpTypeStruct %v4float
+cbuffer buf0 : register(b0) {
+  float4 foo;
+};
+
+// CHECK: %type_buf1 = OpTypeStruct %v4float
+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
+cbuffer buf2 {
+  float4 x;
+  Texture2D y;
+  SamplerState z;
+};
+
+// CHECK: %w = OpVariable %_ptr_UniformConstant_type_sampler UniformConstant
+cbuffer buf3 : register(b2) {
+  SamplerState w;
+}
+
+float4 main(float2 uv : TEXCOORD) : SV_TARGET {
+// CHECK: [[ptr_x:%\d+]] = OpAccessChain %_ptr_Uniform_v4float %buf2 %int_0
+// CHECK: [[x:%\d+]] = OpLoad %v4float [[ptr_x]]
+
+// CHECK: [[y:%\d+]] = OpLoad %type_2d_image %y
+// CHECK: [[z:%\d+]] = OpLoad %type_sampler %z
+// CHECK: [[yz:%\d+]] = OpSampledImage %type_sampled_image [[y]] [[z]]
+// CHECK: [[sample0:%\d+]] = OpImageSampleImplicitLod %v4float [[yz]]
+// CHECK: [[add0:%\d+]] = OpFAdd %v4float [[x]] [[sample0]]
+
+// CHECK: [[y:%\d+]] = OpLoad %type_2d_image %y
+// CHECK: [[w:%\d+]] = OpLoad %type_sampler %w
+// CHECK: [[yw:%\d+]] = OpSampledImage %type_sampled_image [[y]] [[w]]
+// CHECK: [[sample1:%\d+]] = OpImageSampleImplicitLod %v4float [[yw]]
+// CHECK: OpFAdd %v4float [[add0]] [[sample1]]
+  return x + y.Sample(z, uv) + y.Sample(w, uv);
+}

tools/clang/unittests/SPIRV/CodeGenSpirvTest.cpp

@@ -92,6 +92,9 @@ TEST_F(FileTest, RWBufferTypeStructError) {
   runFileTest("type.rwbuffer.struct.error.hlsl", Expect::Failure);
 }
 TEST_F(FileTest, CBufferType) { runFileTest("type.cbuffer.hlsl"); }
+TEST_F(FileTest, TypeCBufferIncludingResource) {
+  runFileTest("type.cbuffer.including.resource.hlsl");
+}
 TEST_F(FileTest, ConstantBufferType) {
   runFileTest("type.constant-buffer.hlsl");
 }