From 80c94a4fa854c65522c3869f4b991e57b24d3d9b Mon Sep 17 00:00:00 2001 From: Lei Zhang Date: Tue, 9 Aug 2016 19:20:35 -0400 Subject: [PATCH] Change the interfaces of in-memory representation to use pointers. Previously we use vectors of objects and move semantics to handle ownership. That approach has the flaw that inserting an object into the middle of a vector, which may trigger a vector reallocation, can invalidate some addresses taken from instructions. Now the in-memory representation internally uses vector of unique pointers to handle ownership. Since objects are explicitly heap- allocated now, pointers to them won't be invalidated by vector resizing anymore. --- source/opt/basic_block.h | 28 +++++---- source/opt/function.cpp | 12 ++-- source/opt/function.h | 39 +++++++++---- source/opt/ir_loader.cpp | 17 +++--- source/opt/module.cpp | 44 +++++++------- source/opt/module.h | 126 ++++++++++++++++++++++++++++------------- source/val/BasicBlock.h | 1 + test/opt/test_pass_manager.cpp | 6 +- 8 files changed, 172 insertions(+), 101 deletions(-) diff --git a/source/opt/basic_block.h b/source/opt/basic_block.h index a1f47c6..360eaef 100644 --- a/source/opt/basic_block.h +++ b/source/opt/basic_block.h @@ -31,8 +31,9 @@ #define LIBSPIRV_OPT_BASIC_BLOCK_H_ #include -#include +#include #include +#include #include "instruction.h" @@ -46,13 +47,13 @@ class BasicBlock { public: // Creates a basic block with the given enclosing |function| and starting // |label|. - BasicBlock(Instruction&& label) + BasicBlock(std::unique_ptr label) : function_(nullptr), label_(std::move(label)) {} // Sets the enclosing function for this basic block. void SetParent(Function* function) { function_ = function; } // Appends an instruction to this basic block. - void AddInstruction(Instruction&& i) { insts_.push_back(std::move(i)); } + inline void AddInstruction(std::unique_ptr i); // Runs the given function |f| on each instruction in this basic block. inline void ForEachInst(const std::function& f); @@ -62,21 +63,28 @@ class BasicBlock { inline void ToBinary(std::vector* binary, bool skip_nop) const; private: - Function* function_; // The enclosing function. - Instruction label_; // The label starting this basic block. - std::vector insts_; // Instructions inside this basic block. + // The enclosing function. + Function* function_; + // The label starting this basic block. + std::unique_ptr label_; + // Instructions inside this basic block. + std::vector> insts_; }; +inline void BasicBlock::AddInstruction(std::unique_ptr i) { + insts_.emplace_back(std::move(i)); +} + inline void BasicBlock::ForEachInst( const std::function& f) { - label_.ForEachInst(f); - for (auto& inst : insts_) f(&inst); + label_->ForEachInst(f); + for (auto& inst : insts_) f(inst.get()); } inline void BasicBlock::ToBinary(std::vector* binary, bool skip_nop) const { - label_.ToBinary(binary, skip_nop); - for (const auto& inst : insts_) inst.ToBinary(binary, skip_nop); + label_->ToBinary(binary, skip_nop); + for (const auto& inst : insts_) inst->ToBinary(binary, skip_nop); } } // namespace ir diff --git a/source/opt/function.cpp b/source/opt/function.cpp index d013fe5..335ccbc 100644 --- a/source/opt/function.cpp +++ b/source/opt/function.cpp @@ -30,16 +30,16 @@ namespace spvtools { namespace ir { void Function::ForEachInst(const std::function& f) { - def_inst_.ForEachInst(f); - for (auto& param : params_) param.ForEachInst(f); - for (auto& bb : blocks_) bb.ForEachInst(f); + def_inst_->ForEachInst(f); + for (auto& param : params_) param->ForEachInst(f); + for (auto& bb : blocks_) bb->ForEachInst(f); end_inst_.ForEachInst(f); } void Function::ToBinary(std::vector* binary, bool skip_nop) const { - def_inst_.ToBinary(binary, skip_nop); - for (const auto& param : params_) param.ToBinary(binary, skip_nop); - for (const auto& bb : blocks_) bb.ToBinary(binary, skip_nop); + def_inst_->ToBinary(binary, skip_nop); + for (const auto& param : params_) param->ToBinary(binary, skip_nop); + for (const auto& bb : blocks_) bb->ToBinary(binary, skip_nop); end_inst_.ToBinary(binary, skip_nop); } diff --git a/source/opt/function.h b/source/opt/function.h index d7c01e1..4249768 100644 --- a/source/opt/function.h +++ b/source/opt/function.h @@ -28,8 +28,9 @@ #define LIBSPIRV_OPT_CONSTRUCTS_H_ #include -#include +#include #include +#include #include "basic_block.h" #include "instruction.h" @@ -42,7 +43,8 @@ class Module; // A SPIR-V function. class Function { public: - Function(Instruction&& def_inst) + // Creates a function instance declared by the given instruction |def_inst|. + Function(std::unique_ptr def_inst) : module_(nullptr), def_inst_(std::move(def_inst)), end_inst_(SpvOpFunctionEnd) {} @@ -50,12 +52,14 @@ class Function { // Sets the enclosing module for this function. void SetParent(Module* module) { module_ = module; } // Appends a parameter to this function. - void AddParameter(Instruction&& p) { params_.push_back(std::move(p)); } + inline void AddParameter(std::unique_ptr p); // Appends a basic block to this function. - void AddBasicBlock(BasicBlock&& b) { blocks_.push_back(std::move(b)); } + inline void AddBasicBlock(std::unique_ptr b); - const std::vector& basic_blocks() const { return blocks_; } - std::vector& basic_blocks() { return blocks_; } + const std::vector>& basic_blocks() const { + return blocks_; + } + std::vector>& basic_blocks() { return blocks_; } // Runs the given function |f| on each instruction in this basic block. void ForEachInst(const std::function& f); @@ -65,13 +69,26 @@ class Function { void ToBinary(std::vector* binary, bool skip_nop) const; private: - Module* module_; // The enclosing module. - Instruction def_inst_; // The instruction definining this function. - std::vector params_; // All parameters to this function. - std::vector blocks_; // All basic blocks inside this function. - Instruction end_inst_; // The OpFunctionEnd instruction. + // The enclosing module. + Module* module_; + // The OpFunction instruction that begins the definition of this function. + std::unique_ptr def_inst_; + // All parameters to this function. + std::vector> params_; + // All basic blocks inside this function. + std::vector> blocks_; + // The OpFunctionEnd instruction. + Instruction end_inst_; }; +inline void Function::AddParameter(std::unique_ptr p) { + params_.emplace_back(std::move(p)); +} + +inline void Function::AddBasicBlock(std::unique_ptr b) { + blocks_.emplace_back(std::move(b)); +} + } // namespace ir } // namespace spvtools diff --git a/source/opt/ir_loader.cpp b/source/opt/ir_loader.cpp index 06265ef..3f8153c 100644 --- a/source/opt/ir_loader.cpp +++ b/source/opt/ir_loader.cpp @@ -40,7 +40,8 @@ void IrLoader::AddInstruction(const spv_parsed_instruction_t* inst) { return; } - Instruction spv_inst(*inst, std::move(dbg_line_info_)); + std::unique_ptr spv_inst( + new Instruction(*inst, std::move(dbg_line_info_))); dbg_line_info_.clear(); // Handle function and basic block boundaries first, then normal // instructions. @@ -51,7 +52,7 @@ void IrLoader::AddInstruction(const spv_parsed_instruction_t* inst) { } else if (opcode == SpvOpFunctionEnd) { assert(function_ != nullptr); assert(block_ == nullptr); - module_->AddFunction(std::move(*function_.release())); + module_->AddFunction(std::move(function_)); function_ = nullptr; } else if (opcode == SpvOpLabel) { assert(function_ != nullptr); @@ -61,7 +62,7 @@ void IrLoader::AddInstruction(const spv_parsed_instruction_t* inst) { assert(function_ != nullptr); assert(block_ != nullptr); block_->AddInstruction(std::move(spv_inst)); - function_->AddBasicBlock(std::move(*block_.release())); + function_->AddBasicBlock(std::move(block_)); block_ = nullptr; } else { if (function_ == nullptr) { // Outside function definition @@ -104,16 +105,12 @@ void IrLoader::AddInstruction(const spv_parsed_instruction_t* inst) { // Resolves internal references among the module, functions, basic blocks, etc. // This function should be called after adding all instructions. -// -// This concluding call is needed because the whole in memory representation is -// designed around rvalues and move semantics, which subject to pointer -// invalidation during module construction internally. void IrLoader::EndModule() { for (auto& function : module_->functions()) { - for (auto& bb : function.basic_blocks()) { - bb.SetParent(&function); + for (auto& bb : function->basic_blocks()) { + bb->SetParent(function.get()); } - function.SetParent(module_); + function->SetParent(module_); } } diff --git a/source/opt/module.cpp b/source/opt/module.cpp index 8f22553..67385e9 100644 --- a/source/opt/module.cpp +++ b/source/opt/module.cpp @@ -33,8 +33,8 @@ namespace ir { std::vector Module::types() { std::vector insts; for (uint32_t i = 0; i < types_values_.size(); ++i) { - if (IsTypeInst(types_values_[i].opcode())) - insts.push_back(&types_values_[i]); + if (IsTypeInst(types_values_[i]->opcode())) + insts.push_back(types_values_[i].get()); } return insts; }; @@ -42,23 +42,23 @@ std::vector Module::types() { std::vector Module::GetConstants() { std::vector insts; for (uint32_t i = 0; i < types_values_.size(); ++i) { - if (IsConstantInst(types_values_[i].opcode())) - insts.push_back(&types_values_[i]); + if (IsConstantInst(types_values_[i]->opcode())) + insts.push_back(types_values_[i].get()); } return insts; }; void Module::ForEachInst(const std::function& f) { - for (auto& i : capabilities_) f(&i); - for (auto& i : extensions_) f(&i); - for (auto& i : ext_inst_imports_) f(&i); + for (auto& i : capabilities_) f(i.get()); + for (auto& i : extensions_) f(i.get()); + for (auto& i : ext_inst_imports_) f(i.get()); if (memory_model_) f(memory_model_.get()); - for (auto& i : entry_points_) f(&i); - for (auto& i : execution_modes_) f(&i); - for (auto& i : debugs_) f(&i); - for (auto& i : annotations_) f(&i); - for (auto& i : types_values_) f(&i); - for (auto& i : functions_) i.ForEachInst(f); + for (auto& i : entry_points_) f(i.get()); + for (auto& i : execution_modes_) f(i.get()); + for (auto& i : debugs_) f(i.get()); + for (auto& i : annotations_) f(i.get()); + for (auto& i : types_values_) f(i.get()); + for (auto& i : functions_) i->ForEachInst(f); } void Module::ToBinary(std::vector* binary, bool skip_nop) const { @@ -70,16 +70,16 @@ void Module::ToBinary(std::vector* binary, bool skip_nop) const { binary->push_back(header_.reserved); // TODO(antiagainst): wow, looks like a duplication of the above. - for (const auto& c : capabilities_) c.ToBinary(binary, skip_nop); - for (const auto& e : extensions_) e.ToBinary(binary, skip_nop); - for (const auto& e : ext_inst_imports_) e.ToBinary(binary, skip_nop); + for (const auto& c : capabilities_) c->ToBinary(binary, skip_nop); + for (const auto& e : extensions_) e->ToBinary(binary, skip_nop); + for (const auto& e : ext_inst_imports_) e->ToBinary(binary, skip_nop); if (memory_model_) memory_model_->ToBinary(binary, skip_nop); - for (const auto& e : entry_points_) e.ToBinary(binary, skip_nop); - for (const auto& e : execution_modes_) e.ToBinary(binary, skip_nop); - for (const auto& d : debugs_) d.ToBinary(binary, skip_nop); - for (const auto& a : annotations_) a.ToBinary(binary, skip_nop); - for (const auto& t : types_values_) t.ToBinary(binary, skip_nop); - for (const auto& f : functions_) f.ToBinary(binary, skip_nop); + for (const auto& e : entry_points_) e->ToBinary(binary, skip_nop); + for (const auto& e : execution_modes_) e->ToBinary(binary, skip_nop); + for (const auto& d : debugs_) d->ToBinary(binary, skip_nop); + for (const auto& a : annotations_) a->ToBinary(binary, skip_nop); + for (const auto& t : types_values_) t->ToBinary(binary, skip_nop); + for (const auto& f : functions_) f->ToBinary(binary, skip_nop); } } // namespace ir diff --git a/source/opt/module.h b/source/opt/module.h index 2fe1d23..8eca515 100644 --- a/source/opt/module.h +++ b/source/opt/module.h @@ -57,51 +57,49 @@ class Module { // Sets the header to the given |header|. void SetHeader(const ModuleHeader& header) { header_ = header; } // Appends a capability instruction to this module. - void AddCapability(Instruction&& c) { capabilities_.push_back(std::move(c)); } + inline void AddCapability(std::unique_ptr c); // Appends an extension instruction to this module. - void AddExtension(Instruction&& e) { extensions_.push_back(std::move(e)); } + inline void AddExtension(std::unique_ptr e); // Appends an extended instruction set instruction to this module. - void AddExtInstImport(Instruction&& e) { - ext_inst_imports_.push_back(std::move(e)); - } - // Adds a memory model instruction to this module. - void SetMemoryModel(Instruction&& m) { - memory_model_.reset(new Instruction(std::move(m))); - } + inline void AddExtInstImport(std::unique_ptr e); + // Set the memory model for this module. + inline void SetMemoryModel(std::unique_ptr m); // Appends an entry point instruction to this module. - void AddEntryPoint(Instruction&& e) { entry_points_.push_back(std::move(e)); } + inline void AddEntryPoint(std::unique_ptr e); // Appends an execution mode instruction to this module. - void AddExecutionMode(Instruction&& e) { - execution_modes_.push_back(std::move(e)); - } + inline void AddExecutionMode(std::unique_ptr e); // Appends a debug instruction (excluding OpLine & OpNoLine) to this module. - void AddDebugInst(Instruction&& d) { debugs_.push_back(std::move(d)); } + inline void AddDebugInst(std::unique_ptr d); // Appends an annotation instruction to this module. - void AddAnnotationInst(Instruction&& a) { - annotations_.push_back(std::move(a)); - } + inline void AddAnnotationInst(std::unique_ptr a); // Appends a type-declaration instruction to this module. - void AddType(Instruction&& t) { types_values_.push_back(std::move(t)); } + inline void AddType(std::unique_ptr t); // Appends a constant-creation instruction to this module. - void AddConstant(Instruction&& c) { types_values_.push_back(std::move(c)); } + inline void AddConstant(std::unique_ptr c); // Appends a global variable-declaration instruction to this module. - void AddGlobalVariable(Instruction&& v) { - types_values_.push_back(std::move(v)); - } + inline void AddGlobalVariable(std::unique_ptr v); // Appends a function to this module. - void AddFunction(Function&& f) { functions_.push_back(std::move(f)); } + inline void AddFunction(std::unique_ptr f); // Returns a vector of pointers to type-declaration instructions in this // module. std::vector types(); // Returns the constant-defining instructions. std::vector GetConstants(); - const std::vector& debugs() const { return debugs_; } - std::vector& debugs() { return debugs_; } - const std::vector& annotations() const { return annotations_; } - std::vector& annotations() { return annotations_; } - const std::vector& functions() const { return functions_; } - std::vector& functions() { return functions_; } + const std::vector>& debugs() const { + return debugs_; + } + std::vector>& debugs() { return debugs_; } + const std::vector>& annotations() const { + return annotations_; + } + std::vector>& annotations() { + return annotations_; + } + const std::vector>& functions() const { + return functions_; + } + std::vector>& functions() { return functions_; } // Invokes function |f| on all instructions in this module. void ForEachInst(const std::function& f); @@ -115,20 +113,68 @@ class Module { // The following fields respect the "Logical Layout of a Module" in // Section 2.4 of the SPIR-V specification. - std::vector capabilities_; - std::vector extensions_; - std::vector ext_inst_imports_; - std::unique_ptr - memory_model_; // A module only has one memory model instruction. - std::vector entry_points_; - std::vector execution_modes_; - std::vector debugs_; - std::vector annotations_; + std::vector> capabilities_; + std::vector> extensions_; + std::vector> ext_inst_imports_; + // A module only has one memory model instruction. + std::unique_ptr memory_model_; + std::vector> entry_points_; + std::vector> execution_modes_; + std::vector> debugs_; + std::vector> annotations_; // Type declarations, constants, and global variable declarations. - std::vector types_values_; - std::vector functions_; + std::vector> types_values_; + std::vector> functions_; }; +inline void Module::AddCapability(std::unique_ptr c) { + capabilities_.emplace_back(std::move(c)); +} + +inline void Module::AddExtension(std::unique_ptr e) { + extensions_.emplace_back(std::move(e)); +} + +inline void Module::AddExtInstImport(std::unique_ptr e) { + ext_inst_imports_.emplace_back(std::move(e)); +} + +inline void Module::SetMemoryModel(std::unique_ptr m) { + memory_model_ = std::move(m); +} + +inline void Module::AddEntryPoint(std::unique_ptr e) { + entry_points_.emplace_back(std::move(e)); +} + +inline void Module::AddExecutionMode(std::unique_ptr e) { + execution_modes_.emplace_back(std::move(e)); +} + +inline void Module::AddDebugInst(std::unique_ptr d) { + debugs_.emplace_back(std::move(d)); +} + +inline void Module::AddAnnotationInst(std::unique_ptr a) { + annotations_.emplace_back(std::move(a)); +} + +inline void Module::AddType(std::unique_ptr t) { + types_values_.emplace_back(std::move(t)); +} + +inline void Module::AddConstant(std::unique_ptr c) { + types_values_.emplace_back(std::move(c)); +} + +inline void Module::AddGlobalVariable(std::unique_ptr v) { + types_values_.emplace_back(std::move(v)); +} + +inline void Module::AddFunction(std::unique_ptr f) { + functions_.emplace_back(std::move(f)); +} + } // namespace ir } // namespace spvtools diff --git a/source/val/BasicBlock.h b/source/val/BasicBlock.h index 21b0e39..851e6dc 100644 --- a/source/val/BasicBlock.h +++ b/source/val/BasicBlock.h @@ -33,6 +33,7 @@ #include #include +#include #include namespace libspirv { diff --git a/test/opt/test_pass_manager.cpp b/test/opt/test_pass_manager.cpp index 1ecde39..b5268ab 100644 --- a/test/opt/test_pass_manager.cpp +++ b/test/opt/test_pass_manager.cpp @@ -54,7 +54,8 @@ TEST(PassManager, Interface) { class AppendOpNopPass : public opt::Pass { const char* name() const override { return "AppendOpNop"; } bool Process(ir::Module* module) override { - module->AddDebugInst(ir::Instruction()); + std::unique_ptr inst(new ir::Instruction()); + module->AddDebugInst(std::move(inst)); return true; } }; @@ -63,7 +64,8 @@ class AppendOpNopPass : public opt::Pass { class DuplicateInstPass : public opt::Pass { const char* name() const override { return "DuplicateInst"; } bool Process(ir::Module* module) override { - ir::Instruction inst = module->debugs().back(); + std::unique_ptr inst( + new ir::Instruction(*module->debugs().back())); module->AddDebugInst(std::move(inst)); return true; } -- 2.7.4