ForEachInst optionally runs on attached debug line insts

Also:
- Add const forms of ForEachInst
- Rewrite Module::ToBinary in terms of ForEachInst
- Add Instruction::ToBinaryWithoutAttachedDebugInsts
- Delete the ToBinary method on Function, BasicBlock, and Instruction
  since it can now be implemented with ForEachInst in a less confusing
  way, e.g. without recursion.
- Preserve debug line instructions on OpFunctionEnd (and store that
  instruction as a unique-pointer, for regularity).

source/opt/basic_block.h

@@ -62,19 +62,19 @@ class BasicBlock {
   const_iterator cbegin() { return const_iterator(&insts_, insts_.cbegin()); }
   const_iterator cend() { return const_iterator(&insts_, insts_.cend()); }
 
-  // Runs the given function |f| on each instruction in this basic block.
-  inline void ForEachInst(const std::function<void(Instruction*)>& f);
-
-  // Pushes the binary segments for this instruction into the back of *|binary|.
-  // If |skip_nop| is true and this is a OpNop, do nothing.
-  inline void ToBinary(std::vector<uint32_t>* binary, bool skip_nop) const;
+  // Runs the given function |f| on each instruction in this basic block, and
+  // optionally on the debug line instructions that might precede them.
+  inline void ForEachInst(const std::function<void(Instruction*)>& f,
+                          bool run_on_debug_line_insts = false);
+  inline void ForEachInst(const std::function<void(const Instruction*)>& f,
+                          bool run_on_debug_line_insts = false) const;
 
  private:
   // The enclosing function.
   Function* function_;
   // The label starting this basic block.
   std::unique_ptr<Instruction> label_;
-  // Instructions inside this basic block.
+  // Instructions inside this basic block, but not the OpLabel.
   std::vector<std::unique_ptr<Instruction>> insts_;
 };
 
@@ -85,16 +85,20 @@ 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.get());
+inline void BasicBlock::ForEachInst(const std::function<void(Instruction*)>& f,
+                                    bool run_on_debug_line_insts) {
+  label_->ForEachInst(f, run_on_debug_line_insts);
+  for (auto& inst : insts_) inst->ForEachInst(f, run_on_debug_line_insts);
 }
 
-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);
+inline void BasicBlock::ForEachInst(
+    const std::function<void(const Instruction*)>& f,
+    bool run_on_debug_line_insts) const {
+  static_cast<const Instruction*>(label_.get())
+      ->ForEachInst(f, run_on_debug_line_insts);
+  for (const auto& inst : insts_)
+    static_cast<const Instruction*>(inst.get())
+        ->ForEachInst(f, run_on_debug_line_insts);
 }
 
 }  // namespace ir

source/opt/function.cpp

@@ -29,18 +29,29 @@
 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);
-  end_inst_.ForEachInst(f);
+void Function::ForEachInst(const std::function<void(Instruction*)>& f,
+                           bool run_on_debug_line_insts) {
+  def_inst_->ForEachInst(f, run_on_debug_line_insts);
+  for (auto& param : params_) param->ForEachInst(f, run_on_debug_line_insts);
+  for (auto& bb : blocks_) bb->ForEachInst(f, run_on_debug_line_insts);
+  end_inst_->ForEachInst(f, run_on_debug_line_insts);
 }
 
-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);
-  end_inst_.ToBinary(binary, skip_nop);
+void Function::ForEachInst(const std::function<void(const Instruction*)>& f,
+                           bool run_on_debug_line_insts) const {
+  static_cast<const Instruction*>(def_inst_.get())
+      ->ForEachInst(f, run_on_debug_line_insts);
+
+  for (const auto& param : params_)
+    static_cast<const Instruction*>(param.get())
+        ->ForEachInst(f, run_on_debug_line_insts);
+
+  for (const auto& bb : blocks_)
+    static_cast<const BasicBlock*>(bb.get())->ForEachInst(
+        f, run_on_debug_line_insts);
+
+  static_cast<const Instruction*>(end_inst_.get())
+      ->ForEachInst(f, run_on_debug_line_insts);
 }
 
 }  // namespace ir

source/opt/function.h

@@ -58,17 +58,20 @@ class Function {
   // Appends a basic block to this function.
   inline void AddBasicBlock(std::unique_ptr<BasicBlock> b);
 
+  // Saves the given function end instruction.
+  inline void SetFunctionEnd(std::unique_ptr<Instruction> end_inst);
+
   iterator begin() { return iterator(&blocks_, blocks_.begin()); }
   iterator end() { return iterator(&blocks_, blocks_.end()); }
   const_iterator cbegin() { return const_iterator(&blocks_, blocks_.cbegin()); }
   const_iterator cend() { return const_iterator(&blocks_, blocks_.cend()); }
 
-  // Runs the given function |f| on each instruction in this basic block.
-  void ForEachInst(const std::function<void(Instruction*)>& f);
-
-  // Pushes the binary segments for this instruction into the back of *|binary|.
-  // If |skip_nop| is true and this is a OpNop, do nothing.
-  void ToBinary(std::vector<uint32_t>* binary, bool skip_nop) const;
+  // Runs the given function |f| on each instruction in this function, and
+  // optionally on debug line instructions that might precede them.
+  void ForEachInst(const std::function<void(Instruction*)>& f,
+                   bool run_on_debug_line_insts = false);
+  void ForEachInst(const std::function<void(const Instruction*)>& f,
+                   bool run_on_debug_line_insts = false) const;
 
  private:
   // The enclosing module.
@@ -80,13 +83,11 @@ class Function {
   // All basic blocks inside this function.
   std::vector<std::unique_ptr<BasicBlock>> blocks_;
   // The OpFunctionEnd instruction.
-  Instruction end_inst_;
+  std::unique_ptr<Instruction> end_inst_;
 };
 
 inline Function::Function(std::unique_ptr<Instruction> def_inst)
-    : module_(nullptr),
-      def_inst_(std::move(def_inst)),
-      end_inst_(SpvOpFunctionEnd) {}
+    : module_(nullptr), def_inst_(std::move(def_inst)), end_inst_() {}
 
 inline void Function::AddParameter(std::unique_ptr<Instruction> p) {
   params_.emplace_back(std::move(p));
@@ -96,6 +97,10 @@ inline void Function::AddBasicBlock(std::unique_ptr<BasicBlock> b) {
   blocks_.emplace_back(std::move(b));
 }
 
+inline void Function::SetFunctionEnd(std::unique_ptr<Instruction> end_inst) {
+  end_inst_ = std::move(end_inst);
+}
+
 }  // namespace ir
 }  // namespace spvtools
 

source/opt/instruction.cpp

@@ -94,13 +94,8 @@ uint32_t Instruction::NumInOperandWords() const {
   return size;
 }
 
-void Instruction::ToBinary(std::vector<uint32_t>* binary, bool skip_nop) const {
-  for (const auto& dbg_line : dbg_line_insts_) {
-    dbg_line.ToBinary(binary, skip_nop);
-  }
-
-  if (skip_nop && IsNop()) return;
-
+void Instruction::ToBinaryWithoutAttachedDebugInsts(
+    std::vector<uint32_t>* binary) const {
   const uint32_t num_words = 1 + NumOperandWords();
   binary->push_back((num_words << 16) | static_cast<uint16_t>(opcode_));
   for (const auto& operand : operands_)

source/opt/instruction.h

@@ -154,13 +154,16 @@ class Instruction {
   // line-related debug instructions.
   inline void ToNop();
 
-  // Runs the given function |f| on this instruction and preceding debug
-  // instructions.
-  inline void ForEachInst(const std::function<void(Instruction*)>& f);
+  // Runs the given function |f| on this instruction and optionally on the
+  // preceding debug line instructions.  The function will always be run
+  // if this is itself a debug line instruction.
+  inline void ForEachInst(const std::function<void(Instruction*)>& f,
+                          bool run_on_debug_line_insts = false);
+  inline void ForEachInst(const std::function<void(const Instruction*)>& f,
+                          bool run_on_debug_line_insts = false) const;
 
   // Pushes the binary segments for this instruction into the back of *|binary|.
-  // If |skip_nop| is true and this is a OpNop, do nothing.
-  void ToBinary(std::vector<uint32_t>* binary, bool skip_nop) const;
+  void ToBinaryWithoutAttachedDebugInsts(std::vector<uint32_t>* binary) const;
 
  private:
   // Returns the toal count of result type id and result id.
@@ -210,9 +213,18 @@ inline void Instruction::ToNop() {
   operands_.clear();
 }
 
+inline void Instruction::ForEachInst(const std::function<void(Instruction*)>& f,
+                                     bool run_on_debug_line_insts) {
+  if (run_on_debug_line_insts)
+    for (auto& dbg_line : dbg_line_insts_) f(&dbg_line);
+  f(this);
+}
+
 inline void Instruction::ForEachInst(
-    const std::function<void(Instruction*)>& f) {
-  for (auto& dbg_line : dbg_line_insts_) f(&dbg_line);
+    const std::function<void(const Instruction*)>& f,
+    bool run_on_debug_line_insts) const {
+  if (run_on_debug_line_insts)
+    for (auto& dbg_line : dbg_line_insts_) f(&dbg_line);
   f(this);
 }
 

source/opt/ir_loader.cpp

@@ -52,6 +52,7 @@ void IrLoader::AddInstruction(const spv_parsed_instruction_t* inst) {
   } else if (opcode == SpvOpFunctionEnd) {
     assert(function_ != nullptr);
     assert(block_ == nullptr);
+    function_->SetFunctionEnd(std::move(spv_inst));
     module_->AddFunction(std::move(function_));
     function_ = nullptr;
   } else if (opcode == SpvOpLabel) {

source/opt/module.cpp

@@ -66,17 +66,41 @@ std::vector<const Instruction*> Module::GetConstants() const {
   return insts;
 };
 
-void Module::ForEachInst(const std::function<void(Instruction*)>& f) {
-  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.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::ForEachInst(const std::function<void(Instruction*)>& f,
+                         bool run_on_debug_line_insts) {
+#define DELEGATE(i) i->ForEachInst(f, run_on_debug_line_insts)
+  for (auto& i : capabilities_) DELEGATE(i);
+  for (auto& i : extensions_) DELEGATE(i);
+  for (auto& i : ext_inst_imports_) DELEGATE(i);
+  if (memory_model_) DELEGATE(memory_model_);
+  for (auto& i : entry_points_) DELEGATE(i);
+  for (auto& i : execution_modes_) DELEGATE(i);
+  for (auto& i : debugs_) DELEGATE(i);
+  for (auto& i : annotations_) DELEGATE(i);
+  for (auto& i : types_values_) DELEGATE(i);
+  for (auto& i : functions_) DELEGATE(i);
+#undef DELEGATE
+}
+
+void Module::ForEachInst(const std::function<void(const Instruction*)>& f,
+                         bool run_on_debug_line_insts) const {
+#define DELEGATE(i)                                      \
+  static_cast<const Instruction*>(i.get())->ForEachInst( \
+      f, run_on_debug_line_insts)
+  for (auto& i : capabilities_) DELEGATE(i);
+  for (auto& i : extensions_) DELEGATE(i);
+  for (auto& i : ext_inst_imports_) DELEGATE(i);
+  if (memory_model_) DELEGATE(memory_model_);
+  for (auto& i : entry_points_) DELEGATE(i);
+  for (auto& i : execution_modes_) DELEGATE(i);
+  for (auto& i : debugs_) DELEGATE(i);
+  for (auto& i : annotations_) DELEGATE(i);
+  for (auto& i : types_values_) DELEGATE(i);
+  for (auto& i : functions_) {
+    static_cast<const Function*>(i.get())->ForEachInst(f,
+                                                       run_on_debug_line_insts);
+  }
+#undef DELEGATE
 }
 
 void Module::ToBinary(std::vector<uint32_t>* binary, bool skip_nop) const {
@@ -87,17 +111,10 @@ void Module::ToBinary(std::vector<uint32_t>* binary, bool skip_nop) const {
   binary->push_back(header_.bound);
   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);
-  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);
+  auto write_inst = [this, binary, skip_nop](const Instruction* i) {
+    if (!skip_nop || !i->IsNop()) i->ToBinaryWithoutAttachedDebugInsts(binary);
+  };
+  ForEachInst(write_inst, true);
 }
 
 }  // namespace ir

source/opt/module.h

@@ -122,8 +122,12 @@ class Module {
   inline const_iterator cbegin() const;
   inline const_iterator cend() const;
 
-  // Invokes function |f| on all instructions in this module.
-  void ForEachInst(const std::function<void(Instruction*)>& f);
+  // Invokes function |f| on all instructions in this module, and optionally on
+  // the debug line instructions that precede them.
+  void ForEachInst(const std::function<void(Instruction*)>& f,
+                   bool run_on_debug_line_insts = false);
+  void ForEachInst(const std::function<void(const Instruction*)>& f,
+                   bool run_on_debug_line_insts = false) const;
 
   // Pushes the binary segments for this instruction into the back of *|binary|.
   // If |skip_nop| is true and this is a OpNop, do nothing.

test/opt/test_def_use.cpp

@@ -48,7 +48,7 @@ std::string DisassembleInst(ir::Instruction* inst) {
   std::vector<uint32_t> binary;
   // We need this to generate the necessary header in the binary.
   tools.Assemble("", &binary);
-  inst->ToBinary(&binary, /* skip_nop = */ false);
+  inst->ToBinaryWithoutAttachedDebugInsts(&binary);
 
   std::string text;
   // We'll need to check the underlying id numbers.

test/opt/test_ir_loader.cpp

@@ -36,7 +36,7 @@ using namespace spvtools;
 void DoRoundTripCheck(const std::string& text) {
   SpvTools t(SPV_ENV_UNIVERSAL_1_1);
   std::unique_ptr<ir::Module> module = t.BuildModule(text);
-  ASSERT_NE(nullptr, module);
+  ASSERT_NE(nullptr, module) << "Failed to assemble\n" << text;
 
   std::vector<uint32_t> binary;
   module->ToBinary(&binary, /* skip_nop = */ false);
@@ -244,4 +244,55 @@ TEST(IrBuilder, OpUndefInBasicBlock) {
   // clang-format on
 }
 
+TEST(IrBuilder, KeepLineDebugInfoBeforeType) {
+  DoRoundTripCheck(
+      // clang-format off
+               "OpCapability Shader\n"
+               "OpMemoryModel Logical GLSL450\n"
+          "%1 = OpString \"minimal.vert\"\n"
+               "OpLine %1 1 1\n"
+               "OpNoLine\n"
+       "%void = OpTypeVoid\n"
+               "OpLine %1 2 2\n"
+          "%3 = OpTypeFunction %void\n");
+  // clang-format on
+}
+
+TEST(IrBuilder, KeepLineDebugInfoBeforeLabel) {
+  DoRoundTripCheck(
+      // clang-format off
+               "OpCapability Shader\n"
+               "OpMemoryModel Logical GLSL450\n"
+          "%1 = OpString \"minimal.vert\"\n"
+       "%void = OpTypeVoid\n"
+          "%3 = OpTypeFunction %void\n"
+       "%4 = OpFunction %void None %3\n"
+          "%5 = OpLabel\n"
+   "OpBranch %6\n"
+               "OpLine %1 1 1\n"
+               "OpLine %1 2 2\n"
+          "%6 = OpLabel\n"
+               "OpBranch %7\n"
+               "OpLine %1 100 100\n"
+          "%7 = OpLabel\n"
+               "OpReturn\n"
+               "OpFunctionEnd\n");
+  // clang-format on
+}
+
+TEST(IrBuilder, KeepLineDebugInfoBeforeFunctionEnd) {
+  DoRoundTripCheck(
+      // clang-format off
+               "OpCapability Shader\n"
+               "OpMemoryModel Logical GLSL450\n"
+          "%1 = OpString \"minimal.vert\"\n"
+       "%void = OpTypeVoid\n"
+          "%3 = OpTypeFunction %void\n"
+       "%4 = OpFunction %void None %3\n"
+               "OpLine %1 1 1\n"
+               "OpLine %1 2 2\n"
+               "OpFunctionEnd\n");
+  // clang-format on
+}
+
 }  // anonymous namespace