tint: spirv writer: add a GeneralImpl to be consistent with HLSL and MSL backends

Also opportunistically optimize out the copying of the spir-v result
vector by moving it instead.

Bug: tint:1495
Change-Id: Ia2c3b3c09e7a23822eb8a782ce57b1fa11a0b54d
Reviewed-on: https://dawn-review.googlesource.com/c/dawn/+/86204
Reviewed-by: David Neto <dneto@google.com>
Kokoro-Run: Antonio Maiorano <amaiorano@google.com>
Kokoro: Kokoro <noreply+kokoro@google.com>
Commit-Queue: Antonio Maiorano <amaiorano@google.com>

src/tint/BUILD.gn

@@ -688,6 +688,8 @@ libtint_source_set("libtint_spv_writer_src") {
     "writer/spirv/function.h",
     "writer/spirv/generator.cc",
     "writer/spirv/generator.h",
+    "writer/spirv/generator_impl.cc",
+    "writer/spirv/generator_impl.h",
     "writer/spirv/instruction.cc",
     "writer/spirv/instruction.h",
     "writer/spirv/operand.cc",

src/tint/CMakeLists.txt

@@ -519,6 +519,8 @@ if(${TINT_BUILD_SPV_WRITER})
     writer/spirv/function.h
     writer/spirv/generator.cc
     writer/spirv/generator.h
+    writer/spirv/generator_impl.cc
+    writer/spirv/generator_impl.h
     writer/spirv/instruction.cc
     writer/spirv/instruction.h
     writer/spirv/operand.cc

src/tint/writer/spirv/binary_writer.h

@@ -45,6 +45,9 @@ class BinaryWriter {
   /// @returns the assembled SPIR-V
   const std::vector<uint32_t>& result() const { return out_; }
 
+  /// @returns the assembled SPIR-V
+  std::vector<uint32_t>& result() { return out_; }
+
  private:
   void process_instruction(const Instruction& inst);
   void process_op(const Operand& op);

src/tint/writer/spirv/builder.cc

@@ -41,26 +41,10 @@
 #include "src/tint/sem/type_conversion.h"
 #include "src/tint/sem/variable.h"
 #include "src/tint/sem/vector_type.h"
-#include "src/tint/transform/add_empty_entry_point.h"
 #include "src/tint/transform/add_spirv_block_attribute.h"
-#include "src/tint/transform/builtin_polyfill.h"
-#include "src/tint/transform/canonicalize_entry_point_io.h"
-#include "src/tint/transform/expand_compound_assignment.h"
-#include "src/tint/transform/fold_constants.h"
-#include "src/tint/transform/for_loop_to_loop.h"
-#include "src/tint/transform/manager.h"
-#include "src/tint/transform/promote_side_effects_to_decl.h"
-#include "src/tint/transform/remove_unreachable_statements.h"
-#include "src/tint/transform/simplify_pointers.h"
-#include "src/tint/transform/unshadow.h"
-#include "src/tint/transform/unwind_discard_functions.h"
-#include "src/tint/transform/var_for_dynamic_index.h"
-#include "src/tint/transform/vectorize_scalar_matrix_constructors.h"
-#include "src/tint/transform/zero_init_workgroup_memory.h"
 #include "src/tint/utils/defer.h"
 #include "src/tint/utils/map.h"
 #include "src/tint/writer/append_vector.h"
-#include "src/tint/writer/generate_external_texture_bindings.h"
 
 namespace tint::writer::spirv {
 namespace {
@@ -255,61 +239,6 @@ const sem::Type* ElementTypeOf(const sem::Type* ty) {
 
 }  // namespace
 
-SanitizedResult Sanitize(const Program* in, const Options& options) {
-  transform::Manager manager;
-  transform::DataMap data;
-
-  {  // Builtin polyfills
-    transform::BuiltinPolyfill::Builtins polyfills;
-    polyfills.count_leading_zeros = true;
-    polyfills.count_trailing_zeros = true;
-    polyfills.extract_bits =
-        transform::BuiltinPolyfill::Level::kClampParameters;
-    polyfills.first_leading_bit = true;
-    polyfills.first_trailing_bit = true;
-    polyfills.insert_bits = transform::BuiltinPolyfill::Level::kClampParameters;
-    data.Add<transform::BuiltinPolyfill::Config>(polyfills);
-    manager.Add<transform::BuiltinPolyfill>();
-  }
-
-  if (options.generate_external_texture_bindings) {
-    auto new_bindings_map = GenerateExternalTextureBindings(in);
-    data.Add<transform::MultiplanarExternalTexture::NewBindingPoints>(
-        new_bindings_map);
-  }
-  manager.Add<transform::MultiplanarExternalTexture>();
-
-  manager.Add<transform::Unshadow>();
-  bool disable_workgroup_init_in_sanitizer =
-      options.disable_workgroup_init ||
-      options.use_zero_initialize_workgroup_memory_extension;
-  if (!disable_workgroup_init_in_sanitizer) {
-    manager.Add<transform::ZeroInitWorkgroupMemory>();
-  }
-  manager.Add<transform::RemoveUnreachableStatements>();
-  manager.Add<transform::ExpandCompoundAssignment>();
-  manager.Add<transform::PromoteSideEffectsToDecl>();
-  manager.Add<transform::UnwindDiscardFunctions>();
-  manager.Add<transform::SimplifyPointers>();  // Required for arrayLength()
-  manager.Add<transform::FoldConstants>();
-  manager.Add<transform::VectorizeScalarMatrixConstructors>();
-  manager.Add<transform::ForLoopToLoop>();  // Must come after
-                                            // ZeroInitWorkgroupMemory
-  manager.Add<transform::CanonicalizeEntryPointIO>();
-  manager.Add<transform::AddEmptyEntryPoint>();
-  manager.Add<transform::AddSpirvBlockAttribute>();
-  manager.Add<transform::VarForDynamicIndex>();
-
-  data.Add<transform::CanonicalizeEntryPointIO::Config>(
-      transform::CanonicalizeEntryPointIO::Config(
-          transform::CanonicalizeEntryPointIO::ShaderStyle::kSpirv, 0xFFFFFFFF,
-          options.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() {}

src/tint/writer/spirv/builder.h

@@ -38,7 +38,6 @@
 #include "src/tint/sem/builtin.h"
 #include "src/tint/sem/storage_texture_type.h"
 #include "src/tint/writer/spirv/function.h"
-#include "src/tint/writer/spirv/generator.h"
 #include "src/tint/writer/spirv/scalar_constant.h"
 
 // Forward declarations
@@ -51,17 +50,6 @@ class TypeConversion;
 
 namespace tint::writer::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.
-/// @program The program to sanitize
-/// @param options The SPIR-V generator options.
-SanitizedResult Sanitize(const Program* program, const Options& options);
-
 /// Builder class to create SPIR-V instructions from a module.
 class Builder {
  public:

src/tint/writer/spirv/generator.cc

@@ -14,7 +14,9 @@
 
 #include "src/tint/writer/spirv/generator.h"
 
-#include "src/tint/writer/spirv/binary_writer.h"
+#include <utility>
+
+#include "src/tint/writer/spirv/generator_impl.h"
 
 namespace tint::writer::spirv {
 
@@ -37,20 +39,12 @@ Result Generate(const Program* program, const Options& options) {
   bool zero_initialize_workgroup_memory =
       !options.disable_workgroup_init &&
       options.use_zero_initialize_workgroup_memory_extension;
-  auto builder = std::make_unique<Builder>(&sanitized_result.program,
-                                           zero_initialize_workgroup_memory);
-  auto writer = std::make_unique<BinaryWriter>();
-  if (!builder->Build()) {
-    result.success = false;
-    result.error = builder->error();
-    return result;
-  }
 
-  writer->WriteHeader(builder->id_bound());
+  auto impl = std::make_unique<GeneratorImpl>(&sanitized_result.program,
-  writer->WriteBuilder(builder.get());
+                                              zero_initialize_workgroup_memory);
-
+  result.success = impl->Generate();
-  result.success = true;
+  result.error = impl->error();
-  result.spirv = writer->result();
+  result.spirv = std::move(impl->result());
 
   return result;
 }

src/tint/writer/spirv/generator_impl.cc

@@ -0,0 +1,120 @@
+// 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/spirv/generator_impl.h"
+
+#include <utility>
+#include <vector>
+
+#include "src/tint/transform/add_empty_entry_point.h"
+#include "src/tint/transform/add_spirv_block_attribute.h"
+#include "src/tint/transform/builtin_polyfill.h"
+#include "src/tint/transform/canonicalize_entry_point_io.h"
+#include "src/tint/transform/expand_compound_assignment.h"
+#include "src/tint/transform/fold_constants.h"
+#include "src/tint/transform/for_loop_to_loop.h"
+#include "src/tint/transform/manager.h"
+#include "src/tint/transform/promote_side_effects_to_decl.h"
+#include "src/tint/transform/remove_unreachable_statements.h"
+#include "src/tint/transform/simplify_pointers.h"
+#include "src/tint/transform/unshadow.h"
+#include "src/tint/transform/unwind_discard_functions.h"
+#include "src/tint/transform/var_for_dynamic_index.h"
+#include "src/tint/transform/vectorize_scalar_matrix_constructors.h"
+#include "src/tint/transform/zero_init_workgroup_memory.h"
+#include "src/tint/writer/generate_external_texture_bindings.h"
+
+namespace tint::writer::spirv {
+
+SanitizedResult Sanitize(const Program* in, const Options& options) {
+  transform::Manager manager;
+  transform::DataMap data;
+
+  {  // Builtin polyfills
+    transform::BuiltinPolyfill::Builtins polyfills;
+    polyfills.count_leading_zeros = true;
+    polyfills.count_trailing_zeros = true;
+    polyfills.extract_bits =
+        transform::BuiltinPolyfill::Level::kClampParameters;
+    polyfills.first_leading_bit = true;
+    polyfills.first_trailing_bit = true;
+    polyfills.insert_bits = transform::BuiltinPolyfill::Level::kClampParameters;
+    data.Add<transform::BuiltinPolyfill::Config>(polyfills);
+    manager.Add<transform::BuiltinPolyfill>();
+  }
+
+  if (options.generate_external_texture_bindings) {
+    auto new_bindings_map = GenerateExternalTextureBindings(in);
+    data.Add<transform::MultiplanarExternalTexture::NewBindingPoints>(
+        new_bindings_map);
+  }
+  manager.Add<transform::MultiplanarExternalTexture>();
+
+  manager.Add<transform::Unshadow>();
+  bool disable_workgroup_init_in_sanitizer =
+      options.disable_workgroup_init ||
+      options.use_zero_initialize_workgroup_memory_extension;
+  if (!disable_workgroup_init_in_sanitizer) {
+    manager.Add<transform::ZeroInitWorkgroupMemory>();
+  }
+  manager.Add<transform::RemoveUnreachableStatements>();
+  manager.Add<transform::ExpandCompoundAssignment>();
+  manager.Add<transform::PromoteSideEffectsToDecl>();
+  manager.Add<transform::UnwindDiscardFunctions>();
+  manager.Add<transform::SimplifyPointers>();  // Required for arrayLength()
+  manager.Add<transform::FoldConstants>();
+  manager.Add<transform::VectorizeScalarMatrixConstructors>();
+  manager.Add<transform::ForLoopToLoop>();  // Must come after
+                                            // ZeroInitWorkgroupMemory
+  manager.Add<transform::CanonicalizeEntryPointIO>();
+  manager.Add<transform::AddEmptyEntryPoint>();
+  manager.Add<transform::AddSpirvBlockAttribute>();
+  manager.Add<transform::VarForDynamicIndex>();
+
+  data.Add<transform::CanonicalizeEntryPointIO::Config>(
+      transform::CanonicalizeEntryPointIO::Config(
+          transform::CanonicalizeEntryPointIO::ShaderStyle::kSpirv, 0xFFFFFFFF,
+          options.emit_vertex_point_size));
+
+  SanitizedResult result;
+  result.program = std::move(manager.Run(in, data).program);
+  return result;
+}
+
+GeneratorImpl::GeneratorImpl(const Program* program,
+                             bool zero_initialize_workgroup_memory)
+    : builder_(program, zero_initialize_workgroup_memory) {}
+
+bool GeneratorImpl::Generate() {
+  if (builder_.Build()) {
+    writer_.WriteHeader(builder_.id_bound());
+    writer_.WriteBuilder(&builder_);
+    return true;
+  }
+  return false;
+}
+
+const std::vector<uint32_t>& GeneratorImpl::result() const {
+  return writer_.result();
+}
+
+std::vector<uint32_t>& GeneratorImpl::result() {
+  return writer_.result();
+}
+
+std::string GeneratorImpl::error() const {
+  return builder_.error();
+}
+
+}  // namespace tint::writer::spirv

src/tint/writer/spirv/generator_impl.h

@@ -0,0 +1,67 @@
+// 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_SPIRV_GENERATOR_IMPL_H_
+#define SRC_TINT_WRITER_SPIRV_GENERATOR_IMPL_H_
+
+#include <string>
+#include <vector>
+
+#include "src/tint/program.h"
+#include "src/tint/writer/spirv/binary_writer.h"
+#include "src/tint/writer/spirv/builder.h"
+#include "src/tint/writer/spirv/generator.h"
+
+namespace tint::writer::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.
+/// @program The program to sanitize
+/// @param options The SPIR-V generator options.
+SanitizedResult Sanitize(const Program* program, const Options& options);
+
+/// Implementation class for SPIR-V generator
+class GeneratorImpl {
+ public:
+  /// Constructor
+  /// @param program the program to generate
+  /// @param zero_initialize_workgroup_memory `true` to initialize all the
+  /// variables in the Workgroup storage class with OpConstantNull
+  GeneratorImpl(const Program* program, bool zero_initialize_workgroup_memory);
+
+  /// @returns true on successful generation; false otherwise
+  bool Generate();
+
+  /// @returns the result data
+  const std::vector<uint32_t>& result() const;
+
+  /// @returns the result data
+  std::vector<uint32_t>& result();
+
+  /// @returns the error
+  std::string error() const;
+
+ private:
+  Builder builder_;
+  BinaryWriter writer_;
+};
+
+}  // namespace tint::writer::spirv
+
+#endif  // SRC_TINT_WRITER_SPIRV_GENERATOR_IMPL_H_

src/tint/writer/spirv/test_helper.h

@@ -22,6 +22,7 @@
 #include "gtest/gtest.h"
 #include "spirv-tools/libspirv.hpp"
 #include "src/tint/writer/spirv/binary_writer.h"
+#include "src/tint/writer/spirv/generator_impl.h"
 
 namespace tint::writer::spirv {