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 <noreply+kokoro@google.com>
Reviewed-by: Ben Clayton <bclayton@google.com>

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",

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

src/transform/spirv.cc

@@ -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 <string>
-#include <utility>
-
-#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<Config>();
-
-  Manager manager;
-  DataMap internal_inputs;
-  if (!cfg || !cfg->disable_workgroup_init) {
-    manager.Add<ZeroInitWorkgroupMemory>();
-  }
-  manager.Add<InlinePointerLets>();  // Required for arrayLength()
-  manager.Add<Simplify>();           // Required for arrayLength()
-  manager.Add<FoldConstants>();
-  manager.Add<ExternalTextureTransform>();
-  manager.Add<ForLoopToLoop>();  // Must come after ZeroInitWorkgroupMemory
-  manager.Add<CanonicalizeEntryPointIO>();
-  manager.Add<AddEmptyEntryPoint>();
-
-  internal_inputs.Add<CanonicalizeEntryPointIO::Config>(
-      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

src/transform/spirv.h

@@ -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 <vector>
-
-#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<Spirv, Transform> {
- public:
-  /// Configuration options for the transform.
-  struct Config : public Castable<Config, Data> {
-    /// 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_

src/transform/spirv_test.cc

@@ -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

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<transform::ZeroInitWorkgroupMemory>();
+  }
+  manager.Add<transform::InlinePointerLets>();  // Required for arrayLength()
+  manager.Add<transform::Simplify>();           // Required for arrayLength()
+  manager.Add<transform::FoldConstants>();
+  manager.Add<transform::ExternalTextureTransform>();
+  manager.Add<transform::ForLoopToLoop>();  // Must come after
+                                            // ZeroInitWorkgroupMemory
+  manager.Add<transform::CanonicalizeEntryPointIO>();
+  manager.Add<transform::AddEmptyEntryPoint>();
+
+  data.Add<transform::CanonicalizeEntryPointIO::Config>(
+      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<transform::Spirv>()) {
-    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,

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:

src/writer/spirv/builder_test.cc

@@ -22,25 +22,6 @@ namespace {
 
 using BuilderTest = TestHelper;
 
-TEST_F(BuilderTest, ErrorIfSanitizerNotRun) {
-  auto program = std::make_unique<Program>(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();
 

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.
+  // Sanitize the program.
-  transform::Spirv sanitizer;
+  auto sanitized_result = Sanitize(program, options.emit_vertex_point_size,
-  transform::DataMap transform_input;
+                                   options.disable_workgroup_init);
-  transform_input.Add<transform::Spirv::Config>(options.emit_vertex_point_size,
+  if (!sanitized_result.program.IsValid()) {
-                                                options.disable_workgroup_init);
-  auto output = sanitizer.Run(program, transform_input);
-  if (!output.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<Builder>(&output.program);
+  auto builder = std::make_unique<Builder>(&sanitized_result.program);
   auto writer = std::make_unique<BinaryWriter>();
   if (!builder->Build()) {
     result.success = false;

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<transform::Spirv>();
     [&]() {
       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());

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",