[msl] Run binding remapper if collisions are allowed.
The MSL backend needs to run the binding remapper even if there are no
remappings in order to mark all the existing bindings as allowing
collisions. There maybe collision added by multiplanar that need to be
accounted for even if we aren't remapping.
Bug: 346174896
Change-Id: Ide85aa7802beb1b0443fc2b12787befcd8d56903
Reviewed-on: https://dawn-review.googlesource.com/c/dawn/+/192681
Commit-Queue: dan sinclair <dsinclair@chromium.org>
Reviewed-by: James Price <jrprice@google.com>
diff --git a/src/dawn/tests/end2end/ExternalTextureTests.cpp b/src/dawn/tests/end2end/ExternalTextureTests.cpp
index 896fb4e..c1c3bdf 100644
--- a/src/dawn/tests/end2end/ExternalTextureTests.cpp
+++ b/src/dawn/tests/end2end/ExternalTextureTests.cpp
@@ -1334,9 +1334,6 @@
// Regression test for issue 346174896.
TEST_P(ExternalTextureTests, Regression346174896) {
- // TODO(346174896): Remove suppression once the issue in Tint MSL AST is fixed.
- DAWN_SUPPRESS_TEST_IF(IsMetal());
-
auto wgslModule = utils::CreateShaderModule(device, R"(
@vertex fn vertexMain() -> @builtin(position) vec4f {
return vec4f(1);
@@ -1345,12 +1342,9 @@
@group(0) @binding(1) var<storage, read_write> dimension : vec2u;
@group(0) @binding(0) var t : texture_external;
- @fragment fn main(@builtin(position) FragCoord : vec4f)
- -> @location(0) vec4f {
- dimension = textureDimensions(t);
-
- var coords = textureDimensions(t) / 2 + vec2u(FragCoord.xy) - vec2(1, 1);
- return textureLoad(t, coords);
+ @fragment fn main(@builtin(position) FragCoord : vec4f) -> @location(0) vec4f {
+ _ = dimension;
+ return textureLoad(t, vec2u(0, 0));
})");
utils::ComboRenderPipelineDescriptor descriptor;
diff --git a/src/tint/lang/wgsl/ast/transform/binding_remapper.cc b/src/tint/lang/wgsl/ast/transform/binding_remapper.cc
index d89d8d8..96316fd 100644
--- a/src/tint/lang/wgsl/ast/transform/binding_remapper.cc
+++ b/src/tint/lang/wgsl/ast/transform/binding_remapper.cc
@@ -71,7 +71,8 @@
return resolver::Resolve(b);
}
- if (remappings->binding_points.empty() && remappings->access_controls.empty()) {
+ if (!remappings->allow_collisions && remappings->binding_points.empty() &&
+ remappings->access_controls.empty()) {
return SkipTransform;
}
diff --git a/src/tint/lang/wgsl/ast/transform/binding_remapper_test.cc b/src/tint/lang/wgsl/ast/transform/binding_remapper_test.cc
index 1fb96a2..30697f0 100644
--- a/src/tint/lang/wgsl/ast/transform/binding_remapper_test.cc
+++ b/src/tint/lang/wgsl/ast/transform/binding_remapper_test.cc
@@ -41,11 +41,23 @@
DataMap data;
data.Add<BindingRemapper::Remappings>(BindingRemapper::BindingPoints{},
- BindingRemapper::AccessControls{});
+ BindingRemapper::AccessControls{},
+ /* allow collisions */ false);
EXPECT_FALSE(ShouldRun<BindingRemapper>(src, data));
}
+TEST_F(BindingRemapperTest, ShouldRunEmptyRemappingsWithCollisions) {
+ auto* src = R"()";
+
+ DataMap data;
+ data.Add<BindingRemapper::Remappings>(BindingRemapper::BindingPoints{},
+ BindingRemapper::AccessControls{},
+ /* allow collisions */ true);
+
+ EXPECT_TRUE(ShouldRun<BindingRemapper>(src, data));
+}
+
TEST_F(BindingRemapperTest, ShouldRunBindingPointRemappings) {
auto* src = R"()";
@@ -71,7 +83,7 @@
EXPECT_TRUE(ShouldRun<BindingRemapper>(src, data));
}
-TEST_F(BindingRemapperTest, NoRemappings) {
+TEST_F(BindingRemapperTest, NoRemappingsWithoutCollisions) {
auto* src = R"(
struct S {
a : f32,
@@ -90,7 +102,46 @@
DataMap data;
data.Add<BindingRemapper::Remappings>(BindingRemapper::BindingPoints{},
- BindingRemapper::AccessControls{});
+ BindingRemapper::AccessControls{},
+ /* allow_collisions */ false);
+ auto got = Run<BindingRemapper>(src, data);
+
+ EXPECT_EQ(expect, str(got));
+}
+
+TEST_F(BindingRemapperTest, NoRemappingsWithCollisions) {
+ auto* src = R"(
+struct S {
+ a : f32,
+}
+
+@group(2) @binding(1) var<storage, read> a : S;
+
+@group(3) @binding(2) var<storage, read> b : S;
+
+@compute @workgroup_size(1)
+fn f() {
+}
+)";
+
+ auto* expect = R"(
+struct S {
+ a : f32,
+}
+
+@internal(disable_validation__binding_point_collision) @group(2) @binding(1) var<storage, read> a : S;
+
+@internal(disable_validation__binding_point_collision) @group(3) @binding(2) var<storage, read> b : S;
+
+@compute @workgroup_size(1)
+fn f() {
+}
+)";
+
+ DataMap data;
+ data.Add<BindingRemapper::Remappings>(BindingRemapper::BindingPoints{},
+ BindingRemapper::AccessControls{},
+ /* allow_collisions */ true);
auto got = Run<BindingRemapper>(src, data);
EXPECT_EQ(expect, str(got));
