BindingRemapper: Allow for binding point collisions

Add a new parameter to BindingRemapper::Remappings that allows resulting binding points to collide.

When enabled, the output of the transform contains two or more module-scoped variables with the same binding point, used by the same entry point, then these variables will be decorated with an internal decoration to disable validation for the collision.

This is to work around collisions generated for the HLSL backend where the variables actually exist in different register classes, which is permitted by D3D12.

The transform will only generate these decorations if it needs to.

Fixed: tint:797
Change-Id: Id8a87523801bd0cd0dd54227ebabd4299bc20c27
Reviewed-on: https://dawn-review.googlesource.com/c/tint/+/50742
Kokoro: Kokoro <noreply+kokoro@google.com>
Commit-Queue: Ben Clayton <bclayton@google.com>
Reviewed-by: James Price <jrprice@google.com>

src/ast/disable_validation_decoration.cc

@@ -32,6 +32,8 @@ std::string DisableValidationDecoration::Name() const {
   switch (validation_) {
     case DisabledValidation::kFunctionHasNoBody:
       return "disable_validation__function_has_no_body";
+    case DisabledValidation::kBindingPointCollision:
+      return "disable_validation__binding_point_collision";
   }
   return "<invalid>";
 }

src/ast/disable_validation_decoration.h

@@ -28,6 +28,9 @@ enum class DisabledValidation {
   /// When applied to a function, the validator will not complain there is no
   /// body to a function.
   kFunctionHasNoBody,
+  /// When applied to a module-scoped variable, the validator will not complain
+  /// if two resource variables have the same binding points.
+  kBindingPointCollision,
 };
 
 /// An internal decoration used to tell the validator to ignore specific

src/resolver/resolver.cc

@@ -552,7 +552,8 @@ bool Resolver::ValidateGlobalVariable(const VariableInfo* info) {
       if (!(deco->Is<ast::BindingDecoration>() ||
             deco->Is<ast::BuiltinDecoration>() ||
             deco->Is<ast::GroupDecoration>() ||
-            deco->Is<ast::LocationDecoration>())) {
+            deco->Is<ast::LocationDecoration>() ||
+            deco->Is<ast::InternalDecoration>())) {
         diagnostics_.add_error("decoration is not valid for variables",
                                deco->source());
         return false;
@@ -1030,7 +1031,13 @@ bool Resolver::ValidateEntryPoint(const ast::Function* func,
     }
     auto bp = var_info->binding_point;
     auto res = binding_points.emplace(bp, var_info->declaration);
-    if (!res.second) {
+    if (!res.second &&
+        !IsValidationDisabled(
+            var_info->declaration->decorations(),
+            ast::DisabledValidation::kBindingPointCollision) &&
+        !IsValidationDisabled(
+            res.first->second->decorations(),
+            ast::DisabledValidation::kBindingPointCollision)) {
       // https://gpuweb.github.io/gpuweb/wgsl/#resource-interface
       // Bindings must not alias within a shader stage: two different
       // variables in the resource interface of a given shader must not have

src/transform/binding_remapper.cc

@@ -14,10 +14,13 @@
 
 #include "src/transform/binding_remapper.h"
 
+#include <unordered_set>
 #include <utility>
 
+#include "src/ast/disable_validation_decoration.h"
 #include "src/program_builder.h"
 #include "src/sem/access_control_type.h"
+#include "src/sem/function.h"
 #include "src/sem/variable.h"
 
 TINT_INSTANTIATE_TYPEINFO(tint::transform::BindingRemapper::Remappings);
@@ -25,8 +28,12 @@ TINT_INSTANTIATE_TYPEINFO(tint::transform::BindingRemapper::Remappings);
 namespace tint {
 namespace transform {
 
-BindingRemapper::Remappings::Remappings(BindingPoints bp, AccessControls ac)
-    : binding_points(std::move(bp)), access_controls(std::move(ac)) {}
+BindingRemapper::Remappings::Remappings(BindingPoints bp,
+                                        AccessControls ac,
+                                        bool may_collide)
+    : binding_points(std::move(bp)),
+      access_controls(std::move(ac)),
+      allow_collisions(may_collide) {}
 
 BindingRemapper::Remappings::Remappings(const Remappings&) = default;
 BindingRemapper::Remappings::~Remappings() = default;
@@ -42,11 +49,52 @@ Output BindingRemapper::Run(const Program* in, const DataMap& datamap) {
         "BindingRemapper did not find the remapping data");
     return Output(Program(std::move(out)));
   }
-  CloneContext ctx(&out, in);
-  for (auto* var : in->AST().GlobalVariables()) {
-    if (auto binding_point = var->binding_point()) {
+
+  // A set of post-remapped binding points that need to be decorated with a
+  // DisableValidationDecoration to disable binding-point-collision validation
+  std::unordered_set<sem::BindingPoint> add_collision_deco;
+
+  if (remappings->allow_collisions) {
+    // Scan for binding point collisions generated by this transform.
+    // Populate all collisions in the `add_collision_deco` set.
+    for (auto* func_ast : in->AST().Functions()) {
+      if (!func_ast->IsEntryPoint()) {
+        continue;
+      }
+      auto* func = in->Sem().Get(func_ast);
+      std::unordered_map<sem::BindingPoint, int> binding_point_counts;
+      for (auto* var : func->ReferencedModuleVariables()) {
+        if (auto binding_point = var->Declaration()->binding_point()) {
           BindingPoint from{binding_point.group->value(),
                             binding_point.binding->value()};
+          auto bp_it = remappings->binding_points.find(from);
+          if (bp_it != remappings->binding_points.end()) {
+            // Remapped
+            BindingPoint to = bp_it->second;
+            if (binding_point_counts[to]++) {
+              add_collision_deco.emplace(to);
+            }
+          } else {
+            // No remapping
+            if (binding_point_counts[from]++) {
+              add_collision_deco.emplace(from);
+            }
+          }
+        }
+      }
+    }
+  }
+
+  CloneContext ctx(&out, in);
+
+  for (auto* var : in->AST().GlobalVariables()) {
+    if (auto binding_point = var->binding_point()) {
+      // The original binding point
+      BindingPoint from{binding_point.group->value(),
+                        binding_point.binding->value()};
+
+      // The binding point after remapping
+      BindingPoint bp = from;
 
       // Replace any group or binding decorations.
       // Note: This has to be performed *before* remapping access controls, as
@@ -56,8 +104,10 @@ Output BindingRemapper::Run(const Program* in, const DataMap& datamap) {
         BindingPoint to = bp_it->second;
         auto* new_group = out.create<ast::GroupDecoration>(to.group);
         auto* new_binding = out.create<ast::BindingDecoration>(to.binding);
+
         ctx.Replace(binding_point.group, new_group);
         ctx.Replace(binding_point.binding, new_binding);
+        bp = to;
       }
 
       // Replace any access controls.
@@ -78,6 +128,15 @@ Output BindingRemapper::Run(const Program* in, const DataMap& datamap) {
             ctx.Clone(var->constructor()), ctx.Clone(var->decorations()));
         ctx.Replace(var, new_var);
       }
+
+      // Add `DisableValidationDecoration`s if required
+      if (add_collision_deco.count(bp)) {
+        auto* decoration =
+            ctx.dst->ASTNodes().Create<ast::DisableValidationDecoration>(
+                ctx.dst->ID(), ast::DisabledValidation::kBindingPointCollision);
+        ctx.InsertBefore(var->decorations(), *var->decorations().begin(),
+                         decoration);
+      }
     }
   }
   ctx.Clone();

src/transform/binding_remapper.h

@@ -44,7 +44,9 @@ class BindingRemapper : public Transform {
     /// Constructor
     /// @param bp a map of new binding points
     /// @param ac a map of new access controls
-    Remappings(BindingPoints bp, AccessControls ac);
+    /// @param may_collide If true, then validation will be disabled for
+    /// binding point collisions generated by this transform
+    Remappings(BindingPoints bp, AccessControls ac, bool may_collide = true);
 
     /// Copy constructor
     Remappings(const Remappings&);
@@ -57,6 +59,10 @@ class BindingRemapper : public Transform {
 
     /// A map of old binding point to new access controls
     AccessControls const access_controls;
+
+    /// If true, then validation will be disabled for binding point collisions
+    /// generated by this transform
+    bool const allow_collisions;
   };
 
   /// Constructor

src/transform/binding_remapper_test.cc

@@ -241,6 +241,124 @@ fn f() {
   EXPECT_EQ(expect, str(got));
 }
 
+TEST_F(BindingRemapperTest, BindingCollisionsSameEntryPoint) {
+  auto* src = R"(
+[[block]]
+struct S {
+  i : i32;
+};
+
+[[group(2), binding(1)]] var<storage> a : [[access(read)]] S;
+
+[[group(3), binding(2)]] var<storage> b : [[access(read)]] S;
+
+[[group(4), binding(3)]] var<storage> c : [[access(read)]] S;
+
+[[group(5), binding(4)]] var<storage> d : [[access(read)]] S;
+
+[[stage(compute)]]
+fn f() {
+  let x : i32 = (((a.i + b.i) + c.i) + d.i);
+}
+)";
+
+  auto* expect = R"(
+[[block]]
+struct S {
+  i : i32;
+};
+
+[[internal(disable_validation__binding_point_collision), group(1), binding(1)]] var<storage> a : [[access(read)]] S;
+
+[[internal(disable_validation__binding_point_collision), group(1), binding(1)]] var<storage> b : [[access(read)]] S;
+
+[[internal(disable_validation__binding_point_collision), group(5), binding(4)]] var<storage> c : [[access(read)]] S;
+
+[[internal(disable_validation__binding_point_collision), group(5), binding(4)]] var<storage> d : [[access(read)]] S;
+
+[[stage(compute)]]
+fn f() {
+  let x : i32 = (((a.i + b.i) + c.i) + d.i);
+}
+)";
+
+  DataMap data;
+  data.Add<BindingRemapper::Remappings>(
+      BindingRemapper::BindingPoints{
+          {{2, 1}, {1, 1}},
+          {{3, 2}, {1, 1}},
+          {{4, 3}, {5, 4}},
+      },
+      BindingRemapper::AccessControls{}, true);
+  auto got = Run<BindingRemapper>(src, data);
+
+  EXPECT_EQ(expect, str(got));
+}
+
+TEST_F(BindingRemapperTest, BindingCollisionsDifferentEntryPoints) {
+  auto* src = R"(
+[[block]]
+struct S {
+  i : i32;
+};
+
+[[group(2), binding(1)]] var<storage> a : [[access(read)]] S;
+
+[[group(3), binding(2)]] var<storage> b : [[access(read)]] S;
+
+[[group(4), binding(3)]] var<storage> c : [[access(read)]] S;
+
+[[group(5), binding(4)]] var<storage> d : [[access(read)]] S;
+
+[[stage(compute)]]
+fn f1() {
+  let x : i32 = (a.i + c.i);
+}
+
+[[stage(compute)]]
+fn f2() {
+  let x : i32 = (b.i + d.i);
+}
+)";
+
+  auto* expect = R"(
+[[block]]
+struct S {
+  i : i32;
+};
+
+[[group(1), binding(1)]] var<storage> a : [[access(read)]] S;
+
+[[group(1), binding(1)]] var<storage> b : [[access(read)]] S;
+
+[[group(5), binding(4)]] var<storage> c : [[access(read)]] S;
+
+[[group(5), binding(4)]] var<storage> d : [[access(read)]] S;
+
+[[stage(compute)]]
+fn f1() {
+  let x : i32 = (a.i + c.i);
+}
+
+[[stage(compute)]]
+fn f2() {
+  let x : i32 = (b.i + d.i);
+}
+)";
+
+  DataMap data;
+  data.Add<BindingRemapper::Remappings>(
+      BindingRemapper::BindingPoints{
+          {{2, 1}, {1, 1}},
+          {{3, 2}, {1, 1}},
+          {{4, 3}, {5, 4}},
+      },
+      BindingRemapper::AccessControls{}, true);
+  auto got = Run<BindingRemapper>(src, data);
+
+  EXPECT_EQ(expect, str(got));
+}
+
 TEST_F(BindingRemapperTest, NoData) {
   auto* src = R"(
 [[block]]