Factor out code to flatten bindings for msl

And use it in the fuzzers to fix ICE that occurs when bindings are not
flattened when processing MSL.

Bug: chromium:1314938
Change-Id: Ic35503e53395fad232487226c324067975291fbf
Reviewed-on: https://dawn-review.googlesource.com/c/dawn/+/88461
Kokoro: Kokoro <noreply+kokoro@google.com>
Commit-Queue: Antonio Maiorano <amaiorano@google.com>
Reviewed-by: James Price <jrprice@google.com>

src/tint/BUILD.gn

@@ -530,6 +530,8 @@ libtint_source_set("libtint_core_all_src") {
     "writer/append_vector.h",
     "writer/array_length_from_uniform_options.cc",
     "writer/array_length_from_uniform_options.h",
+    "writer/flatten_bindings.cc",
+    "writer/flatten_bindings.h",
     "writer/float_to_string.cc",
     "writer/float_to_string.h",
     "writer/generate_external_texture_bindings.cc",

src/tint/CMakeLists.txt

@@ -458,6 +458,8 @@ set(TINT_LIB_SRCS
   writer/append_vector.h
   writer/array_length_from_uniform_options.cc
   writer/array_length_from_uniform_options.h
+  writer/flatten_bindings.cc
+  writer/flatten_bindings.h
   writer/float_to_string.cc
   writer/float_to_string.h
   writer/generate_external_texture_bindings.cc
@@ -826,6 +828,7 @@ if(TINT_BUILD_TESTS)
     utils/unique_allocator_test.cc
     utils/unique_vector_test.cc
     writer/append_vector_test.cc
+    writer/flatten_bindings_test.cc
     writer/float_to_string_test.cc
     writer/generate_external_texture_bindings_test.cc
     writer/text_generator_test.cc

src/tint/cmd/main.cc

@@ -32,6 +32,7 @@
 #include "src/tint/utils/io/command.h"
 #include "src/tint/utils/string.h"
 #include "src/tint/val/val.h"
+#include "src/tint/writer/flatten_bindings.h"
 #include "tint/tint.h"
 
 namespace {
@@ -640,58 +641,12 @@ bool GenerateWgsl(const tint::Program* program, const Options& options) {
 /// @returns true on success
 bool GenerateMsl(const tint::Program* program, const Options& options) {
 #if TINT_BUILD_MSL_WRITER
-    const tint::Program* input_program = program;
-
     // Remap resource numbers to a flat namespace.
-    // TODO(crbug.com/tint/1101): Make this more robust for multiple entry points.
+    // TODO(crbug.com/tint/1501): Do this via Options::BindingMap.
-    using BindingPoint = tint::transform::BindingPoint;
+    const tint::Program* input_program = program;
-    tint::transform::BindingRemapper::BindingPoints binding_points;
+    auto flattened = tint::writer::FlattenBindings(program);
-    uint32_t next_buffer_idx = 0;
+    if (flattened) {
-    uint32_t next_sampler_idx = 0;
+        input_program = &*flattened;
-    uint32_t next_texture_idx = 0;
-
-    tint::inspector::Inspector inspector(program);
-    auto entry_points = inspector.GetEntryPoints();
-    for (auto& entry_point : entry_points) {
-        auto bindings = inspector.GetResourceBindings(entry_point.name);
-        for (auto& binding : bindings) {
-            BindingPoint src = {binding.bind_group, binding.binding};
-            if (binding_points.count(src)) {
-                continue;
-            }
-            switch (binding.resource_type) {
-                case tint::inspector::ResourceBinding::ResourceType::kUniformBuffer:
-                case tint::inspector::ResourceBinding::ResourceType::kStorageBuffer:
-                case tint::inspector::ResourceBinding::ResourceType::kReadOnlyStorageBuffer:
-                    binding_points.emplace(src, BindingPoint{0, next_buffer_idx++});
-                    break;
-                case tint::inspector::ResourceBinding::ResourceType::kSampler:
-                case tint::inspector::ResourceBinding::ResourceType::kComparisonSampler:
-                    binding_points.emplace(src, BindingPoint{0, next_sampler_idx++});
-                    break;
-                case tint::inspector::ResourceBinding::ResourceType::kSampledTexture:
-                case tint::inspector::ResourceBinding::ResourceType::kMultisampledTexture:
-                case tint::inspector::ResourceBinding::ResourceType::kWriteOnlyStorageTexture:
-                case tint::inspector::ResourceBinding::ResourceType::kDepthTexture:
-                case tint::inspector::ResourceBinding::ResourceType::kDepthMultisampledTexture:
-                case tint::inspector::ResourceBinding::ResourceType::kExternalTexture:
-                    binding_points.emplace(src, BindingPoint{0, next_texture_idx++});
-                    break;
-            }
-        }
-    }
-
-    // Run the binding remapper transform.
-    tint::transform::Output transform_output;
-    if (!binding_points.empty()) {
-        tint::transform::Manager manager;
-        tint::transform::DataMap inputs;
-        inputs.Add<tint::transform::BindingRemapper::Remappings>(
-            std::move(binding_points), tint::transform::BindingRemapper::AccessControls{},
-            /* mayCollide */ true);
-        manager.Add<tint::transform::BindingRemapper>();
-        transform_output = manager.Run(program, inputs);
-        input_program = &transform_output.program;
     }
 
     // TODO(jrprice): Provide a way for the user to set non-default options.

src/tint/fuzzers/tint_common_fuzzer.cc

@@ -31,6 +31,7 @@
 #include "src/tint/diagnostic/formatter.h"
 #include "src/tint/program.h"
 #include "src/tint/utils/hash.h"
+#include "src/tint/writer/flatten_bindings.h"
 
 namespace tint::fuzzers {
 
@@ -257,10 +258,18 @@ int CommonFuzzer::Run(const uint8_t* data, size_t size) {
         }
         case OutputFormat::kMSL: {
 #if TINT_BUILD_MSL_WRITER
-            auto result = writer::msl::Generate(&program, options_msl_);
+            // Remap resource numbers to a flat namespace.
+            // TODO(crbug.com/tint/1501): Do this via Options::BindingMap.
+            auto input_program = &program;
+            auto flattened = tint::writer::FlattenBindings(&program);
+            if (flattened) {
+                input_program = &*flattened;
+            }
+
+            auto result = writer::msl::Generate(input_program, options_msl_);
             generated_msl_ = std::move(result.msl);
             if (!result.success) {
-                VALIDITY_ERROR(program.Diagnostics(),
+                VALIDITY_ERROR(input_program->Diagnostics(),
                                "MSL writer errored on validated input:\n" + result.error);
             }
 #endif  // TINT_BUILD_MSL_WRITER

src/tint/writer/flatten_bindings.cc

@@ -0,0 +1,78 @@
+// Copyright 2022 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/tint/writer/flatten_bindings.h"
+
+#include <utility>
+
+#include "src/tint/inspector/inspector.h"
+#include "src/tint/transform/binding_remapper.h"
+#include "src/tint/transform/manager.h"
+
+namespace tint::writer {
+std::optional<Program> FlattenBindings(const Program* program) {
+    // TODO(crbug.com/tint/1101): Make this more robust for multiple entry points.
+    using BindingPoint = tint::transform::BindingPoint;
+    tint::transform::BindingRemapper::BindingPoints binding_points;
+    uint32_t next_buffer_idx = 0;
+    uint32_t next_sampler_idx = 0;
+    uint32_t next_texture_idx = 0;
+
+    tint::inspector::Inspector inspector(program);
+    auto entry_points = inspector.GetEntryPoints();
+    for (auto& entry_point : entry_points) {
+        auto bindings = inspector.GetResourceBindings(entry_point.name);
+        for (auto& binding : bindings) {
+            BindingPoint src = {binding.bind_group, binding.binding};
+            if (binding_points.count(src)) {
+                continue;
+            }
+            switch (binding.resource_type) {
+                case tint::inspector::ResourceBinding::ResourceType::kUniformBuffer:
+                case tint::inspector::ResourceBinding::ResourceType::kStorageBuffer:
+                case tint::inspector::ResourceBinding::ResourceType::kReadOnlyStorageBuffer:
+                    binding_points.emplace(src, BindingPoint{0, next_buffer_idx++});
+                    break;
+                case tint::inspector::ResourceBinding::ResourceType::kSampler:
+                case tint::inspector::ResourceBinding::ResourceType::kComparisonSampler:
+                    binding_points.emplace(src, BindingPoint{0, next_sampler_idx++});
+                    break;
+                case tint::inspector::ResourceBinding::ResourceType::kSampledTexture:
+                case tint::inspector::ResourceBinding::ResourceType::kMultisampledTexture:
+                case tint::inspector::ResourceBinding::ResourceType::kWriteOnlyStorageTexture:
+                case tint::inspector::ResourceBinding::ResourceType::kDepthTexture:
+                case tint::inspector::ResourceBinding::ResourceType::kDepthMultisampledTexture:
+                case tint::inspector::ResourceBinding::ResourceType::kExternalTexture:
+                    binding_points.emplace(src, BindingPoint{0, next_texture_idx++});
+                    break;
+            }
+        }
+    }
+
+    // Run the binding remapper transform.
+    tint::transform::Output transform_output;
+    if (!binding_points.empty()) {
+        tint::transform::Manager manager;
+        tint::transform::DataMap inputs;
+        inputs.Add<tint::transform::BindingRemapper::Remappings>(
+            std::move(binding_points), tint::transform::BindingRemapper::AccessControls{},
+            /* mayCollide */ true);
+        manager.Add<tint::transform::BindingRemapper>();
+        transform_output = manager.Run(program, inputs);
+        return std::move(transform_output.program);
+    }
+
+    return {};
+}
+}  // namespace tint::writer

src/tint/writer/flatten_bindings.h

@@ -0,0 +1,31 @@
+// Copyright 2022 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_TINT_WRITER_FLATTEN_BINDINGS_H_
+#define SRC_TINT_WRITER_FLATTEN_BINDINGS_H_
+
+#include <optional>
+#include "src/tint/program.h"
+
+namespace tint::writer {
+
+/// If needed, remaps resource numbers of `program` to a flat namespace: all in
+/// group 0 within unique binding numbers.
+/// @param program A valid program
+/// @return A new program with bindings remapped if needed
+std::optional<Program> FlattenBindings(const Program* program);
+
+}  // namespace tint::writer
+
+#endif  // SRC_TINT_WRITER_FLATTEN_BINDINGS_H_

src/tint/writer/flatten_bindings_test.cc

@@ -0,0 +1,142 @@
+// Copyright 2022 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/tint/writer/flatten_bindings.h"
+
+#include <utility>
+
+#include "gtest/gtest.h"
+#include "src/tint/program_builder.h"
+#include "src/tint/resolver/resolver.h"
+#include "src/tint/sem/variable.h"
+
+namespace tint::writer {
+namespace {
+
+class FlattenBindingsTest : public ::testing::Test {};
+
+TEST_F(FlattenBindingsTest, NoBindings) {
+    ProgramBuilder b;
+    b.WrapInFunction();
+
+    resolver::Resolver resolver(&b);
+
+    Program program(std::move(b));
+    ASSERT_TRUE(program.IsValid()) << program.Diagnostics().str();
+
+    auto flattened = tint::writer::FlattenBindings(&program);
+    EXPECT_FALSE(flattened);
+}
+
+TEST_F(FlattenBindingsTest, AlreadyFlat) {
+    ProgramBuilder b;
+    b.Global("a", b.ty.i32(), ast::StorageClass::kUniform, b.GroupAndBinding(0, 0));
+    b.Global("b", b.ty.i32(), ast::StorageClass::kUniform, b.GroupAndBinding(0, 1));
+    b.Global("c", b.ty.i32(), ast::StorageClass::kUniform, b.GroupAndBinding(0, 2));
+    b.WrapInFunction();
+
+    resolver::Resolver resolver(&b);
+
+    Program program(std::move(b));
+    ASSERT_TRUE(program.IsValid()) << program.Diagnostics().str();
+
+    auto flattened = tint::writer::FlattenBindings(&program);
+    EXPECT_FALSE(flattened);
+}
+
+TEST_F(FlattenBindingsTest, NotFlat_SingleNamespace) {
+    ProgramBuilder b;
+    b.Global("a", b.ty.i32(), ast::StorageClass::kUniform, b.GroupAndBinding(0, 0));
+    b.Global("b", b.ty.i32(), ast::StorageClass::kUniform, b.GroupAndBinding(1, 1));
+    b.Global("c", b.ty.i32(), ast::StorageClass::kUniform, b.GroupAndBinding(2, 2));
+    b.WrapInFunction(b.Expr("a"), b.Expr("b"), b.Expr("c"));
+
+    resolver::Resolver resolver(&b);
+
+    Program program(std::move(b));
+    ASSERT_TRUE(program.IsValid()) << program.Diagnostics().str();
+
+    auto flattened = tint::writer::FlattenBindings(&program);
+    EXPECT_TRUE(flattened);
+
+    auto& vars = flattened->AST().GlobalVariables();
+    EXPECT_EQ(vars[0]->BindingPoint().group->value, 0u);
+    EXPECT_EQ(vars[0]->BindingPoint().binding->value, 0u);
+    EXPECT_EQ(vars[1]->BindingPoint().group->value, 0u);
+    EXPECT_EQ(vars[1]->BindingPoint().binding->value, 1u);
+    EXPECT_EQ(vars[2]->BindingPoint().group->value, 0u);
+    EXPECT_EQ(vars[2]->BindingPoint().binding->value, 2u);
+}
+
+TEST_F(FlattenBindingsTest, NotFlat_MultipleNamespaces) {
+    ProgramBuilder b;
+
+    const size_t num_buffers = 3;
+    b.Global("buffer1", b.ty.i32(), ast::StorageClass::kUniform, b.GroupAndBinding(0, 0));
+    b.Global("buffer2", b.ty.i32(), ast::StorageClass::kStorage, b.GroupAndBinding(1, 1));
+    b.Global("buffer3", b.ty.i32(), ast::StorageClass::kStorage, ast::Access::kRead,
+             b.GroupAndBinding(2, 2));
+
+    const size_t num_samplers = 2;
+    b.Global("sampler1", b.ty.sampler(ast::SamplerKind::kSampler), b.GroupAndBinding(3, 3));
+    b.Global("sampler2", b.ty.sampler(ast::SamplerKind::kComparisonSampler),
+             b.GroupAndBinding(4, 4));
+
+    const size_t num_textures = 6;
+    b.Global("texture1", b.ty.sampled_texture(ast::TextureDimension::k2d, b.ty.f32()),
+             b.GroupAndBinding(5, 5));
+    b.Global("texture2", b.ty.multisampled_texture(ast::TextureDimension::k2d, b.ty.f32()),
+             b.GroupAndBinding(6, 6));
+    b.Global("texture3",
+             b.ty.storage_texture(ast::TextureDimension::k2d, ast::TexelFormat::kR32Float,
+                                  ast::Access::kWrite),
+             b.GroupAndBinding(7, 7));
+    b.Global("texture4", b.ty.depth_texture(ast::TextureDimension::k2d), b.GroupAndBinding(8, 8));
+    b.Global("texture5", b.ty.depth_multisampled_texture(ast::TextureDimension::k2d),
+             b.GroupAndBinding(9, 9));
+    b.Global("texture6", b.ty.external_texture(), b.GroupAndBinding(10, 10));
+
+    b.WrapInFunction(b.Assign(b.Phony(), "buffer1"), b.Assign(b.Phony(), "buffer2"),
+                     b.Assign(b.Phony(), "buffer3"), b.Assign(b.Phony(), "sampler1"),
+                     b.Assign(b.Phony(), "sampler2"), b.Assign(b.Phony(), "texture1"),
+                     b.Assign(b.Phony(), "texture2"), b.Assign(b.Phony(), "texture3"),
+                     b.Assign(b.Phony(), "texture4"), b.Assign(b.Phony(), "texture5"),
+                     b.Assign(b.Phony(), "texture6"));
+
+    resolver::Resolver resolver(&b);
+
+    Program program(std::move(b));
+    ASSERT_TRUE(program.IsValid()) << program.Diagnostics().str();
+
+    auto flattened = tint::writer::FlattenBindings(&program);
+    EXPECT_TRUE(flattened);
+
+    auto& vars = flattened->AST().GlobalVariables();
+
+    for (size_t i = 0; i < num_buffers; ++i) {
+        EXPECT_EQ(vars[i]->BindingPoint().group->value, 0u);
+        EXPECT_EQ(vars[i]->BindingPoint().binding->value, i);
+    }
+    for (size_t i = 0; i < num_samplers; ++i) {
+        EXPECT_EQ(vars[i + num_buffers]->BindingPoint().group->value, 0u);
+        EXPECT_EQ(vars[i + num_buffers]->BindingPoint().binding->value, i);
+    }
+    for (size_t i = 0; i < num_textures; ++i) {
+        EXPECT_EQ(vars[i + num_buffers + num_samplers]->BindingPoint().group->value, 0u);
+        EXPECT_EQ(vars[i + num_buffers + num_samplers]->BindingPoint().binding->value, i);
+    }
+}
+
+}  // namespace
+}  // namespace tint::writer

test/tint/BUILD.gn

@@ -379,6 +379,7 @@ tint_unittests_source_set("tint_unittests_utils_src") {
 tint_unittests_source_set("tint_unittests_writer_src") {
   sources = [
     "../../src/tint/writer/append_vector_test.cc",
+    "../../src/tint/writer/flatten_bindings_test.cc",
     "../../src/tint/writer/float_to_string_test.cc",
     "../../src/tint/writer/generate_external_texture_bindings_test.cc",
     "../../src/tint/writer/text_generator_test.cc",