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>
diff --git a/src/ast/disable_validation_decoration.cc b/src/ast/disable_validation_decoration.cc
index 829172e..ef93c0f 100644
--- a/src/ast/disable_validation_decoration.cc
+++ b/src/ast/disable_validation_decoration.cc
@@ -32,6 +32,8 @@
switch (validation_) {
case DisabledValidation::kFunctionHasNoBody:
return "disable_validation__function_has_no_body";
+ case DisabledValidation::kBindingPointCollision:
+ return "disable_validation__binding_point_collision";
}
return "<invalid>";
}
diff --git a/src/ast/disable_validation_decoration.h b/src/ast/disable_validation_decoration.h
index ec4821b..a825230 100644
--- a/src/ast/disable_validation_decoration.h
+++ b/src/ast/disable_validation_decoration.h
@@ -28,6 +28,9 @@
/// 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
diff --git a/src/resolver/resolver.cc b/src/resolver/resolver.cc
index cede2c5..b38c2d1 100644
--- a/src/resolver/resolver.cc
+++ b/src/resolver/resolver.cc
@@ -552,7 +552,8 @@
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 @@
}
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
diff --git a/src/transform/binding_remapper.cc b/src/transform/binding_remapper.cc
index 455a3b3..04e7739 100644
--- a/src/transform/binding_remapper.cc
+++ b/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 @@
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,12 +49,53 @@
"BindingRemapper did not find the remapping data");
return Output(Program(std::move(out)));
}
+
+ // 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
// `ctx.Clone(var->decorations())` depend on these replacements.
@@ -56,8 +104,10 @@
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 @@
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();
diff --git a/src/transform/binding_remapper.h b/src/transform/binding_remapper.h
index 1f004b0..81efa25 100644
--- a/src/transform/binding_remapper.h
+++ b/src/transform/binding_remapper.h
@@ -44,7 +44,9 @@
/// 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 @@
/// 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
diff --git a/src/transform/binding_remapper_test.cc b/src/transform/binding_remapper_test.cc
index bb7a20c..263d20b 100644
--- a/src/transform/binding_remapper_test.cc
+++ b/src/transform/binding_remapper_test.cc
@@ -241,6 +241,124 @@
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]]
