[spirv] Allow the same binding number for combined image sampler (#729)

Vulkan combined image sampler is logically considered a sampled
image and a sampler bound together.

docs/SPIR-V.rst

@@ -678,6 +678,13 @@ If there is no register specification, the corresponding resource will be
 assigned to the next available binding number, starting from 0, in descriptor
 set #0.
 
+Error checking
+~~~~~~~~~~~~~~
+
+Trying to reuse the same binding number of the same descriptor set results in
+a compiler error, unless we have exactly two resources and one is an image and
+the other is a sampler. This is to support the Vulkan combined image sampler.
+
 Summary
 ~~~~~~~
 
@@ -704,7 +711,7 @@ As an example, for the following code:
   [[vk::binding(3)]]
   RWBuffer<float4> rwbuffer1 : register(u5, space2);
 
-If we compile with ``-fvk-t-shift 0 10 -fvk-t-shift 1 20``:
+If we compile with ``-fvk-t-shift 10 0 -fvk-t-shift 20 1``:
 
 - ``rwbuffer1`` will take binding #3 in set #0, since explicit binding
   assignment has precedence over the rest.

tools/clang/lib/SPIRV/DeclResultIdMapper.cpp

@@ -44,6 +44,16 @@ const hlsl::RegisterAssignment *getResourceBinding(const NamedDecl *decl) {
   }
   return nullptr;
 }
+
+/// \brief Returns the resource category for the given type.
+ResourceVar::Category getResourceCategory(QualType type) {
+  if (TypeTranslator::isTexture(type) || TypeTranslator::isRWTexture(type))
+    return ResourceVar::Category::Image;
+  if (TypeTranslator::isSampler(type))
+    return ResourceVar::Category::Sampler;
+  return ResourceVar::Category::Other;
+}
+
 } // anonymous namespace
 
 bool DeclResultIdMapper::createStageOutputVar(const DeclaratorDecl *decl,
@@ -148,7 +158,8 @@ uint32_t DeclResultIdMapper::createExternVar(const VarDecl *var) {
   const uint32_t id = theBuilder.addModuleVar(varType, storageClass,
                                               var->getName(), llvm::None);
   astDecls[var] = {id, storageClass, rule};
-  resourceVars.emplace_back(id, getResourceBinding(var),
+  resourceVars.emplace_back(id, getResourceCategory(var->getType()),
+                            getResourceBinding(var),
                             var->getAttr<VKBindingAttr>());
 
   if (isACSBuffer) {
@@ -159,7 +170,8 @@ uint32_t DeclResultIdMapper::createExternVar(const VarDecl *var) {
     const uint32_t counterId =
         theBuilder.addModuleVar(counterType, storageClass, counterName);
 
-    resourceVars.emplace_back(counterId, nullptr, nullptr);
+    resourceVars.emplace_back(counterId, ResourceVar::Category::Other, nullptr,
+                              nullptr);
     counterVars[var] = counterId;
   }
 
@@ -225,7 +237,8 @@ uint32_t DeclResultIdMapper::createCTBuffer(const HLSLBufferDecl *decl) {
     astDecls[varDecl] = {bufferVar, spv::StorageClass::Uniform,
                          LayoutRule::GLSLStd140, index++};
   }
-  resourceVars.emplace_back(bufferVar, getResourceBinding(decl),
+  resourceVars.emplace_back(bufferVar, ResourceVar::Category::Other,
+                            getResourceBinding(decl),
                             decl->getAttr<VKBindingAttr>());
 
   return bufferVar;
@@ -247,7 +260,8 @@ uint32_t DeclResultIdMapper::createCTBuffer(const VarDecl *decl) {
   // TODO: std140 rules may not suit tbuffers.
   astDecls[decl] = {bufferVar, spv::StorageClass::Uniform,
                     LayoutRule::GLSLStd140};
-  resourceVars.emplace_back(bufferVar, getResourceBinding(context),
+  resourceVars.emplace_back(bufferVar, ResourceVar::Category::Other,
+                            getResourceBinding(context),
                             decl->getAttr<VKBindingAttr>());
 
   return bufferVar;
@@ -316,27 +330,32 @@ class BindingSet {
 public:
   BindingSet() : nextBinding(0) {}
 
-  /// Uses the given set and binding number.
-  void useBinding(uint32_t binding, uint32_t set) {
-    bindings[set].insert(binding);
+  /// Tries to use the given set and binding number. Returns true if possible,
+  /// false otherwise.
+  bool tryToUseBinding(uint32_t binding, uint32_t set,
+                       ResourceVar::Category category) {
+    const auto cat = static_cast<uint32_t>(category);
+    // Note that we will create the entry for binding in bindings[set] here.
+    // But that should not have bad effects since it defaults to zero.
+    if ((bindings[set][binding] & cat) == 0) {
+      bindings[set][binding] |= cat;
+      return true;
+    }
+    return false;
   }
 
   /// Uses the next avaiable binding number in set 0.
-  uint32_t useNextBinding() {
+  uint32_t useNextBinding(ResourceVar::Category category) {
     auto &set0bindings = bindings[0];
     while (set0bindings.count(nextBinding))
       nextBinding++;
-    set0bindings.insert(nextBinding);
+    set0bindings[nextBinding] = static_cast<uint32_t>(category);
     return nextBinding++;
   }
 
-  /// Returns true if the given set and binding number is already used.
-  bool isBindingUsed(uint32_t binding, uint32_t set) {
-    return bindings[set].count(binding);
-  }
-
 private:
-  std::unordered_map<uint32_t, llvm::SmallSet<uint32_t, 8>> bindings;
+  ///< set number -> (binding number -> resource category)
+  llvm::DenseMap<uint32_t, llvm::DenseMap<uint32_t, uint32_t>> bindings;
   uint32_t nextBinding; ///< Next available binding number in set 0
 };
 } // namespace
@@ -498,17 +517,17 @@ bool DeclResultIdMapper::decorateResourceBindings() {
   // Process variables with [[vk::binding(...)]] binding assignment
   for (const auto &var : resourceVars)
     if (const auto *vkBinding = var.getBinding()) {
+      const auto cat = var.getCategory();
       const auto set = vkBinding->getSet();
       const auto binding = vkBinding->getBinding();
 
-      if (bindingSet.isBindingUsed(binding, set)) {
+      if (bindingSet.tryToUseBinding(binding, set, cat)) {
+        theBuilder.decorateDSetBinding(var.getSpirvId(), set, binding);
+      } else {
         emitError("resource binding #%0 in descriptor set #%1 already assigned",
                   vkBinding->getLocation())
             << binding << set;
         noError = false;
-      } else {
-        theBuilder.decorateDSetBinding(var.getSpirvId(), set, binding);
-        bindingSet.useBinding(binding, set);
       }
     }
 
@@ -543,23 +562,24 @@ bool DeclResultIdMapper::decorateResourceBindings() {
           llvm_unreachable("unknown register type found");
         }
 
-        if (bindingSet.isBindingUsed(binding, set)) {
+        const auto cat = var.getCategory();
+
+        if (bindingSet.tryToUseBinding(binding, set, cat)) {
+          theBuilder.decorateDSetBinding(var.getSpirvId(), set, binding);
+        } else {
           emitError(
               "resource binding #%0 in descriptor set #%1 already assigned",
               reg->Loc)
               << binding << set;
           noError = false;
-        } else {
-          theBuilder.decorateDSetBinding(var.getSpirvId(), set, binding);
-          bindingSet.useBinding(binding, set);
         }
       }
 
   // Process variables with no binding assignment
   for (const auto &var : resourceVars)
     if (!var.getBinding() && !var.getRegister())
-      theBuilder.decorateDSetBinding(var.getSpirvId(), 0,
-                                     bindingSet.useNextBinding());
+      theBuilder.decorateDSetBinding(
+          var.getSpirvId(), 0, bindingSet.useNextBinding(var.getCategory()));
 
   return noError;
 }

tools/clang/lib/SPIRV/DeclResultIdMapper.h

@@ -85,16 +85,31 @@ private:
 
 class ResourceVar {
 public:
-  ResourceVar(uint32_t id, const hlsl::RegisterAssignment *r,
+  /// The category of this resource.
+  ///
+  /// We only care about Vulkan image and sampler types here, since they can be
+  /// bundled together as a combined image sampler which takes the same binding
+  /// number. The compiler should allow this case.
+  ///
+  /// Numbers are assigned to make bit check easiser.
+  enum class Category : uint32_t {
+    Image = 1,
+    Sampler = 2,
+    Other = 3,
+  };
+
+  ResourceVar(uint32_t id, Category cat, const hlsl::RegisterAssignment *r,
               const VKBindingAttr *b)
-      : varId(id), reg(r), binding(b) {}
+      : varId(id), category(cat), reg(r), binding(b) {}
 
   uint32_t getSpirvId() const { return varId; }
+  Category getCategory() const { return category; }
   const hlsl::RegisterAssignment *getRegister() const { return reg; }
   const VKBindingAttr *getBinding() const { return binding; }
 
 private:
   uint32_t varId;                      ///< <result-id>
+  Category category;                   ///< Resource category
   const hlsl::RegisterAssignment *reg; ///< HLSL register assignment
   const VKBindingAttr *binding;        ///< Vulkan binding assignment
 };

tools/clang/lib/SPIRV/TypeTranslator.cpp

@@ -402,6 +402,15 @@ bool TypeTranslator::isTextureMS(QualType type) {
   return false;
 }
 
+bool TypeTranslator::isSampler(QualType type) {
+  if (const auto *rt = type->getAs<RecordType>()) {
+    const auto name = rt->getDecl()->getName();
+    if (name == "SamplerState" || name == "SamplerComparisonState")
+      return true;
+  }
+  return false;
+}
+
 bool TypeTranslator::isVectorType(QualType type, QualType *elemType,
                                   uint32_t *elemCount) {
   bool isVec = false;

tools/clang/lib/SPIRV/TypeTranslator.h

@@ -85,6 +85,9 @@ public:
   /// \brief Returns true if the given type is an HLSL RWTexture type.
   static bool isRWTexture(QualType);
 
+  /// \brief Returns true if the given type is an HLSL sampler type.
+  static bool isSampler(QualType);
+
   /// \brief Returns true if the given type is an HLSL OutputPatch type.
   static bool isOutputPatch(QualType);
 

tools/clang/test/CodeGenSPIRV/vk.binding.explicit.error.hlsl

@@ -6,19 +6,36 @@ SamplerState sampler1      : register(s1, space1);
 [[vk::binding(3, 1)]]
 SamplerState sampler2      : register(s2);
 
-[[vk::binding(1)]] // duplicate
+[[vk::binding(1)]] // reuse - allowed for combined image sampler
 Texture2D<float4> texture1;
 
-[[vk::binding(3, 1)]] // duplicate
+[[vk::binding(3, 1)]] // reuse - allowed for combined image sampler
 Texture3D<float4> texture2 : register(t0, space0);
 
-[[vk::binding(3, 1)]] // duplicate
+[[vk::binding(3, 1)]] // reuse - disallowed
 Texture3D<float4> texture3 : register(t0, space0);
 
+[[vk::binding(1)]] // reuse - disallowed
+SamplerState sampler3      : register(s1, space1);
+
+struct S { float f; };
+
+[[vk::binding(5)]]
+StructuredBuffer<S> buf1;
+
+[[vk::binding(5)]] // reuse - disallowed
+SamplerState sampler4;
+
+[[vk::binding(5)]] // reuse - disallowed
+Texture2D<float4> texture4;
+
 float4 main() : SV_Target {
     return 1.0;
 }
 
-// CHECK: :9:3: error: resource binding #1 in descriptor set #0 already assigned
-// CHECK: :12:3: error: resource binding #3 in descriptor set #1 already assigned
+// CHECK-NOT:  :9:{{%\d+}}: error: resource binding #1 in descriptor set #0 already assigned
+// CHECK-NOT: :12:{{%\d+}}: error: resource binding #3 in descriptor set #1 already assigned
 // CHECK: :15:3: error: resource binding #3 in descriptor set #1 already assigned
+// CHECK: :18:3: error: resource binding #1 in descriptor set #0 already assigned
+// CHECK: :26:3: error: resource binding #5 in descriptor set #0 already assigned
+// CHECK: :29:3: error: resource binding #5 in descriptor set #0 already assigned

tools/clang/test/CodeGenSPIRV/vk.binding.register.error.hlsl

@@ -7,13 +7,22 @@ struct S {
 ConstantBuffer<S>     myCbuffer1 : register(b0);
 ConstantBuffer<S>     myCbuffer2 : register(b0, space1);
 
-RWStructuredBuffer<S> mySBuffer1 : register(u0);         // duplicate
-RWStructuredBuffer<S> mySBuffer2 : register(u0, space1); // duplicate
+RWStructuredBuffer<S> mySBuffer1 : register(u0);         // reuse - disallowed
+RWStructuredBuffer<S> mySBuffer2 : register(u0, space1); // reuse - disallowed
 RWStructuredBuffer<S> mySBuffer3 : register(u0, space2);
 
+SamplerState mySampler1 : register(s5, space1);
+Texture2D    myTexture1 : register(t5, space1); // reuse - allowed
+
+Texture2D    myTexture2 : register(t6, space6);
+[[vk::binding(6, 6)]] // reuse - allowed
+SamplerState mySampler2;
+
 float4 main() : SV_Target {
     return 1.0;
 }
 
 // CHECK: :10:36: error: resource binding #0 in descriptor set #0 already assigned
-// CHECK: :11:36: error: resource binding #0 in descriptor set #1 already assigned
+// CHECK: :11:36: error: resource binding #0 in descriptor set #1 already assigned
+// CHECK-NOT: :15:{{%\d+}}: error: resource binding #5 in descriptor set #1 already assigned
+// CHECK-NOT: :18:{{%\d+}}: error: resource binding #6 in descriptor set #6 already assigned