BuildModule: optionally avoid adding new OpLine instructions (#4033)

* BuildModule: optionally avoid adding new OpLine instructions

Fixes #4029 for my use case

* Fix formatting

* Create last_line_inst_ only if doing extra line tracking

source/opt/build_module.cpp

@@ -50,11 +50,20 @@ std::unique_ptr<opt::IRContext> BuildModule(spv_target_env env,
                                             MessageConsumer consumer,
                                             const uint32_t* binary,
                                             const size_t size) {
+  return BuildModule(env, consumer, binary, size, true);
+}
+
+std::unique_ptr<opt::IRContext> BuildModule(spv_target_env env,
+                                            MessageConsumer consumer,
+                                            const uint32_t* binary,
+                                            const size_t size,
+                                            bool extra_line_tracking) {
   auto context = spvContextCreate(env);
   SetContextMessageConsumer(context, consumer);
 
   auto irContext = MakeUnique<opt::IRContext>(env, consumer);
   opt::IrLoader loader(consumer, irContext->module());
+  loader.SetExtraLineTracking(extra_line_tracking);
 
   spv_result_t status = spvBinaryParse(context, &loader, binary, size,
                                        SetSpvHeader, SetSpvInst, nullptr);

source/opt/build_module.h

@@ -27,7 +27,15 @@ namespace spvtools {
 // Builds an Module returns the owning IRContext from the given SPIR-V
 // |binary|. |size| specifies number of words in |binary|. The |binary| will be
 // decoded according to the given target |env|. Returns nullptr if errors occur
-// and sends the errors to |consumer|.
+// and sends the errors to |consumer|.  When |extra_line_tracking| is true,
+// extra OpLine instructions are injected to better presere line numbers while
+// later transforms mutate the module.
+std::unique_ptr<opt::IRContext> BuildModule(spv_target_env env,
+                                            MessageConsumer consumer,
+                                            const uint32_t* binary, size_t size,
+                                            bool extra_line_tracking);
+
+// Like above, with extra line tracking turned on.
 std::unique_ptr<opt::IRContext> BuildModule(spv_target_env env,
                                             MessageConsumer consumer,
                                             const uint32_t* binary,

source/opt/ir_loader.cpp

@@ -92,7 +92,8 @@ bool IrLoader::AddInstruction(const spv_parsed_instruction_t* inst) {
   std::unique_ptr<Instruction> spv_inst(
       new Instruction(module()->context(), *inst, std::move(dbg_line_info_)));
   if (!spv_inst->dbg_line_insts().empty()) {
-    if (spv_inst->dbg_line_insts().back().opcode() != SpvOpNoLine) {
+    if (extra_line_tracking_ &&
+        (spv_inst->dbg_line_insts().back().opcode() != SpvOpNoLine)) {
       last_line_inst_ = std::unique_ptr<Instruction>(
           spv_inst->dbg_line_insts().back().Clone(module()->context()));
     }

source/opt/ir_loader.h

@@ -63,6 +63,10 @@ class IrLoader {
   // or a missing OpFunctionEnd.  Resolves internal bookkeeping.
   void EndModule();
 
+  // Sets whether extra OpLine instructions should be injected to better
+  // track line information.
+  void SetExtraLineTracking(bool flag) { extra_line_tracking_ = flag; }
+
  private:
   // Consumer for communicating messages to outside.
   const MessageConsumer& consumer_;
@@ -78,11 +82,17 @@ class IrLoader {
   std::unique_ptr<BasicBlock> block_;
   // Line related debug instructions accumulated thus far.
   std::vector<Instruction> dbg_line_info_;
-  // Line instruction that should be applied to the next instruction.
+  // If doing extra line tracking, this is the line instruction that should be
+  // applied to the next instruction.  Otherwise it always contains null.
   std::unique_ptr<Instruction> last_line_inst_;
 
   // The last DebugScope information that IrLoader::AddInstruction() handled.
   DebugScope last_dbg_scope_;
+
+  // When true, do extra line information tracking: Additional OpLine
+  // instructions will be injected to help track line info more robustly during
+  // transformations.
+  bool extra_line_tracking_ = true;
 };
 
 }  // namespace opt

test/opt/ir_loader_test.cpp

@@ -19,7 +19,7 @@
 #include <utility>
 #include <vector>
 
-#include "gtest/gtest.h"
+#include "gmock/gmock.h"
 #include "source/opt/build_module.h"
 #include "source/opt/def_use_manager.h"
 #include "source/opt/ir_context.h"
@@ -29,6 +29,8 @@ namespace spvtools {
 namespace opt {
 namespace {
 
+using ::testing::ContainerEq;
+
 constexpr uint32_t kOpLineOperandLineIndex = 1;
 
 void DoRoundTripCheck(const std::string& text) {
@@ -236,6 +238,100 @@ TEST(IrBuilder, DistributeLineDebugInfo) {
   }
 }
 
+TEST(IrBuilder, BuildModule_WithoutExtraLines) {
+  const std::string text = R"(OpCapability Shader
+OpMemoryModel Logical Simple
+OpEntryPoint Vertex %main "main"
+%file = OpString "my file"
+%void = OpTypeVoid
+%voidfn = OpTypeFunction %void
+%float = OpTypeFloat 32
+%float_1 = OpConstant %float 1
+%main = OpFunction %void None %voidfn
+%100 = OpLabel
+%1 = OpFAdd %float %float_1 %float_1
+OpLine %file 1 0
+%2 = OpFMul %float %1 %1
+%3 = OpFSub %float %2 %2
+OpReturn
+OpFunctionEnd
+)";
+
+  std::vector<uint32_t> binary;
+  SpirvTools t(SPV_ENV_UNIVERSAL_1_1);
+  ASSERT_TRUE(t.Assemble(text, &binary,
+                         SPV_TEXT_TO_BINARY_OPTION_PRESERVE_NUMERIC_IDS));
+
+  // This is the function we're testing.
+  std::unique_ptr<IRContext> context = BuildModule(
+      SPV_ENV_UNIVERSAL_1_5, nullptr, binary.data(), binary.size(), false);
+  ASSERT_NE(nullptr, context);
+
+  spvtools::opt::analysis::DefUseManager* def_use_mgr =
+      context->get_def_use_mgr();
+
+  std::vector<SpvOp> opcodes;
+  for (auto* inst = def_use_mgr->GetDef(1);
+       inst && (inst->opcode() != SpvOpFunctionEnd); inst = inst->NextNode()) {
+    inst->ForEachInst(
+        [&opcodes](spvtools::opt::Instruction* sub_inst) {
+          opcodes.push_back(sub_inst->opcode());
+        },
+        true);
+  }
+
+  EXPECT_THAT(opcodes,
+              ContainerEq(std::vector<SpvOp>{SpvOpFAdd, SpvOpLine, SpvOpFMul,
+                                             SpvOpFSub, SpvOpReturn}));
+}
+
+TEST(IrBuilder, BuildModule_WithExtraLines_IsDefault) {
+  const std::string text = R"(OpCapability Shader
+OpMemoryModel Logical Simple
+OpEntryPoint Vertex %main "main"
+%file = OpString "my file"
+%void = OpTypeVoid
+%voidfn = OpTypeFunction %void
+%float = OpTypeFloat 32
+%float_1 = OpConstant %float 1
+%main = OpFunction %void None %voidfn
+%100 = OpLabel
+%1 = OpFAdd %float %float_1 %float_1
+OpLine %file 1 0
+%2 = OpFMul %float %1 %1
+%3 = OpFSub %float %2 %2
+OpReturn
+OpFunctionEnd
+)";
+
+  std::vector<uint32_t> binary;
+
+  SpirvTools t(SPV_ENV_UNIVERSAL_1_1);
+  ASSERT_TRUE(t.Assemble(text, &binary,
+                         SPV_TEXT_TO_BINARY_OPTION_PRESERVE_NUMERIC_IDS));
+
+  // This is the function we're testing.
+  std::unique_ptr<IRContext> context =
+      BuildModule(SPV_ENV_UNIVERSAL_1_5, nullptr, binary.data(), binary.size());
+
+  spvtools::opt::analysis::DefUseManager* def_use_mgr =
+      context->get_def_use_mgr();
+
+  std::vector<SpvOp> opcodes;
+  for (auto* inst = def_use_mgr->GetDef(1);
+       inst && (inst->opcode() != SpvOpFunctionEnd); inst = inst->NextNode()) {
+    inst->ForEachInst(
+        [&opcodes](spvtools::opt::Instruction* sub_inst) {
+          opcodes.push_back(sub_inst->opcode());
+        },
+        true);
+  }
+
+  EXPECT_THAT(opcodes, ContainerEq(std::vector<SpvOp>{
+                           SpvOpFAdd, SpvOpLine, SpvOpFMul, SpvOpLine,
+                           SpvOpFSub, SpvOpLine, SpvOpReturn}));
+}
+
 TEST(IrBuilder, ConsumeDebugInfoInst) {
   // /* HLSL */
   //