[spirv] Stop checking descriptor binding number overlap (#1210)

The Vulkan 1.1.72 spec says:

> It is valid for multiple shader variables to be assigned the same
> descriptor set and binding values, as long as all those that are
> statically used by the entry point being compiled are compatible
> with the descriptor type in the descriptor set layout binding.

tools/clang/lib/SPIRV/DeclResultIdMapper.cpp

@@ -37,15 +37,6 @@ 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;
-}
-
 /// \brief Returns true if the given declaration has a primitive type qualifier.
 /// Returns false otherwise.
 inline bool hasGSPrimitiveTypeQualifier(const Decl *decl) {
@@ -436,8 +427,7 @@ SpirvEvalInfo DeclResultIdMapper::createExternVar(const VarDecl *var) {
   const auto *bindingAttr = var->getAttr<VKBindingAttr>();
   const auto *counterBindingAttr = var->getAttr<VKCounterBindingAttr>();
 
-  resourceVars.emplace_back(id, getResourceCategory(var->getType()), regAttr,
-                            bindingAttr, counterBindingAttr);
+  resourceVars.emplace_back(id, regAttr, bindingAttr, counterBindingAttr);
 
   if (const auto *inputAttachment = var->getAttr<VKInputAttachmentIndexAttr>())
     theBuilder.decorateInputAttachmentIndex(id, inputAttachment->getIndex());
@@ -577,9 +567,9 @@ uint32_t DeclResultIdMapper::createCTBuffer(const HLSLBufferDecl *decl) {
                                              : spirvOptions.tBufferLayoutRule);
     astDecls[varDecl].indexInCTBuffer = index++;
   }
-  resourceVars.emplace_back(
-      bufferVar, ResourceVar::Category::Other, getResourceBinding(decl),
-      decl->getAttr<VKBindingAttr>(), decl->getAttr<VKCounterBindingAttr>());
+  resourceVars.emplace_back(bufferVar, getResourceBinding(decl),
+                            decl->getAttr<VKBindingAttr>(),
+                            decl->getAttr<VKCounterBindingAttr>());
 
   return bufferVar;
 }
@@ -615,9 +605,9 @@ uint32_t DeclResultIdMapper::createCTBuffer(const VarDecl *decl) {
           .setStorageClass(spv::StorageClass::Uniform)
           .setLayoutRule(context->isCBuffer() ? spirvOptions.cBufferLayoutRule
                                               : spirvOptions.tBufferLayoutRule);
-  resourceVars.emplace_back(
-      bufferVar, ResourceVar::Category::Other, getResourceBinding(context),
-      decl->getAttr<VKBindingAttr>(), decl->getAttr<VKCounterBindingAttr>());
+  resourceVars.emplace_back(bufferVar, getResourceBinding(context),
+                            decl->getAttr<VKBindingAttr>(),
+                            decl->getAttr<VKCounterBindingAttr>());
 
   return bufferVar;
 }
@@ -652,8 +642,7 @@ void DeclResultIdMapper::createGlobalsCBuffer(const VarDecl *var) {
       context, /*arraySize*/ 0, ContextUsageKind::Globals, "type.$Globals",
       "$Globals");
 
-  resourceVars.emplace_back(globals, ResourceVar::Category::Other, nullptr,
-                            nullptr, nullptr);
+  resourceVars.emplace_back(globals, nullptr, nullptr, nullptr);
 
   uint32_t index = 0;
   for (const auto *decl : typeTranslator.collectDeclsInDeclContext(context))
@@ -760,8 +749,7 @@ void DeclResultIdMapper::createCounterVar(
   if (!isAlias) {
     // Non-alias counter variables should be put in to resourceVars so that
     // descriptors can be allocated for them.
-    resourceVars.emplace_back(counterId, ResourceVar::Category::Other,
-                              getResourceBinding(decl),
+    resourceVars.emplace_back(counterId, getResourceBinding(decl),
                               decl->getAttr<VKBindingAttr>(),
                               decl->getAttr<VKCounterBindingAttr>(), true);
     assert(declId);
@@ -861,39 +849,24 @@ private:
 /// set and binding number.
 class BindingSet {
 public:
-  /// Tries to use the given set and binding number. Returns true if possible,
-  /// false otherwise and writes the source location of where the binding number
-  /// is used to *usedLoc.
-  bool tryToUseBinding(uint32_t binding, uint32_t set,
-                       ResourceVar::Category category, SourceLocation tryLoc,
-                       SourceLocation *usedLoc) {
-    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 ((usedBindings[set][binding] & cat) == 0) {
-      usedBindings[set][binding] |= cat;
-      whereUsed[set][binding] = tryLoc;
-      return true;
-    }
-    *usedLoc = whereUsed[set][binding];
-    return false;
+  /// Uses the given set and binding number.
+  void useBinding(uint32_t binding, uint32_t set) {
+    usedBindings[set].insert(binding);
   }
 
   /// Uses the next avaiable binding number in set 0.
-  uint32_t useNextBinding(uint32_t set, ResourceVar::Category category) {
+  uint32_t useNextBinding(uint32_t set) {
     auto &binding = usedBindings[set];
     auto &next = nextBindings[set];
     while (binding.count(next))
       ++next;
-    binding[next] = static_cast<uint32_t>(category);
+    binding.insert(next);
     return next++;
   }
 
 private:
-  ///< set number -> (binding number -> resource category)
-  llvm::DenseMap<uint32_t, llvm::DenseMap<uint32_t, uint32_t>> usedBindings;
-  ///< set number -> (binding number -> source location)
-  llvm::DenseMap<uint32_t, llvm::DenseMap<uint32_t, SourceLocation>> whereUsed;
+  ///< set number -> set of used binding number
+  llvm::DenseMap<uint32_t, llvm::DenseSet<uint32_t>> usedBindings;
   ///< set number -> next available binding number
   llvm::DenseMap<uint32_t, uint32_t> nextBindings;
 };
@@ -1086,19 +1059,11 @@ bool DeclResultIdMapper::decorateResourceBindings() {
   BindingSet bindingSet;
 
   // Decorates the given varId of the given category with set number
-  // setNo, binding number bindingNo. Emits warning if overlap.
-  const auto tryToDecorate = [this, &bindingSet](
-                                 const uint32_t varId, const uint32_t setNo,
-                                 const uint32_t bindingNo,
-                                 const ResourceVar::Category cat,
-                                 SourceLocation loc) {
-    SourceLocation prevUseLoc;
-    if (!bindingSet.tryToUseBinding(bindingNo, setNo, cat, loc, &prevUseLoc)) {
-      emitWarning("resource binding #%0 in descriptor set #%1 already assigned",
-                  loc)
-          << bindingNo << setNo;
-      emitNote("binding number previously assigned here", prevUseLoc);
-    }
+  // setNo, binding number bindingNo. Ignores overlaps.
+  const auto tryToDecorate = [this, &bindingSet](const uint32_t varId,
+                                                 const uint32_t setNo,
+                                                 const uint32_t bindingNo) {
+    bindingSet.useBinding(bindingNo, setNo);
     theBuilder.decorateDSetBinding(varId, setNo, bindingNo);
   };
 
@@ -1112,15 +1077,13 @@ bool DeclResultIdMapper::decorateResourceBindings() {
         if (const auto *reg = var.getRegister())
           set = reg->RegisterSpace;
 
-        tryToDecorate(var.getSpirvId(), set, vkCBinding->getBinding(),
-                      var.getCategory(), vkCBinding->getLocation());
+        tryToDecorate(var.getSpirvId(), set, vkCBinding->getBinding());
       }
     } else {
       if (const auto *vkBinding = var.getBinding()) {
         // Process m1
         tryToDecorate(var.getSpirvId(), vkBinding->getSet(),
-                      vkBinding->getBinding(), var.getCategory(),
-                      vkBinding->getLocation());
+                      vkBinding->getBinding());
       }
     }
   }
@@ -1156,12 +1119,10 @@ bool DeclResultIdMapper::decorateResourceBindings() {
           llvm_unreachable("unknown register type found");
         }
 
-        tryToDecorate(var.getSpirvId(), set, binding, var.getCategory(),
-                      reg->Loc);
+        tryToDecorate(var.getSpirvId(), set, binding);
       }
 
   for (const auto &var : resourceVars) {
-    const auto cat = var.getCategory();
     if (var.isCounter()) {
       if (!var.getCounterBinding()) {
         // Process mX * c2
@@ -1172,12 +1133,12 @@ bool DeclResultIdMapper::decorateResourceBindings() {
           set = reg->RegisterSpace;
 
         theBuilder.decorateDSetBinding(var.getSpirvId(), set,
-                                       bindingSet.useNextBinding(set, cat));
+                                       bindingSet.useNextBinding(set));
       }
     } else if (!var.getBinding() && !var.getRegister()) {
       // Process m3
       theBuilder.decorateDSetBinding(var.getSpirvId(), 0,
-                                     bindingSet.useNextBinding(0, cat));
+                                     bindingSet.useNextBinding(0));
     }
   }
 

tools/clang/lib/SPIRV/DeclResultIdMapper.h

@@ -103,27 +103,13 @@ private:
 
 class ResourceVar {
 public:
-  /// 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,
+  ResourceVar(uint32_t id, const hlsl::RegisterAssignment *r,
               const VKBindingAttr *b, const VKCounterBindingAttr *cb,
               bool counter = false)
-      : varId(id), category(cat), reg(r), binding(b), counterBinding(cb),
+      : varId(id), reg(r), binding(b), counterBinding(cb),
         isCounterVar(counter) {}
 
   uint32_t getSpirvId() const { return varId; }
-  Category getCategory() const { return category; }
   const hlsl::RegisterAssignment *getRegister() const { return reg; }
   const VKBindingAttr *getBinding() const { return binding; }
   bool isCounter() const { return isCounterVar; }
@@ -131,7 +117,6 @@ public:
 
 private:
   uint32_t varId;                             ///< <result-id>
-  Category category;                          ///< Resource category
   const hlsl::RegisterAssignment *reg;        ///< HLSL register assignment
   const VKBindingAttr *binding;               ///< Vulkan binding assignment
   const VKCounterBindingAttr *counterBinding; ///< Vulkan counter binding

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

@@ -1,30 +0,0 @@
-// Run: %dxc -T ps_6_0 -E main -fvk-b-shift 2 0 -fvk-t-shift 2 0 -fvk-s-shift 3 0 -fvk-u-shift 3 0
-
-struct S {
-    float4 f;
-};
-
-[[vk::binding(2)]]
-ConstantBuffer<S> cbuffer3;
-
-ConstantBuffer<S> cbuffer1 : register(b0); // Collision with cbuffer3 after shift
-
-Texture2D<float4> texture1: register(t0, space1);
-Texture2D<float4> texture2: register(t0); // Collision with cbuffer3 after shift
-
-SamplerState sampler1: register(s0);
-SamplerState sampler2: register(s0, space2);
-
-RWBuffer<float4> rwbuffer1 : register(u0, space3);
-RWBuffer<float4> rwbuffer2 : register(u0); // Collision with sampler1 after shift
-
-float4 main() : SV_Target {
-    return cbuffer1.f;
-}
-
-//CHECK: :10:30: warning: resource binding #2 in descriptor set #0 already assigned
-//CHECK:   :7:3: note: binding number previously assigned here
-
-//CHECK: :13:29: warning: resource binding #2 in descriptor set #0 already assigned
-
-//CHECK: :19:30: warning: resource binding #3 in descriptor set #0 already assigned

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

@@ -1,42 +0,0 @@
-// Run: %dxc -T ps_6_0 -E main
-
-[[vk::binding(1)]]
-SamplerState sampler1      : register(s1, space1);
-
-[[vk::binding(3, 1)]]
-SamplerState sampler2      : register(s2);
-
-[[vk::binding(1)]] // reuse - allowed for combined image sampler
-Texture2D<float4> texture1;
-
-[[vk::binding(3, 1)]] // reuse - allowed for combined image sampler
-Texture3D<float4> texture2 : register(t0, space0);
-
-[[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-NOT:  :9:{{%\d+}}: warning: resource binding #1 in descriptor set #0 already assigned
-// CHECK-NOT: :12:{{%\d+}}: warning: resource binding #3 in descriptor set #1 already assigned
-// CHECK: :15:3: warning: resource binding #3 in descriptor set #1 already assigned
-// CHECK: :12:3: note: binding number previously assigned here
-// CHECK: :18:3: warning: resource binding #1 in descriptor set #0 already assigned
-// CHECK: :26:3: warning: resource binding #5 in descriptor set #0 already assigned
-// CHECK: :29:3: warning: resource binding #5 in descriptor set #0 already assigned

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

@@ -1,29 +0,0 @@
-// Run: %dxc -T ps_6_0 -E main
-
-struct S {
-    float4 f;
-};
-
-ConstantBuffer<S>     myCbuffer1 : register(b0);
-ConstantBuffer<S>     myCbuffer2 : register(b0, space1);
-
-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: warning: resource binding #0 in descriptor set #0 already assigned
-// CHECK:  :7:36: note: binding number previously assigned here
-// CHECK: :11:36: warning: resource binding #0 in descriptor set #1 already assigned
-// CHECK-NOT: :15:{{%\d+}}: warning: resource binding #5 in descriptor set #1 already assigned
-// CHECK-NOT: :18:{{%\d+}}: warning: resource binding #6 in descriptor set #6 already assigned

tools/clang/unittests/SPIRV/CodeGenSPIRVTest.cpp

@@ -1321,15 +1321,6 @@ TEST_F(FileTest, VulkanRegisterBindingShift) {
   // command line option
   runFileTest("vk.binding.cl.hlsl");
 }
-TEST_F(FileTest, VulkanExplicitBindingReassigned) {
-  runFileTest("vk.binding.explicit.error.hlsl", Expect::Warning);
-}
-TEST_F(FileTest, VulkanRegisterBindingReassigned) {
-  runFileTest("vk.binding.register.error.hlsl", Expect::Warning);
-}
-TEST_F(FileTest, VulkanRegisterBindingShiftReassigned) {
-  runFileTest("vk.binding.cl.error.hlsl", Expect::Warning);
-}
 TEST_F(FileTest, VulkanStructuredBufferCounter) {
   // [[vk::counter_binding()]] for RWStructuredBuffer, AppendStructuredBuffer,
   // and ConsumeStructuredBuffer