[spirv-reader] Refactor function emission

Create a FunctionEmitter class.
Move ParserImpl::Name to Namer::Name, and add tests.

Bug: tint:3
Change-Id: I271e8c75f6f5a0edf9d94fe0a4af5a022afac708
Reviewed-on: https://dawn-review.googlesource.com/c/tint/+/18702
Reviewed-by: dan sinclair <dsinclair@google.com>

src/CMakeLists.txt

@@ -204,6 +204,8 @@ if(${TINT_BUILD_SPV_READER})
     reader/spirv/enum_converter.h
     reader/spirv/enum_converter.cc
     reader/spirv/fail_stream.h
+    reader/spirv/function.cc
+    reader/spirv/function.h
     reader/spirv/namer.cc
     reader/spirv/namer.h
     reader/spirv/parser.cc

src/reader/spirv/function.cc

@@ -0,0 +1,86 @@
+// 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/reader/spirv/function.h"
+
+#include "source/opt/basic_block.h"
+#include "source/opt/function.h"
+#include "source/opt/instruction.h"
+#include "source/opt/module.h"
+#include "src/ast/variable.h"
+#include "src/ast/variable_decl_statement.h"
+#include "src/reader/spirv/fail_stream.h"
+#include "src/reader/spirv/parser_impl.h"
+
+namespace tint {
+namespace reader {
+namespace spirv {
+
+FunctionEmitter::FunctionEmitter(ParserImpl* pi,
+                                 const spvtools::opt::Function& function)
+    : parser_impl_(*pi),
+      ast_module_(pi->get_module()),
+      ir_context_(*(pi->ir_context())),
+      fail_stream_(pi->fail_stream()),
+      namer_(pi->namer()),
+      function_(function) {}
+
+FunctionEmitter::~FunctionEmitter() = default;
+
+bool FunctionEmitter::Emit() {
+  // We only care about functions with bodies.
+  if (function_.cbegin() == function_.cend()) {
+    return true;
+  }
+
+  const auto name = namer_.Name(function_.result_id());
+  // Surprisingly, the "type id" on an OpFunction is the result type of the
+  // function, not the type of the function.  This is the one exceptional case
+  // in SPIR-V where the type ID is not the type of the result ID.
+  auto* ret_ty = parser_impl_.ConvertType(function_.type_id());
+  if (failed()) {
+    return false;
+  }
+  if (ret_ty == nullptr) {
+    return Fail()
+           << "internal error: unregistered return type for function with ID "
+           << function_.result_id();
+  }
+
+  std::vector<std::unique_ptr<ast::Variable>> ast_params;
+  function_.ForEachParam(
+      [this, &ast_params](const spvtools::opt::Instruction* param) {
+        auto* ast_type = parser_impl_.ConvertType(param->type_id());
+        if (ast_type != nullptr) {
+          ast_params.emplace_back(parser_impl_.MakeVariable(
+              param->result_id(), ast::StorageClass::kNone, ast_type));
+        } else {
+          // We've already logged an error and emitted a diagnostic. Do nothing
+          // here.
+        }
+      });
+  if (failed()) {
+    return false;
+  }
+
+  auto ast_fn =
+      std::make_unique<ast::Function>(name, std::move(ast_params), ret_ty);
+  ast_module_.AddFunction(std::move(ast_fn));
+
+  return success();
+}
+
+}  // namespace spirv
+}  // namespace reader
+}  // namespace tint

src/reader/spirv/function.h

@@ -0,0 +1,66 @@
+// 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.
+
+#ifndef SRC_READER_SPIRV_FUNCTION_H_
+#define SRC_READER_SPIRV_FUNCTION_H_
+
+#include "source/opt/function.h"
+#include "source/opt/ir_context.h"
+#include "src/ast/module.h"
+#include "src/reader/spirv/fail_stream.h"
+#include "src/reader/spirv/namer.h"
+#include "src/reader/spirv/parser_impl.h"
+
+namespace tint {
+namespace reader {
+namespace spirv {
+
+/// A FunctionEmitter emits a SPIR-V function onto a Tint AST module.
+class FunctionEmitter {
+ public:
+  /// Creates a FunctionEmitter, and prepares to write to the AST module
+  /// in |pi|.
+  /// @param pi a ParserImpl which has already executed BuildInternalModule
+  /// @param function the function to emit
+  FunctionEmitter(ParserImpl* pi, const spvtools::opt::Function& function);
+  /// Destructor
+  ~FunctionEmitter();
+
+  /// Emits the function to AST module.
+  /// @return whether emission succeeded
+  bool Emit();
+
+  /// @returns true if emission has not yet failed.
+  bool success() const { return fail_stream_.status(); }
+  /// @returns true if emission has failed.
+  bool failed() const { return !success(); }
+
+  /// Records failure.
+  /// @returns a FailStream on which to emit diagnostics.
+  FailStream& Fail() { return fail_stream_.Fail(); }
+
+ private:
+  ParserImpl& parser_impl_;
+  ast::Module& ast_module_;
+  spvtools::opt::IRContext& ir_context_;
+  FailStream& fail_stream_;
+  Namer& namer_;
+  const spvtools::opt::Function& function_;
+};
+
+}  // namespace spirv
+}  // namespace reader
+}  // namespace tint
+
+#endif  // SRC_READER_SPIRV_FUNCTION_H_

src/reader/spirv/namer.h

@@ -32,7 +32,7 @@ namespace spirv {
 /// Some names are user-suggested, but "sanitized" in the sense that an
 /// unusual character (e.g. invalid for use in WGSL identifiers) is remapped
 /// to a safer character such as an underscore.  Also, sanitized names
-/// never start with an underscorre.
+/// never start with an underscore.
 class Namer {
  public:
   /// Creates a new namer
@@ -64,6 +64,17 @@ class Namer {
     return id_to_name_.find(id)->second;
   }
 
+  /// Gets a unique name for the ID. If one already exists, then return
+  /// that, otherwise synthesize a name and remember it for later.
+  /// @param id the SPIR-V ID
+  /// @returns a name for the given ID. Generates a name if non exists.
+  const std::string& Name(uint32_t id) {
+    if (!HasName(id)) {
+      SuggestSanitizedName(id, "x_" + std::to_string(id));
+    }
+    return GetName(id);
+  }
+
   /// Gets the registered name for a struct member. If no name has
   /// been registered for this member, then returns the empty string.
   /// member index is in bounds.

src/reader/spirv/namer_test.cc

@@ -216,6 +216,23 @@ TEST_F(SpvNamerTest,
   EXPECT_THAT(namer.GetMemberName(1, 2), Eq("mother"));
 }
 
+TEST_F(SpvNamerTest, Name_GeneratesNameIfNoneRegistered) {
+  Namer namer(fail_stream_);
+  EXPECT_THAT(namer.Name(14), Eq("x_14"));
+}
+
+TEST_F(SpvNamerTest, Name_GeneratesNameWithoutConflict) {
+  Namer namer(fail_stream_);
+  namer.SaveName(42, "x_14");
+  EXPECT_THAT(namer.Name(14), Eq("x_14_1"));
+}
+
+TEST_F(SpvNamerTest, Name_ReturnsRegisteredName) {
+  Namer namer(fail_stream_);
+  namer.SaveName(14, "hello");
+  EXPECT_THAT(namer.Name(14), Eq("hello"));
+}
+
 TEST_F(SpvNamerTest,
        ResolveMemberNamesForStruct_GeneratesRegularNamesOnItsOwn) {
   Namer namer(fail_stream_);

src/reader/spirv/parser_impl.cc

@@ -54,6 +54,7 @@
 #include "src/ast/variable.h"
 #include "src/ast/variable_decl_statement.h"
 #include "src/ast/variable_decoration.h"
+#include "src/reader/spirv/function.h"
 #include "src/type_manager.h"
 
 namespace tint {
@@ -707,7 +708,7 @@ std::unique_ptr<ast::Variable> ParserImpl::MakeVariable(uint32_t id,
     return nullptr;
   }
 
-  auto ast_var = std::make_unique<ast::Variable>(Name(id), sc, type);
+  auto ast_var = std::make_unique<ast::Variable>(namer_.Name(id), sc, type);
 
   ast::VariableDecorationList ast_decorations;
   for (auto& deco : GetDecorationsFor(id)) {
@@ -745,50 +746,12 @@ bool ParserImpl::EmitFunctions() {
   }
   for (const auto* f :
        FunctionTraverser(*module_).TopologicallyOrderedFunctions()) {
-    EmitFunction(*f);
+    if (!success_) {
-  }
+      return false;
-  return success_;
-}
-
-bool ParserImpl::EmitFunction(const spvtools::opt::Function& f) {
-  if (!success_) {
-    return false;
-  }
-  // We only care about functions with bodies.
-  if (f.cbegin() == f.cend()) {
-    return true;
-  }
-
-  const auto name = Name(f.result_id());
-  // Surprisingly, the "type id" on an OpFunction is the result type of the
-  // function, not the type of the function.  This is the one exceptional case
-  // in SPIR-V where the type ID is not the type of the result ID.
-  auto* ret_ty = ConvertType(f.type_id());
-  if (!success_) {
-    return false;
-  }
-  if (ret_ty == nullptr) {
-    return Fail()
-           << "internal error: unregistered return type for function with ID "
-           << f.result_id();
-  }
-
-  ast::VariableList ast_params;
-  f.ForEachParam([this, &ast_params](const spvtools::opt::Instruction* param) {
-    auto* ast_type = ConvertType(param->type_id());
-    if (ast_type != nullptr) {
-      ast_params.emplace_back(
-          MakeVariable(param->result_id(), ast::StorageClass::kNone, ast_type));
     }
-  });
+    FunctionEmitter emitter(this, *f);
-  if (!success_) {
+    success_ = emitter.Emit();
-    return false;
   }
-
-  auto ast_fn =
-      std::make_unique<ast::Function>(name, std::move(ast_params), ret_ty);
-  ast_module_.AddFunction(std::move(ast_fn));
-
   return success_;
 }
 

src/reader/spirv/parser_impl.h

@@ -67,6 +67,10 @@ class ParserImpl : Reader {
   /// @returns the module. The module in the parser will be reset after this.
   ast::Module module() override;
 
+  /// Returns a pointer to the module, without resetting it.
+  /// @returns the module
+  ast::Module& get_module() { return ast_module_; }
+
   /// Logs failure, ands return a failure stream to accumulate diagnostic
   /// messages. By convention, a failure should only be logged along with
   /// a non-empty string diagnostic.
@@ -100,8 +104,13 @@ class ParserImpl : Reader {
   /// @returns a Tint type, or nullptr
   ast::type::Type* ConvertType(uint32_t type_id);
 
+  /// @returns the fail stream object
+  FailStream& fail_stream() { return fail_stream_; }
   /// @returns the namer object
   Namer& namer() { return namer_; }
+  /// @returns a borrowed pointer to the internal representation of the module.
+  /// This is null until BuildInternalModule has been called.
+  spvtools::opt::IRContext* ir_context() { return ir_context_.get(); }
 
   /// Gets the list of decorations for a SPIR-V result ID.  Returns an empty
   /// vector if the ID is not a result ID, or if no decorations target that ID.
@@ -189,15 +198,17 @@ class ParserImpl : Reader {
   /// @returns true if parser is still successful.
   bool EmitFunction(const spvtools::opt::Function& f);
 
- private:
+  /// Creates an AST Variable node for a SPIR-V ID, including any attached
-  /// @returns a name for the given ID. Generates a name if non exists.
+  /// decorations.
-  std::string Name(uint32_t id) {
+  /// @param id the SPIR-V result ID
-    if (!namer_.HasName(id)) {
+  /// @param sc the storage class, which can be ast::StorageClass::kNone
-      namer_.SuggestSanitizedName(id, "x_" + std::to_string(id));
+  /// @param type the type
-    }
+  /// @returns a new Variable node, or null in the error case
-    return namer_.GetName(id);
+  std::unique_ptr<ast::Variable> MakeVariable(uint32_t id,
-  }
+                                              ast::StorageClass sc,
+                                              ast::type::Type* type);
 
+ private:
   /// Converts a specific SPIR-V type to a Tint type. Integer case
   ast::type::Type* ConvertType(const spvtools::opt::analysis::Integer* int_ty);
   /// Converts a specific SPIR-V type to a Tint type. Float case
@@ -217,16 +228,6 @@ class ParserImpl : Reader {
   /// Converts a specific SPIR-V type to a Tint type. Pointer case
   ast::type::Type* ConvertType(const spvtools::opt::analysis::Pointer* ptr_ty);
 
-  /// Creates an AST Variable node for a SPIR-V ID, including any attached
-  /// decorations.
-  /// @param id the SPIR-V result ID
-  /// @param sc the storage class, which can be ast::StorageClass::kNone
-  /// @param type the type
-  /// @returns a new Variable node, or null in the error case
-  std::unique_ptr<ast::Variable> MakeVariable(uint32_t id,
-                                              ast::StorageClass sc,
-                                              ast::type::Type* type);
-
   // The SPIR-V binary we're parsing
   std::vector<uint32_t> spv_binary_;