From 4e98fb0bd8c99ac7cc91f7157428f645099b86d3 Mon Sep 17 00:00:00 2001 From: Antonio Maiorano Date: Mon, 2 May 2022 19:49:19 +0000 Subject: [PATCH] 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 Commit-Queue: Antonio Maiorano Reviewed-by: James Price --- src/tint/BUILD.gn | 2 + src/tint/CMakeLists.txt | 3 + src/tint/cmd/main.cc | 57 +-------- src/tint/fuzzers/tint_common_fuzzer.cc | 13 ++- src/tint/writer/flatten_bindings.cc | 78 +++++++++++++ src/tint/writer/flatten_bindings.h | 31 +++++ src/tint/writer/flatten_bindings_test.cc | 142 +++++++++++++++++++++++ test/tint/BUILD.gn | 1 + 8 files changed, 274 insertions(+), 53 deletions(-) create mode 100644 src/tint/writer/flatten_bindings.cc create mode 100644 src/tint/writer/flatten_bindings.h create mode 100644 src/tint/writer/flatten_bindings_test.cc diff --git a/src/tint/BUILD.gn b/src/tint/BUILD.gn index 3d47acf091..1b30b66548 100644 --- a/src/tint/BUILD.gn +++ b/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", diff --git a/src/tint/CMakeLists.txt b/src/tint/CMakeLists.txt index e5dcb14833..ec533ea63f 100644 --- a/src/tint/CMakeLists.txt +++ b/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 diff --git a/src/tint/cmd/main.cc b/src/tint/cmd/main.cc index e038218450..fdb625bb77 100644 --- a/src/tint/cmd/main.cc +++ b/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. - 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( - std::move(binding_points), tint::transform::BindingRemapper::AccessControls{}, - /* mayCollide */ true); - manager.Add(); - transform_output = manager.Run(program, inputs); - input_program = &transform_output.program; + // TODO(crbug.com/tint/1501): Do this via Options::BindingMap. + const tint::Program* input_program = program; + auto flattened = tint::writer::FlattenBindings(program); + if (flattened) { + input_program = &*flattened; } // TODO(jrprice): Provide a way for the user to set non-default options. diff --git a/src/tint/fuzzers/tint_common_fuzzer.cc b/src/tint/fuzzers/tint_common_fuzzer.cc index 3fb137c9a1..bbea6fce08 100644 --- a/src/tint/fuzzers/tint_common_fuzzer.cc +++ b/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 diff --git a/src/tint/writer/flatten_bindings.cc b/src/tint/writer/flatten_bindings.cc new file mode 100644 index 0000000000..1efc02a87f --- /dev/null +++ b/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 + +#include "src/tint/inspector/inspector.h" +#include "src/tint/transform/binding_remapper.h" +#include "src/tint/transform/manager.h" + +namespace tint::writer { +std::optional 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( + std::move(binding_points), tint::transform::BindingRemapper::AccessControls{}, + /* mayCollide */ true); + manager.Add(); + transform_output = manager.Run(program, inputs); + return std::move(transform_output.program); + } + + return {}; +} +} // namespace tint::writer diff --git a/src/tint/writer/flatten_bindings.h b/src/tint/writer/flatten_bindings.h new file mode 100644 index 0000000000..02505715fe --- /dev/null +++ b/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 +#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 FlattenBindings(const Program* program); + +} // namespace tint::writer + +#endif // SRC_TINT_WRITER_FLATTEN_BINDINGS_H_ diff --git a/src/tint/writer/flatten_bindings_test.cc b/src/tint/writer/flatten_bindings_test.cc new file mode 100644 index 0000000000..1c516c92e5 --- /dev/null +++ b/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 + +#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 diff --git a/test/tint/BUILD.gn b/test/tint/BUILD.gn index 9c64420f49..817bca593f 100644 --- a/test/tint/BUILD.gn +++ b/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",