Add DuplicateStorageStruct transform
This transform is currently required by Dawn for it's GLSL backend so
that SPIRV-Cross does not end up renaming the structures internally,
which it does to fix aliasing problems. We can remove this transform
if/once Dawn using binding numbers rather than names for uniform/storage
buffer data.
Bug: tint:386
Bug: tint:808
CloneContext: allow insert before/after of a node marked for removal
Change-Id: I3ff3b37bca62db07d5c759250dd4777e279b7a4f
Reviewed-on: https://dawn-review.googlesource.com/c/tint/+/51403
Commit-Queue: Ben Clayton <bclayton@google.com>
Kokoro: Kokoro <noreply+kokoro@google.com>
Reviewed-by: Ben Clayton <bclayton@google.com>
diff --git a/src/BUILD.gn b/src/BUILD.gn
index 8c0a9fe..89c5b96 100644
--- a/src/BUILD.gn
+++ b/src/BUILD.gn
@@ -509,6 +509,8 @@
"transform/canonicalize_entry_point_io.h",
"transform/decompose_storage_access.cc",
"transform/decompose_storage_access.h",
+ "transform/duplicate_storage_structs.cc",
+ "transform/duplicate_storage_structs.h",
"transform/external_texture_transform.cc",
"transform/external_texture_transform.h",
"transform/first_index_offset.cc",
diff --git a/src/CMakeLists.txt b/src/CMakeLists.txt
index 9927bdd..f9f5a82 100644
--- a/src/CMakeLists.txt
+++ b/src/CMakeLists.txt
@@ -273,6 +273,8 @@
transform/canonicalize_entry_point_io.h
transform/decompose_storage_access.cc
transform/decompose_storage_access.h
+ transform/duplicate_storage_structs.cc
+ transform/duplicate_storage_structs.h
transform/external_texture_transform.cc
transform/external_texture_transform.h
transform/first_index_offset.cc
@@ -800,6 +802,7 @@
transform/calculate_array_length_test.cc
transform/canonicalize_entry_point_io_test.cc
transform/decompose_storage_access_test.cc
+ transform/duplicate_storage_structs_test.cc
transform/external_texture_transform_test.cc
transform/first_index_offset_test.cc
transform/renamer_test.cc
diff --git a/src/transform/duplicate_storage_structs.cc b/src/transform/duplicate_storage_structs.cc
new file mode 100644
index 0000000..42ffccc
--- /dev/null
+++ b/src/transform/duplicate_storage_structs.cc
@@ -0,0 +1,126 @@
+// Copyright 2021 The Tint Authors.
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+#include "src/transform/duplicate_storage_structs.h"
+
+#include <unordered_set>
+#include <utility>
+#include <vector>
+
+#include "src/program_builder.h"
+#include "src/sem/variable.h"
+#include "src/utils/get_or_create.h"
+#include "src/utils/hash.h"
+
+namespace {
+struct StructAccessPair {
+ tint::sem::Struct const* str;
+ tint::ast::AccessControl::Access access;
+
+ bool operator==(const StructAccessPair& rhs) const {
+ return str == rhs.str && access == rhs.access;
+ }
+};
+} // namespace
+
+namespace std {
+/// Custom std::hash specialization for StructAccessPair so
+/// StructAccessPairs can be used as keys for std::unordered_map and
+/// std::unordered_set.
+template <>
+class hash<StructAccessPair> {
+ public:
+ /// @param struct_access_pair the StructAccessPair to create a hash for
+ /// @return the hash value
+ inline std::size_t operator()(
+ const StructAccessPair& struct_access_pair) const {
+ return tint::utils::Hash(struct_access_pair.str, struct_access_pair.access);
+ }
+};
+} // namespace std
+
+namespace tint {
+namespace transform {
+DuplicateStorageStructs::DuplicateStorageStructs() = default;
+DuplicateStorageStructs::~DuplicateStorageStructs() = default;
+
+Output DuplicateStorageStructs::Run(const Program* in, const DataMap&) {
+ ProgramBuilder out;
+ CloneContext ctx(&out, in);
+
+ // Create struct duplicates of storage buffers per access type.
+ // We do this by finding all storage variables, and creating a copy of the
+ // Struct they point to per struct/access pair.
+
+ std::unordered_map<StructAccessPair, ast::Struct*> sap_to_new_struct;
+
+ for (auto* node : in->ASTNodes().Objects()) {
+ if (auto* ast_var = node->As<ast::Variable>()) {
+ auto* var = in->Sem().Get(ast_var);
+ if (auto* str = var->Type()->UnwrapRef()->As<sem::Struct>()) {
+ // Skip non-storage structs
+ if (!str->UsedAs(ast::StorageClass::kStorage)) {
+ continue;
+ }
+
+ auto sap = StructAccessPair{str, var->AccessControl()};
+
+ auto* new_str = utils::GetOrCreate(sap_to_new_struct, sap, [&] {
+ // For this ast::Struct/Access pair, create a clone of the existing
+ // ast::Struct with a new name.
+ auto* old_str = str->Declaration();
+ std::string old_name = ctx.src->Symbols().NameFor(old_str->name());
+
+ auto* new_struct = [&] {
+ Symbol new_name = ctx.dst->Symbols().New(old_name);
+ auto new_members = ctx.Clone(old_str->members());
+ auto new_decorations = ctx.Clone(old_str->decorations());
+ return ctx.dst->create<ast::Struct>(new_name, new_members,
+ new_decorations);
+ }();
+
+ // Insert it after the original struct
+ ctx.InsertAfter(ctx.src->AST().GlobalDeclarations(),
+ str->Declaration(), new_struct);
+
+ // Remove the original struct
+ ctx.Remove(ctx.src->AST().GlobalDeclarations(), old_str);
+
+ return new_struct;
+ });
+
+ // Replace the existing variable with a new one who's type is an
+ // AccessControl<TypeName<"New Struct">>.
+ auto* new_var = [&] {
+ auto new_name = ctx.Clone(ast_var->symbol());
+ auto* new_ty = ctx.dst->ty.access(
+ var->AccessControl(), ctx.dst->ty.type_name(new_str->name()));
+ auto* new_constructor = ctx.Clone(ast_var->constructor());
+ auto new_decorations = ctx.Clone(ast_var->decorations());
+ return ctx.dst->create<ast::Variable>(
+ new_name, ast_var->declared_storage_class(), new_ty,
+ ast_var->is_const(), new_constructor, new_decorations);
+ }();
+ ctx.Replace(ast_var, new_var);
+ }
+ }
+ }
+
+ ctx.Clone();
+
+ return Output(Program(std::move(out)));
+}
+
+} // namespace transform
+} // namespace tint
diff --git a/src/transform/duplicate_storage_structs.h b/src/transform/duplicate_storage_structs.h
new file mode 100644
index 0000000..f843977
--- /dev/null
+++ b/src/transform/duplicate_storage_structs.h
@@ -0,0 +1,54 @@
+// Copyright 2021 The Tint Authors.
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+#ifndef SRC_TRANSFORM_DUPLICATE_STORAGE_STRUCTS_H_
+#define SRC_TRANSFORM_DUPLICATE_STORAGE_STRUCTS_H_
+
+#include <string>
+#include <unordered_map>
+
+#include "src/transform/transform.h"
+
+namespace tint {
+namespace transform {
+
+/// DuplicateStorageStructs is a Transform that creates duplicates of storage
+/// structures, one per access control type.
+///
+/// This transform is currently required by Dawn for it's GLSL backend so that
+/// SPIRV-Cross does not end up renaming the structures internally, which it
+/// does to fix aliasing problems (see
+/// https://github.com/KhronosGroup/SPIRV-Cross/pull/799). We can remove this
+/// transform if/once Dawn using binding numbers rather than names for
+/// uniform/storage buffer data. See https://crbug.com/tint/386.
+///
+class DuplicateStorageStructs : public Transform {
+ public:
+ /// Constructor
+ DuplicateStorageStructs();
+
+ /// Destructor
+ ~DuplicateStorageStructs() override;
+
+ /// Runs the transform on `program`, returning the transformation result.
+ /// @param program the source program to transform
+ /// @param data optional extra transform-specific data
+ /// @returns the transformation result
+ Output Run(const Program* program, const DataMap& data = {}) override;
+};
+
+} // namespace transform
+} // namespace tint
+
+#endif // SRC_TRANSFORM_DUPLICATE_STORAGE_STRUCTS_H_
diff --git a/src/transform/duplicate_storage_structs_test.cc b/src/transform/duplicate_storage_structs_test.cc
new file mode 100644
index 0000000..ab58a1d
--- /dev/null
+++ b/src/transform/duplicate_storage_structs_test.cc
@@ -0,0 +1,275 @@
+// Copyright 2021 The Tint Authors.
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+#include "src/transform/duplicate_storage_structs.h"
+
+#include "src/transform/test_helper.h"
+namespace tint {
+namespace transform {
+namespace {
+
+using DuplicateStorageStructsTransformTest = TransformTest;
+
+TEST_F(DuplicateStorageStructsTransformTest, Simple) {
+ auto* src = R"(
+[[block]]
+ struct S {
+ a : f32;
+};
+
+[[group(0), binding(0)]] var<storage> r : [[access(read)]] S;
+[[group(0), binding(1)]] var<storage> w : [[access(write)]] S;
+
+[[stage(compute)]]
+ fn main() {
+}
+)";
+
+ auto* expect = R"(
+[[block]]
+struct S_1 {
+ a : f32;
+};
+
+[[block]]
+struct S_2 {
+ a : f32;
+};
+
+[[group(0), binding(0)]] var<storage> r : [[access(read)]] S_1;
+
+[[group(0), binding(1)]] var<storage> w : [[access(write)]] S_2;
+
+[[stage(compute)]]
+fn main() {
+}
+)";
+
+ auto got = Run<DuplicateStorageStructs>(src);
+ EXPECT_EQ(expect, str(got));
+}
+
+TEST_F(DuplicateStorageStructsTransformTest, MultipleStorageStructs) {
+ auto* src = R"(
+[[block]]
+ struct S1 {
+ a : f32;
+};
+
+[[block]]
+ struct S2 {
+ a : i32;
+};
+
+[[group(0), binding(0)]] var<storage> r1 : [[access(read)]] S1;
+[[group(0), binding(1)]] var<storage> w1 : [[access(write)]] S1;
+
+[[group(0), binding(2)]] var<storage> r2 : [[access(read)]] S2;
+[[group(0), binding(3)]] var<storage> w2 : [[access(write)]] S2;
+
+[[stage(compute)]]
+ fn main() {
+}
+)";
+
+ auto* expect = R"(
+[[block]]
+struct S1_1 {
+ a : f32;
+};
+
+[[block]]
+struct S1_2 {
+ a : f32;
+};
+
+[[block]]
+struct S2_1 {
+ a : i32;
+};
+
+[[block]]
+struct S2_2 {
+ a : i32;
+};
+
+[[group(0), binding(0)]] var<storage> r1 : [[access(read)]] S1_1;
+
+[[group(0), binding(1)]] var<storage> w1 : [[access(write)]] S1_2;
+
+[[group(0), binding(2)]] var<storage> r2 : [[access(read)]] S2_1;
+
+[[group(0), binding(3)]] var<storage> w2 : [[access(write)]] S2_2;
+
+[[stage(compute)]]
+fn main() {
+}
+)";
+
+ auto got = Run<DuplicateStorageStructs>(src);
+ EXPECT_EQ(expect, str(got));
+}
+
+TEST_F(DuplicateStorageStructsTransformTest, MultipleStorageVariables) {
+ auto* src = R"(
+[[block]]
+ struct S {
+ a : f32;
+};
+
+[[group(0), binding(0)]] var<storage> r1 : [[access(read)]] S;
+[[group(0), binding(1)]] var<storage> r2 : [[access(read)]] S;
+[[group(0), binding(2)]] var<storage> r3 : [[access(read)]] S;
+[[group(0), binding(3)]] var<storage> w1 : [[access(write)]] S;
+[[group(0), binding(4)]] var<storage> w2 : [[access(write)]] S;
+[[group(0), binding(5)]] var<storage> w3 : [[access(write)]] S;
+[[group(0), binding(3)]] var<storage> rw1 : [[access(read_write)]] S;
+[[group(0), binding(4)]] var<storage> rw2 : [[access(read_write)]] S;
+[[group(0), binding(5)]] var<storage> rw3 : [[access(read_write)]] S;
+
+[[stage(compute)]]
+ fn main() {
+}
+)";
+
+ auto* expect = R"(
+[[block]]
+struct S_1 {
+ a : f32;
+};
+
+[[block]]
+struct S_2 {
+ a : f32;
+};
+
+[[block]]
+struct S_3 {
+ a : f32;
+};
+
+[[group(0), binding(0)]] var<storage> r1 : [[access(read)]] S_1;
+
+[[group(0), binding(1)]] var<storage> r2 : [[access(read)]] S_1;
+
+[[group(0), binding(2)]] var<storage> r3 : [[access(read)]] S_1;
+
+[[group(0), binding(3)]] var<storage> w1 : [[access(write)]] S_2;
+
+[[group(0), binding(4)]] var<storage> w2 : [[access(write)]] S_2;
+
+[[group(0), binding(5)]] var<storage> w3 : [[access(write)]] S_2;
+
+[[group(0), binding(3)]] var<storage> rw1 : [[access(read_write)]] S_3;
+
+[[group(0), binding(4)]] var<storage> rw2 : [[access(read_write)]] S_3;
+
+[[group(0), binding(5)]] var<storage> rw3 : [[access(read_write)]] S_3;
+
+[[stage(compute)]]
+fn main() {
+}
+)";
+
+ auto got = Run<DuplicateStorageStructs>(src);
+ EXPECT_EQ(expect, str(got));
+}
+
+TEST_F(DuplicateStorageStructsTransformTest, MixedStructTypes) {
+ auto* src = R"(
+
+[[block]]
+struct S_1 {
+ a : f32;
+};
+
+[[block]]
+struct S_2 {
+ a : f32;
+};
+
+[[block]]
+struct S_3 {
+ a : f32;
+};
+
+[[group(0), binding(0)]] var<storage> r1 : [[access(read)]] S_1;
+
+[[group(0), binding(1)]] var<storage> r2 : [[access(read)]] S_1;
+
+[[group(0), binding(2)]] var<storage> r3 : [[access(read)]] S_1;
+
+[[group(0), binding(3)]] var<storage> w1 : [[access(write)]] S_2;
+
+[[group(0), binding(4)]] var<storage> w2 : [[access(write)]] S_2;
+
+[[group(0), binding(5)]] var<storage> w3 : [[access(write)]] S_2;
+
+[[group(0), binding(3)]] var<storage> rw1 : [[access(read_write)]] S_3;
+
+[[group(0), binding(4)]] var<storage> rw2 : [[access(read_write)]] S_3;
+
+[[group(0), binding(5)]] var<storage> rw3 : [[access(read_write)]] S_3;
+
+[[stage(compute)]]
+fn main() {
+}
+)";
+
+ auto* expect = R"(
+[[block]]
+struct S_1_1 {
+ a : f32;
+};
+
+[[block]]
+struct S_2_1 {
+ a : f32;
+};
+
+[[block]]
+struct S_3_1 {
+ a : f32;
+};
+
+[[group(0), binding(0)]] var<storage> r1 : [[access(read)]] S_1_1;
+
+[[group(0), binding(1)]] var<storage> r2 : [[access(read)]] S_1_1;
+
+[[group(0), binding(2)]] var<storage> r3 : [[access(read)]] S_1_1;
+
+[[group(0), binding(3)]] var<storage> w1 : [[access(write)]] S_2_1;
+
+[[group(0), binding(4)]] var<storage> w2 : [[access(write)]] S_2_1;
+
+[[group(0), binding(5)]] var<storage> w3 : [[access(write)]] S_2_1;
+
+[[group(0), binding(3)]] var<storage> rw1 : [[access(read_write)]] S_3_1;
+
+[[group(0), binding(4)]] var<storage> rw2 : [[access(read_write)]] S_3_1;
+
+[[group(0), binding(5)]] var<storage> rw3 : [[access(read_write)]] S_3_1;
+
+[[stage(compute)]]
+fn main() {
+}
+)";
+
+ auto got = Run<DuplicateStorageStructs>(src);
+ EXPECT_EQ(expect, str(got));
+}
+
+} // namespace
+} // namespace transform
+} // namespace tint
diff --git a/src/transform/spirv.cc b/src/transform/spirv.cc
index 20bdf68..75525cb 100644
--- a/src/transform/spirv.cc
+++ b/src/transform/spirv.cc
@@ -26,6 +26,7 @@
#include "src/sem/statement.h"
#include "src/sem/struct.h"
#include "src/sem/variable.h"
+#include "src/transform/duplicate_storage_structs.h"
#include "src/transform/external_texture_transform.h"
#include "src/transform/manager.h"
@@ -40,6 +41,7 @@
Output Spirv::Run(const Program* in, const DataMap& data) {
Manager manager;
manager.Add<ExternalTextureTransform>();
+ manager.Add<DuplicateStorageStructs>();
auto transformedInput = manager.Run(in, data);
auto* cfg = data.Get<Config>();
diff --git a/test/BUILD.gn b/test/BUILD.gn
index 867e8d7..616ab31 100644
--- a/test/BUILD.gn
+++ b/test/BUILD.gn
@@ -257,9 +257,9 @@
"../src/resolver/intrinsic_test.cc",
"../src/resolver/is_host_shareable_test.cc",
"../src/resolver/is_storeable_test.cc",
+ "../src/resolver/pipeline_overridable_constant_test.cc",
"../src/resolver/ptr_ref_test.cc",
"../src/resolver/ptr_ref_validation_test.cc",
- "../src/resolver/pipeline_overridable_constant_test.cc",
"../src/resolver/resolver_test.cc",
"../src/resolver/resolver_test_helper.cc",
"../src/resolver/resolver_test_helper.h",
@@ -299,6 +299,7 @@
"../src/transform/calculate_array_length_test.cc",
"../src/transform/canonicalize_entry_point_io_test.cc",
"../src/transform/decompose_storage_access_test.cc",
+ "../src/transform/duplicate_storage_structs_test.cc",
"../src/transform/external_texture_transform_test.cc",
"../src/transform/first_index_offset_test.cc",
"../src/transform/renamer_test.cc",
