[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>
diff --git a/src/CMakeLists.txt b/src/CMakeLists.txt
index 102f26a..f8c57cd 100644
--- a/src/CMakeLists.txt
+++ b/src/CMakeLists.txt
@@ -204,6 +204,8 @@
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
diff --git a/src/reader/spirv/function.cc b/src/reader/spirv/function.cc
new file mode 100644
index 0000000..28a710c
--- /dev/null
+++ b/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
diff --git a/src/reader/spirv/function.h b/src/reader/spirv/function.h
new file mode 100644
index 0000000..9e1347b
--- /dev/null
+++ b/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_
diff --git a/src/reader/spirv/namer.h b/src/reader/spirv/namer.h
index bb9c6a6..e76fc2b 100644
--- a/src/reader/spirv/namer.h
+++ b/src/reader/spirv/namer.h
@@ -32,7 +32,7 @@
/// 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 @@
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.
diff --git a/src/reader/spirv/namer_test.cc b/src/reader/spirv/namer_test.cc
index 4f94907..ef5e4ac 100644
--- a/src/reader/spirv/namer_test.cc
+++ b/src/reader/spirv/namer_test.cc
@@ -216,6 +216,23 @@
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_);
diff --git a/src/reader/spirv/parser_impl.cc b/src/reader/spirv/parser_impl.cc
index 0dc415e..ca89a05 100644
--- a/src/reader/spirv/parser_impl.cc
+++ b/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 @@
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 @@
}
for (const auto* f :
FunctionTraverser(*module_).TopologicallyOrderedFunctions()) {
- EmitFunction(*f);
- }
- 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));
+ if (!success_) {
+ return false;
}
- });
- if (!success_) {
- return false;
+ FunctionEmitter emitter(this, *f);
+ success_ = emitter.Emit();
}
-
- auto ast_fn =
- std::make_unique<ast::Function>(name, std::move(ast_params), ret_ty);
- ast_module_.AddFunction(std::move(ast_fn));
-
return success_;
}
diff --git a/src/reader/spirv/parser_impl.h b/src/reader/spirv/parser_impl.h
index f9a6e45..46d9c7f 100644
--- a/src/reader/spirv/parser_impl.h
+++ b/src/reader/spirv/parser_impl.h
@@ -67,6 +67,10 @@
/// @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 @@
/// @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 @@
/// @returns true if parser is still successful.
bool EmitFunction(const spvtools::opt::Function& f);
- private:
- /// @returns a name for the given ID. Generates a name if non exists.
- std::string Name(uint32_t id) {
- if (!namer_.HasName(id)) {
- namer_.SuggestSanitizedName(id, "x_" + std::to_string(id));
- }
- return namer_.GetName(id);
- }
+ /// 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);
+ 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 @@
/// 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_;
