From ebab7d2f7a7e156445f3789bc68294372cf47173 Mon Sep 17 00:00:00 2001 From: James Price Date: Thu, 9 Sep 2021 14:40:07 +0000 Subject: [PATCH] spirv: Remove the sanitizer transform Invoke the required transforms directly in the SPIR-V backend. Change-Id: I78dc667d5c4c9c1d4da13ef5a99ece831c103982 Reviewed-on: https://dawn-review.googlesource.com/c/tint/+/63801 Kokoro: Kokoro Reviewed-by: Ben Clayton --- src/BUILD.gn | 2 - src/CMakeLists.txt | 5 -- src/transform/spirv.cc | 86 -------------------------------- src/transform/spirv.h | 75 ---------------------------- src/transform/spirv_test.cc | 29 ----------- src/writer/spirv/builder.cc | 45 ++++++++++++++--- src/writer/spirv/builder.h | 14 ++++++ src/writer/spirv/builder_test.cc | 19 ------- src/writer/spirv/generator.cc | 16 +++--- src/writer/spirv/test_helper.h | 6 +-- test/BUILD.gn | 1 - 11 files changed, 58 insertions(+), 240 deletions(-) delete mode 100644 src/transform/spirv.cc delete mode 100644 src/transform/spirv.h delete mode 100644 src/transform/spirv_test.cc diff --git a/src/BUILD.gn b/src/BUILD.gn index ccbbfd6c28..da64b3cd83 100644 --- a/src/BUILD.gn +++ b/src/BUILD.gn @@ -621,8 +621,6 @@ libtint_source_set("libtint_spv_reader_src") { libtint_source_set("libtint_spv_writer_src") { sources = [ - "transform/spirv.cc", - "transform/spirv.h", "writer/spirv/binary_writer.cc", "writer/spirv/binary_writer.h", "writer/spirv/builder.cc", diff --git a/src/CMakeLists.txt b/src/CMakeLists.txt index 51a8b91b1f..ecceb80a97 100644 --- a/src/CMakeLists.txt +++ b/src/CMakeLists.txt @@ -448,8 +448,6 @@ endif() if(${TINT_BUILD_SPV_WRITER}) list(APPEND TINT_LIB_SRCS - transform/spirv.cc - transform/spirv.h writer/spirv/binary_writer.cc writer/spirv/binary_writer.h writer/spirv/builder.cc @@ -847,9 +845,6 @@ if(${TINT_BUILD_TESTS}) endif() if(${TINT_BUILD_SPV_WRITER}) - if(${TINT_BUILD_WGSL_READER} AND ${TINT_BUILD_WGSL_WRITER}) - list(APPEND TINT_TEST_SRCS transform/spirv_test.cc) - endif() list(APPEND TINT_TEST_SRCS writer/spirv/binary_writer_test.cc writer/spirv/builder_accessor_expression_test.cc diff --git a/src/transform/spirv.cc b/src/transform/spirv.cc deleted file mode 100644 index 76fca09d9c..0000000000 --- a/src/transform/spirv.cc +++ /dev/null @@ -1,86 +0,0 @@ -// Copyright 2020 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/spirv.h" - -#include -#include - -#include "src/ast/stage_decoration.h" -#include "src/program_builder.h" -#include "src/sem/variable.h" -#include "src/transform/add_empty_entry_point.h" -#include "src/transform/canonicalize_entry_point_io.h" -#include "src/transform/external_texture_transform.h" -#include "src/transform/fold_constants.h" -#include "src/transform/for_loop_to_loop.h" -#include "src/transform/inline_pointer_lets.h" -#include "src/transform/manager.h" -#include "src/transform/simplify.h" -#include "src/transform/zero_init_workgroup_memory.h" - -TINT_INSTANTIATE_TYPEINFO(tint::transform::Spirv); -TINT_INSTANTIATE_TYPEINFO(tint::transform::Spirv::Config); - -namespace tint { -namespace transform { - -Spirv::Spirv() = default; -Spirv::~Spirv() = default; - -Output Spirv::Run(const Program* in, const DataMap& data) { - auto* cfg = data.Get(); - - Manager manager; - DataMap internal_inputs; - if (!cfg || !cfg->disable_workgroup_init) { - manager.Add(); - } - manager.Add(); // Required for arrayLength() - manager.Add(); // Required for arrayLength() - manager.Add(); - manager.Add(); - manager.Add(); // Must come after ZeroInitWorkgroupMemory - manager.Add(); - manager.Add(); - - internal_inputs.Add( - CanonicalizeEntryPointIO::Config( - CanonicalizeEntryPointIO::ShaderStyle::kSpirv, 0xFFFFFFFF, - (cfg && cfg->emit_vertex_point_size))); - - auto transformedInput = manager.Run(in, internal_inputs); - - if (transformedInput.program.Diagnostics().contains_errors()) { - return transformedInput; - } - - ProgramBuilder builder; - CloneContext ctx(&builder, &transformedInput.program); - // TODO(jrprice): Move the sanitizer into the backend. - ctx.Clone(); - - builder.SetTransformApplied(this); - return Output{Program(std::move(builder))}; -} - -Spirv::Config::Config(bool emit_vps, bool disable_wi) - : emit_vertex_point_size(emit_vps), disable_workgroup_init(disable_wi) {} - -Spirv::Config::Config(const Config&) = default; -Spirv::Config::~Config() = default; -Spirv::Config& Spirv::Config::operator=(const Config&) = default; - -} // namespace transform -} // namespace tint diff --git a/src/transform/spirv.h b/src/transform/spirv.h deleted file mode 100644 index 7e633ab5f3..0000000000 --- a/src/transform/spirv.h +++ /dev/null @@ -1,75 +0,0 @@ -// 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_SPIRV_H_ -#define SRC_TRANSFORM_SPIRV_H_ - -#include - -#include "src/transform/transform.h" - -namespace tint { - -// Forward declarations -class CloneContext; - -namespace transform { - -/// Spirv is a transform used to sanitize a Program for use with the Spirv -/// writer. Passing a non-sanitized Program to the Spirv writer will result in -/// undefined behavior. -class Spirv : public Castable { - public: - /// Configuration options for the transform. - struct Config : public Castable { - /// Constructor - /// @param emit_vertex_point_size `true` to generate a PointSize builtin - /// @param disable_workgroup_init `true` to disable workgroup memory zero - /// initialization - Config(bool emit_vertex_point_size = false, - bool disable_workgroup_init = false); - - /// Copy constructor. - Config(const Config&); - - /// Destructor. - ~Config() override; - - /// Assignment operator. - /// @returns this Config - Config& operator=(const Config&); - - /// Set to `true` to generate a PointSize builtin and have it set to 1.0 - /// from all vertex shaders in the module. - bool emit_vertex_point_size = false; - - /// Set to `true` to disable workgroup memory zero initialization - bool disable_workgroup_init = false; - }; - - /// Constructor - Spirv(); - ~Spirv() override; - - /// Runs the transform on `program`, returning the transformation result. - /// @param program the source program to transform - /// @param data optional extra transform-specific input data - /// @returns the transformation result - Output Run(const Program* program, const DataMap& data = {}) override; -}; - -} // namespace transform -} // namespace tint - -#endif // SRC_TRANSFORM_SPIRV_H_ diff --git a/src/transform/spirv_test.cc b/src/transform/spirv_test.cc deleted file mode 100644 index e2d4c3c60b..0000000000 --- a/src/transform/spirv_test.cc +++ /dev/null @@ -1,29 +0,0 @@ -// 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/spirv.h" - -#include "src/transform/test_helper.h" - -namespace tint { -namespace transform { -namespace { - -using SpirvTest = TransformTest; - -// TODO(jrprice): Remove this file when we remove the sanitizers. - -} // namespace -} // namespace transform -} // namespace tint diff --git a/src/writer/spirv/builder.cc b/src/writer/spirv/builder.cc index 11942ef669..1610463ce7 100644 --- a/src/writer/spirv/builder.cc +++ b/src/writer/spirv/builder.cc @@ -36,7 +36,15 @@ #include "src/sem/struct.h" #include "src/sem/variable.h" #include "src/sem/vector_type.h" -#include "src/transform/spirv.h" +#include "src/transform/add_empty_entry_point.h" +#include "src/transform/canonicalize_entry_point_io.h" +#include "src/transform/external_texture_transform.h" +#include "src/transform/fold_constants.h" +#include "src/transform/for_loop_to_loop.h" +#include "src/transform/inline_pointer_lets.h" +#include "src/transform/manager.h" +#include "src/transform/simplify.h" +#include "src/transform/zero_init_workgroup_memory.h" #include "src/utils/get_or_create.h" #include "src/writer/append_vector.h" @@ -260,6 +268,34 @@ const sem::Type* ElementTypeOf(const sem::Type* ty) { } // namespace +SanitizedResult Sanitize(const Program* in, + bool emit_vertex_point_size, + bool disable_workgroup_init) { + transform::Manager manager; + transform::DataMap data; + + if (!disable_workgroup_init) { + manager.Add(); + } + manager.Add(); // Required for arrayLength() + manager.Add(); // Required for arrayLength() + manager.Add(); + manager.Add(); + manager.Add(); // Must come after + // ZeroInitWorkgroupMemory + manager.Add(); + manager.Add(); + + data.Add( + transform::CanonicalizeEntryPointIO::Config( + transform::CanonicalizeEntryPointIO::ShaderStyle::kSpirv, 0xFFFFFFFF, + emit_vertex_point_size)); + + SanitizedResult result; + result.program = std::move(manager.Run(in, data).program); + return result; +} + Builder::AccessorInfo::AccessorInfo() : source_id(0), source_type(nullptr) {} Builder::AccessorInfo::~AccessorInfo() {} @@ -270,13 +306,6 @@ Builder::Builder(const Program* program) Builder::~Builder() = default; bool Builder::Build() { - if (!builder_.HasTransformApplied()) { - error_ = - "SPIR-V writer requires the transform::Spirv sanitizer to have been " - "applied to the input program"; - return false; - } - push_capability(SpvCapabilityShader); push_memory_model(spv::Op::OpMemoryModel, diff --git a/src/writer/spirv/builder.h b/src/writer/spirv/builder.h index 59ae87b181..8af4cf268e 100644 --- a/src/writer/spirv/builder.h +++ b/src/writer/spirv/builder.h @@ -51,6 +51,20 @@ class Reference; namespace writer { namespace spirv { +/// The result of sanitizing a program for generation. +struct SanitizedResult { + /// The sanitized program. + Program program; +}; + +/// Sanitize a program in preparation for generating SPIR-V. +/// @param emit_vertex_point_size `true` to emit a vertex point size builtin +/// @param disable_workgroup_init `true` to disable workgroup memory zero +/// @returns the sanitized program and any supplementary information +SanitizedResult Sanitize(const Program* program, + bool emit_vertex_point_size = false, + bool disable_workgroup_init = false); + /// Builder class to create SPIR-V instructions from a module. class Builder { public: diff --git a/src/writer/spirv/builder_test.cc b/src/writer/spirv/builder_test.cc index acf48d6980..977cdf3987 100644 --- a/src/writer/spirv/builder_test.cc +++ b/src/writer/spirv/builder_test.cc @@ -22,25 +22,6 @@ namespace { using BuilderTest = TestHelper; -TEST_F(BuilderTest, ErrorIfSanitizerNotRun) { - auto program = std::make_unique(std::move(*this)); - spirv::Builder b(program.get()); - EXPECT_FALSE(b.Build()); - EXPECT_EQ( - b.error(), - "SPIR-V writer requires the transform::Spirv sanitizer to have been " - "applied to the input program"); -} - -TEST_F(BuilderTest, InsertsPreamble) { - spirv::Builder& b = Build(); - - ASSERT_TRUE(b.Build()); - EXPECT_EQ(DumpBuilder(b), R"(OpCapability Shader -OpMemoryModel Logical GLSL450 -)"); -} - TEST_F(BuilderTest, TracksIdBounds) { spirv::Builder& b = Build(); diff --git a/src/writer/spirv/generator.cc b/src/writer/spirv/generator.cc index ec249ac64a..ee477f0f22 100644 --- a/src/writer/spirv/generator.cc +++ b/src/writer/spirv/generator.cc @@ -14,7 +14,6 @@ #include "src/writer/spirv/generator.h" -#include "src/transform/spirv.h" #include "src/writer/spirv/binary_writer.h" namespace tint { @@ -28,20 +27,17 @@ Result::Result(const Result&) = default; Result Generate(const Program* program, const Options& options) { Result result; - // Run the SPIR-V sanitizer. - transform::Spirv sanitizer; - transform::DataMap transform_input; - transform_input.Add(options.emit_vertex_point_size, - options.disable_workgroup_init); - auto output = sanitizer.Run(program, transform_input); - if (!output.program.IsValid()) { + // Sanitize the program. + auto sanitized_result = Sanitize(program, options.emit_vertex_point_size, + options.disable_workgroup_init); + if (!sanitized_result.program.IsValid()) { result.success = false; - result.error = output.program.Diagnostics().str(); + result.error = sanitized_result.program.Diagnostics().str(); return result; } // Generate the SPIR-V code. - auto builder = std::make_unique(&output.program); + auto builder = std::make_unique(&sanitized_result.program); auto writer = std::make_unique(); if (!builder->Build()) { result.success = false; diff --git a/src/writer/spirv/test_helper.h b/src/writer/spirv/test_helper.h index dd8f5c2230..90fa2cb2d2 100644 --- a/src/writer/spirv/test_helper.h +++ b/src/writer/spirv/test_helper.h @@ -21,7 +21,6 @@ #include "gtest/gtest.h" #include "spirv-tools/libspirv.hpp" -#include "src/transform/spirv.h" #include "src/writer/spirv/binary_writer.h" namespace tint { @@ -43,9 +42,6 @@ class TestHelperBase : public ProgramBuilder, public BASE { if (spirv_builder) { return *spirv_builder; } - // Fake that the SPIR-V sanitizer has been applied, so that we can unit test - // the writer without it erroring. - SetTransformApplied(); [&]() { ASSERT_TRUE(IsValid()) << "Builder program is not valid\n" << diag::Formatter().format(Diagnostics()); @@ -77,7 +73,7 @@ class TestHelperBase : public ProgramBuilder, public BASE { ASSERT_TRUE(program->IsValid()) << diag::Formatter().format(program->Diagnostics()); }(); - auto result = transform::Spirv().Run(program.get()); + auto result = Sanitize(program.get()); [&]() { ASSERT_TRUE(result.program.IsValid()) << diag::Formatter().format(result.program.Diagnostics()); diff --git a/test/BUILD.gn b/test/BUILD.gn index 203b853828..13c97b58b5 100644 --- a/test/BUILD.gn +++ b/test/BUILD.gn @@ -380,7 +380,6 @@ tint_unittests_source_set("tint_unittests_spv_reader_src") { tint_unittests_source_set("tint_unittests_spv_writer_src") { sources = [ - "../src/transform/spirv_test.cc", "../src/writer/spirv/binary_writer_test.cc", "../src/writer/spirv/builder_accessor_expression_test.cc", "../src/writer/spirv/builder_assign_test.cc",