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.

diff --git a/source/opt/basic_block.h b/source/opt/basic_block.h
index a1f47c6..360eaef 100644 (file)
--- a/source/opt/basic_block.h
+++ b/source/opt/basic_block.h
@@ -31,8 +31,9 @@
 #define LIBSPIRV_OPT_BASIC_BLOCK_H_
 
 #include <functional>
-#include <vector>
+#include <memory>
 #include <utility>
+#include <vector>
 
 #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<Instruction> 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<Instruction> i);
 
   // Runs the given function |f| on each instruction in this basic block.
   inline void ForEachInst(const std::function<void(Instruction*)>& f);
@@ -62,21 +63,28 @@ class BasicBlock {
   inline void ToBinary(std::vector<uint32_t>* binary, bool skip_nop) const;
 
  private:
-  Function* function_;              // The enclosing function.
-  Instruction label_;               // The label starting this basic block.
-  std::vector<Instruction> insts_;  // Instructions inside this basic block.
+  // The enclosing function.
+  Function* function_;
+  // The label starting this basic block.
+  std::unique_ptr<Instruction> label_;
+  // Instructions inside this basic block.
+  std::vector<std::unique_ptr<Instruction>> insts_;
 };
 
+inline void BasicBlock::AddInstruction(std::unique_ptr<Instruction> i) {
+  insts_.emplace_back(std::move(i));
+}
+
 inline void BasicBlock::ForEachInst(
     const std::function<void(Instruction*)>& 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<uint32_t>* 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 (file)
--- a/source/opt/function.cpp
+++ b/source/opt/function.cpp
@@ -30,16 +30,16 @@ namespace spvtools {
 namespace ir {
 
 void Function::ForEachInst(const std::function<void(Instruction*)>& 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<uint32_t>* 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 (file)
--- a/source/opt/function.h
+++ b/source/opt/function.h
@@ -28,8 +28,9 @@
 #define LIBSPIRV_OPT_CONSTRUCTS_H_
 
 #include <functional>
-#include <vector>
+#include <memory>
 #include <utility>
+#include <vector>
 
 #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<Instruction> 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<Instruction> p);
   // Appends a basic block to this function.
-  void AddBasicBlock(BasicBlock&& b) { blocks_.push_back(std::move(b)); }
+  inline void AddBasicBlock(std::unique_ptr<BasicBlock> b);
 
-  const std::vector<BasicBlock>& basic_blocks() const { return blocks_; }
-  std::vector<BasicBlock>& basic_blocks() { return blocks_; }
+  const std::vector<std::unique_ptr<BasicBlock>>& basic_blocks() const {
+    return blocks_;
+  }
+  std::vector<std::unique_ptr<BasicBlock>>& basic_blocks() { return blocks_; }
 
   // Runs the given function |f| on each instruction in this basic block.
   void ForEachInst(const std::function<void(Instruction*)>& f);
@@ -65,13 +69,26 @@ class Function {
   void ToBinary(std::vector<uint32_t>* binary, bool skip_nop) const;
 
  private:
-  Module* module_;        // The enclosing module.
-  Instruction def_inst_;  // The instruction definining this function.
-  std::vector<Instruction> params_;  // All parameters to this function.
-  std::vector<BasicBlock> 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<Instruction> def_inst_;
+  // All parameters to this function.
+  std::vector<std::unique_ptr<Instruction>> params_;
+  // All basic blocks inside this function.
+  std::vector<std::unique_ptr<BasicBlock>> blocks_;
+  // The OpFunctionEnd instruction.
+  Instruction end_inst_;
 };
 
+inline void Function::AddParameter(std::unique_ptr<Instruction> p) {
+  params_.emplace_back(std::move(p));
+}
+
+inline void Function::AddBasicBlock(std::unique_ptr<BasicBlock> 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 (file)
--- 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<Instruction> 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 (file)
--- a/source/opt/module.cpp
+++ b/source/opt/module.cpp
@@ -33,8 +33,8 @@ namespace ir {
 std::vector<Instruction*> Module::types() {
   std::vector<Instruction*> 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<Instruction*> Module::types() {
 std::vector<Instruction*> Module::GetConstants() {
   std::vector<Instruction*> 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<void(Instruction*)>& 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<uint32_t>* binary, bool skip_nop) const {
@@ -70,16 +70,16 @@ void Module::ToBinary(std::vector<uint32_t>* 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 (file)
--- 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<Instruction> c);
   // Appends an extension instruction to this module.
-  void AddExtension(Instruction&& e) { extensions_.push_back(std::move(e)); }
+  inline void AddExtension(std::unique_ptr<Instruction> 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<Instruction> e);
+  // Set the memory model for this module.
+  inline void SetMemoryModel(std::unique_ptr<Instruction> 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<Instruction> 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<Instruction> 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<Instruction> d);
   // Appends an annotation instruction to this module.
-  void AddAnnotationInst(Instruction&& a) {
-    annotations_.push_back(std::move(a));
-  }
+  inline void AddAnnotationInst(std::unique_ptr<Instruction> 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<Instruction> 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<Instruction> 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<Instruction> v);
   // Appends a function to this module.
-  void AddFunction(Function&& f) { functions_.push_back(std::move(f)); }
+  inline void AddFunction(std::unique_ptr<Function> f);
 
   // Returns a vector of pointers to type-declaration instructions in this
   // module.
   std::vector<Instruction*> types();
   // Returns the constant-defining instructions.
   std::vector<Instruction*> GetConstants();
-  const std::vector<Instruction>& debugs() const { return debugs_; }
-  std::vector<Instruction>& debugs() { return debugs_; }
-  const std::vector<Instruction>& annotations() const { return annotations_; }
-  std::vector<Instruction>& annotations() { return annotations_; }
-  const std::vector<Function>& functions() const { return functions_; }
-  std::vector<Function>& functions() { return functions_; }
+  const std::vector<std::unique_ptr<Instruction>>& debugs() const {
+    return debugs_;
+  }
+  std::vector<std::unique_ptr<Instruction>>& debugs() { return debugs_; }
+  const std::vector<std::unique_ptr<Instruction>>& annotations() const {
+    return annotations_;
+  }
+  std::vector<std::unique_ptr<Instruction>>& annotations() {
+    return annotations_;
+  }
+  const std::vector<std::unique_ptr<Function>>& functions() const {
+    return functions_;
+  }
+  std::vector<std::unique_ptr<Function>>& functions() { return functions_; }
 
   // Invokes function |f| on all instructions in this module.
   void ForEachInst(const std::function<void(Instruction*)>& 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<Instruction> capabilities_;
-  std::vector<Instruction> extensions_;
-  std::vector<Instruction> ext_inst_imports_;
-  std::unique_ptr<Instruction>
-      memory_model_;  // A module only has one memory model instruction.
-  std::vector<Instruction> entry_points_;
-  std::vector<Instruction> execution_modes_;
-  std::vector<Instruction> debugs_;
-  std::vector<Instruction> annotations_;
+  std::vector<std::unique_ptr<Instruction>> capabilities_;
+  std::vector<std::unique_ptr<Instruction>> extensions_;
+  std::vector<std::unique_ptr<Instruction>> ext_inst_imports_;
+  // A module only has one memory model instruction.
+  std::unique_ptr<Instruction> memory_model_;
+  std::vector<std::unique_ptr<Instruction>> entry_points_;
+  std::vector<std::unique_ptr<Instruction>> execution_modes_;
+  std::vector<std::unique_ptr<Instruction>> debugs_;
+  std::vector<std::unique_ptr<Instruction>> annotations_;
   // Type declarations, constants, and global variable declarations.
-  std::vector<Instruction> types_values_;
-  std::vector<Function> functions_;
+  std::vector<std::unique_ptr<Instruction>> types_values_;
+  std::vector<std::unique_ptr<Function>> functions_;
 };
 
+inline void Module::AddCapability(std::unique_ptr<Instruction> c) {
+  capabilities_.emplace_back(std::move(c));
+}
+
+inline void Module::AddExtension(std::unique_ptr<Instruction> e) {
+  extensions_.emplace_back(std::move(e));
+}
+
+inline void Module::AddExtInstImport(std::unique_ptr<Instruction> e) {
+  ext_inst_imports_.emplace_back(std::move(e));
+}
+
+inline void Module::SetMemoryModel(std::unique_ptr<Instruction> m) {
+  memory_model_ = std::move(m);
+}
+
+inline void Module::AddEntryPoint(std::unique_ptr<Instruction> e) {
+  entry_points_.emplace_back(std::move(e));
+}
+
+inline void Module::AddExecutionMode(std::unique_ptr<Instruction> e) {
+  execution_modes_.emplace_back(std::move(e));
+}
+
+inline void Module::AddDebugInst(std::unique_ptr<Instruction> d) {
+  debugs_.emplace_back(std::move(d));
+}
+
+inline void Module::AddAnnotationInst(std::unique_ptr<Instruction> a) {
+  annotations_.emplace_back(std::move(a));
+}
+
+inline void Module::AddType(std::unique_ptr<Instruction> t) {
+  types_values_.emplace_back(std::move(t));
+}
+
+inline void Module::AddConstant(std::unique_ptr<Instruction> c) {
+  types_values_.emplace_back(std::move(c));
+}
+
+inline void Module::AddGlobalVariable(std::unique_ptr<Instruction> v) {
+  types_values_.emplace_back(std::move(v));
+}
+
+inline void Module::AddFunction(std::unique_ptr<Function> 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 (file)
--- a/source/val/BasicBlock.h
+++ b/source/val/BasicBlock.h
@@ -33,6 +33,7 @@
 
 #include <bitset>
 #include <functional>
+#include <memory>
 #include <vector>
 
 namespace libspirv {

diff --git a/test/opt/test_pass_manager.cpp b/test/opt/test_pass_manager.cpp
index 1ecde39..b5268ab 100644 (file)
--- 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<ir::Instruction> 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<ir::Instruction> inst(
+        new ir::Instruction(*module->debugs().back()));
     module->AddDebugInst(std::move(inst));
     return true;
   }