Debug info preservation in loop-unroll pass (#3548)

When we copy the loop body to unroll it, we have to copy its
instructions but DebugDeclare or DebugValue used for the declaration
i.e., DebugValue with Deref must not be copied and only the first block
can contain those instructions.
diff --git a/source/opt/debug_info_manager.cpp b/source/opt/debug_info_manager.cpp
index e3474c4..b91ef43 100644
--- a/source/opt/debug_info_manager.cpp
+++ b/source/opt/debug_info_manager.cpp
@@ -372,7 +372,7 @@
       std::move(new_inlined_at));
 }
 
-bool DebugInfoManager::IsDebugDeclared(uint32_t variable_id) {
+bool DebugInfoManager::IsVariableDebugDeclared(uint32_t variable_id) {
   auto dbg_decl_itr = var_id_to_dbg_decl_.find(variable_id);
   return dbg_decl_itr != var_id_to_dbg_decl_.end();
 }
@@ -554,6 +554,12 @@
   return 0;
 }
 
+bool DebugInfoManager::IsDebugDeclare(Instruction* instr) {
+  if (!instr->IsOpenCL100DebugInstr()) return false;
+  return instr->GetOpenCL100DebugOpcode() == OpenCLDebugInfo100DebugDeclare ||
+         GetVariableIdOfDebugValueUsedForDeclare(instr) != 0;
+}
+
 void DebugInfoManager::AnalyzeDebugInst(Instruction* dbg_inst) {
   if (!dbg_inst->IsOpenCL100DebugInstr()) return;
 
diff --git a/source/opt/debug_info_manager.h b/source/opt/debug_info_manager.h
index a378b99..cdafc65 100644
--- a/source/opt/debug_info_manager.h
+++ b/source/opt/debug_info_manager.h
@@ -133,10 +133,12 @@
   uint32_t BuildDebugInlinedAtChain(uint32_t callee_inlined_at,
                                     DebugInlinedAtContext* inlined_at_ctx);
 
-  // Return true if |variable_id| has DebugDeclare or DebugVal.
-  bool IsDebugDeclared(uint32_t variable_id);
+  // Returns true if there is a debug declaration instruction whose
+  // 'Local Variable' operand is |variable_id|.
+  bool IsVariableDebugDeclared(uint32_t variable_id);
 
-  // Kill all DebugDeclares for |variable_id|
+  // Kills all debug declaration instructions with Deref whose 'Local Variable'
+  // operand is |variable_id|.
   void KillDebugDeclares(uint32_t variable_id);
 
   // Generates a DebugValue instruction with value |value_id| for every local
@@ -166,6 +168,9 @@
   void ConvertDebugGlobalToLocalVariable(Instruction* dbg_global_var,
                                          Instruction* local_var);
 
+  // Returns true if |instr| is a debug declaration instruction.
+  bool IsDebugDeclare(Instruction* instr);
+
  private:
   IRContext* context() { return context_; }
 
diff --git a/source/opt/instruction.h b/source/opt/instruction.h
index c395f97..509c42f 100644
--- a/source/opt/instruction.h
+++ b/source/opt/instruction.h
@@ -246,6 +246,11 @@
   // Clear line-related debug instructions attached to this instruction.
   void clear_dbg_line_insts() { dbg_line_insts_.clear(); }
 
+  // Set line-related debug instructions.
+  void set_dbg_line_insts(const std::vector<Instruction>& lines) {
+    dbg_line_insts_ = lines;
+  }
+
   // Same semantics as in the base class except the list the InstructionList
   // containing |pos| will now assume ownership of |this|.
   // inline void MoveBefore(Instruction* pos);
diff --git a/source/opt/local_single_block_elim_pass.cpp b/source/opt/local_single_block_elim_pass.cpp
index bd5d751..5111236 100644
--- a/source/opt/local_single_block_elim_pass.cpp
+++ b/source/opt/local_single_block_elim_pass.cpp
@@ -83,7 +83,8 @@
             auto prev_store = var2store_.find(varId);
             if (prev_store != var2store_.end() &&
                 instructions_to_save.count(prev_store->second) == 0 &&
-                !context()->get_debug_info_mgr()->IsDebugDeclared(varId)) {
+                !context()->get_debug_info_mgr()->IsVariableDebugDeclared(
+                    varId)) {
               instructions_to_kill.push_back(prev_store->second);
               modified = true;
             }
diff --git a/source/opt/local_single_store_elim_pass.cpp b/source/opt/local_single_store_elim_pass.cpp
index 63f3341..eb325dd 100644
--- a/source/opt/local_single_store_elim_pass.cpp
+++ b/source/opt/local_single_store_elim_pass.cpp
@@ -142,7 +142,7 @@
   // the DebugDeclare.
   uint32_t var_id = var_inst->result_id();
   if (all_rewritten &&
-      context()->get_debug_info_mgr()->IsDebugDeclared(var_id)) {
+      context()->get_debug_info_mgr()->IsVariableDebugDeclared(var_id)) {
     const analysis::Type* var_type =
         context()->get_type_mgr()->GetType(var_inst->type_id());
     const analysis::Type* store_type = var_type->AsPointer()->pointee_type();
diff --git a/source/opt/loop_unroller.cpp b/source/opt/loop_unroller.cpp
index 9274095..6cdced4 100644
--- a/source/opt/loop_unroller.cpp
+++ b/source/opt/loop_unroller.cpp
@@ -286,6 +286,9 @@
   // to be the actual value of the phi at that point.
   void LinkLastPhisToStart(Loop* loop) const;
 
+  // Kill all debug declaration instructions from |bb|.
+  void KillDebugDeclares(BasicBlock* bb);
+
   // A pointer to the IRContext. Used to add/remove instructions and for usedef
   // chains.
   IRContext* context_;
@@ -598,6 +601,20 @@
       IRContext::Analysis::kAnalysisDefUse);
 }
 
+void LoopUnrollerUtilsImpl::KillDebugDeclares(BasicBlock* bb) {
+  // We cannot kill an instruction inside BasicBlock::ForEachInst()
+  // because it will generate dangling pointers. We use |to_be_killed|
+  // to kill them after the loop.
+  std::vector<Instruction*> to_be_killed;
+
+  bb->ForEachInst([&to_be_killed, this](Instruction* inst) {
+    if (context_->get_debug_info_mgr()->IsDebugDeclare(inst)) {
+      to_be_killed.push_back(inst);
+    }
+  });
+  for (auto* inst : to_be_killed) context_->KillInst(inst);
+}
+
 // Copy a given basic block, give it a new result_id, and store the new block
 // and the id mapping in the state. |preserve_instructions| is used to determine
 // whether or not this function should edit instructions other than the
@@ -608,6 +625,9 @@
   BasicBlock* basic_block = itr->Clone(context_);
   basic_block->SetParent(itr->GetParent());
 
+  // We do not want to duplicate DebugDeclare.
+  KillDebugDeclares(basic_block);
+
   // Assign each result a new unique ID and keep a mapping of the old ids to
   // the new ones.
   AssignNewResultIds(basic_block);
@@ -729,13 +749,19 @@
   Instruction& old_branch = *condition_block->tail();
   uint32_t new_target = old_branch.GetSingleWordOperand(operand_label);
 
+  DebugScope scope = old_branch.GetDebugScope();
+  const std::vector<Instruction> lines = old_branch.dbg_line_insts();
+
   context_->KillInst(&old_branch);
   // Add the new unconditional branch to the merge block.
   InstructionBuilder builder(
       context_, condition_block,
       IRContext::Analysis::kAnalysisDefUse |
           IRContext::Analysis::kAnalysisInstrToBlockMapping);
-  builder.AddBranch(new_target);
+  Instruction* new_branch = builder.AddBranch(new_target);
+
+  new_branch->set_dbg_line_insts(lines);
+  new_branch->SetDebugScope(scope);
 }
 
 void LoopUnrollerUtilsImpl::CloseUnrolledLoop(Loop* loop) {
diff --git a/test/opt/debug_info_manager_test.cpp b/test/opt/debug_info_manager_test.cpp
index 82890f7..b8965b6 100644
--- a/test/opt/debug_info_manager_test.cpp
+++ b/test/opt/debug_info_manager_test.cpp
@@ -484,7 +484,7 @@
   auto* dbg_info_mgr = context->get_debug_info_mgr();
   auto* def_use_mgr = context->get_def_use_mgr();
 
-  EXPECT_TRUE(dbg_info_mgr->IsDebugDeclared(100));
+  EXPECT_TRUE(dbg_info_mgr->IsVariableDebugDeclared(100));
   EXPECT_EQ(def_use_mgr->GetDef(36)->GetOpenCL100DebugOpcode(),
             OpenCLDebugInfo100DebugDeclare);
   EXPECT_EQ(def_use_mgr->GetDef(37)->GetOpenCL100DebugOpcode(),
@@ -496,7 +496,7 @@
   EXPECT_EQ(def_use_mgr->GetDef(36), nullptr);
   EXPECT_EQ(def_use_mgr->GetDef(37), nullptr);
   EXPECT_EQ(def_use_mgr->GetDef(38), nullptr);
-  EXPECT_FALSE(dbg_info_mgr->IsDebugDeclared(100));
+  EXPECT_FALSE(dbg_info_mgr->IsVariableDebugDeclared(100));
 }
 
 }  // namespace
diff --git a/test/opt/loop_optimizations/unroll_simple.cpp b/test/opt/loop_optimizations/unroll_simple.cpp
index 6030135..ca70e19 100644
--- a/test/opt/loop_optimizations/unroll_simple.cpp
+++ b/test/opt/loop_optimizations/unroll_simple.cpp
@@ -194,6 +194,190 @@
   SinglePassRunAndCheck<LoopUnroller>(text, output, false);
 }
 
+/*
+Generated from the following GLSL
+#version 330 core
+layout(location = 0) out vec4 c;
+void main() {
+  float x[4];
+  for (int i = 0; i < 4; ++i) {
+    x[i] = 1.0f;
+  }
+}
+*/
+TEST_F(PassClassTest, SimpleFullyUnrollWithDebugInstructions) {
+  // We must preserve the debug information including OpenCL.DebugInfo.100
+  // instructions and OpLine instructions. Only the first block has
+  // DebugDeclare and DebugValue used for the declaration (i.e., DebugValue
+  // with Deref). Other blocks unrolled from the loop must not contain them.
+  const std::string text = R"(
+OpCapability Shader
+%1 = OpExtInstImport "GLSL.std.450"
+%ext = OpExtInstImport "OpenCL.DebugInfo.100"
+OpMemoryModel Logical GLSL450
+OpEntryPoint Fragment %2 "main" %3
+OpExecutionMode %2 OriginUpperLeft
+OpSource GLSL 330
+%file_name = OpString "test"
+%float_name = OpString "float"
+%main_name = OpString "main"
+%f_name = OpString "f"
+%i_name = OpString "i"
+OpName %2 "main"
+OpName %5 "x"
+OpName %3 "c"
+OpDecorate %3 Location 0
+%6 = OpTypeVoid
+%7 = OpTypeFunction %6
+%8 = OpTypeInt 32 1
+%9 = OpTypePointer Function %8
+%10 = OpConstant %8 0
+%11 = OpConstant %8 4
+%12 = OpTypeBool
+%13 = OpTypeFloat 32
+%14 = OpTypeInt 32 0
+%uint_32 = OpConstant %14 32
+%15 = OpConstant %14 4
+%16 = OpTypeArray %13 %15
+%17 = OpTypePointer Function %16
+%18 = OpConstant %13 1
+%19 = OpTypePointer Function %13
+%20 = OpConstant %8 1
+%21 = OpTypeVector %13 4
+%22 = OpTypePointer Output %21
+%3 = OpVariable %22 Output
+%null_expr = OpExtInst %6 %ext DebugExpression
+%deref = OpExtInst %6 %ext DebugOperation Deref
+%deref_expr = OpExtInst %6 %ext DebugExpression %deref
+%src = OpExtInst %6 %ext DebugSource %file_name
+%cu = OpExtInst %6 %ext DebugCompilationUnit 1 4 %src HLSL
+%dbg_tf = OpExtInst %6 %ext DebugTypeBasic %float_name %uint_32 Float
+%dbg_v4f = OpExtInst %6 %ext DebugTypeVector %dbg_tf 4
+%main_ty = OpExtInst %6 %ext DebugTypeFunction FlagIsProtected|FlagIsPrivate %dbg_v4f %dbg_v4f
+%dbg_main = OpExtInst %6 %ext DebugFunction %main_name %main_ty %src 0 0 %cu %main_name FlagIsProtected|FlagIsPrivate 10 %2
+%bb = OpExtInst %6 %ext DebugLexicalBlock %src 0 0 %dbg_main
+%dbg_f = OpExtInst %6 %ext DebugLocalVariable %f_name %dbg_v4f %src 0 0 %dbg_main FlagIsLocal
+%dbg_i = OpExtInst %6 %ext DebugLocalVariable %i_name %dbg_v4f %src 1 0 %bb FlagIsLocal
+
+; CHECK: [[f:%\w+]] = OpString "f"
+; CHECK: [[i:%\w+]] = OpString "i"
+; CHECK: [[int_0:%\w+]] = OpConstant {{%\w+}} 0
+
+; CHECK: [[null_expr:%\w+]] = OpExtInst {{%\w+}} {{%\w+}} DebugExpression
+; CHECK: [[deref:%\w+]] = OpExtInst {{%\w+}} {{%\w+}} DebugOperation Deref
+; CHECK: [[deref_expr:%\w+]] = OpExtInst {{%\w+}} {{%\w+}} DebugExpression [[deref]]
+; CHECK: [[dbg_fn:%\w+]] = OpExtInst {{%\w+}} {{%\w+}} DebugFunction
+; CHECK: [[dbg_bb:%\w+]] = OpExtInst {{%\w+}} {{%\w+}} DebugLexicalBlock
+; CHECK: [[dbg_f:%\w+]] = OpExtInst {{%\w+}} {{%\w+}} DebugLocalVariable [[f]] {{%\w+}} {{%\w+}} 0 0 [[dbg_fn]]
+; CHECK: [[dbg_i:%\w+]] = OpExtInst {{%\w+}} {{%\w+}} DebugLocalVariable [[i]] {{%\w+}} {{%\w+}} 1 0 [[dbg_bb]]
+
+%2 = OpFunction %6 None %7
+%23 = OpLabel
+
+; The first block has DebugDeclare and DebugValue with Deref
+;
+; CHECK: OpLabel
+; CHECK: DebugScope [[dbg_fn]]
+; CHECK: [[x:%\w+]] = OpVariable {{%\w+}} Function
+; CHECK: OpLine {{%\w+}} 0 0
+; CHECK: OpBranch
+; CHECK: OpLabel
+; CHECK: DebugScope [[dbg_fn]]
+; CHECK: DebugValue [[dbg_f]] [[int_0]] [[null_expr]]
+; CHECK: OpBranch
+; CHECK: DebugScope [[dbg_fn]]
+; CHECK: OpLine {{%\w+}} 1 1
+; CHECK: OpSLessThan
+; CHECK: OpLine {{%\w+}} 2 0
+; CHECK: OpBranch
+; CHECK: OpLabel
+; CHECK: DebugScope [[dbg_bb]]
+; CHECK: DebugDeclare [[dbg_f]] [[x]] [[null_expr]]
+; CHECK: DebugValue [[dbg_i]] [[x]] [[deref_expr]]
+; CHECK: OpLine {{%\w+}} 3 0
+;
+; CHECK: OpLine {{%\w+}} 6 0
+; CHECK: [[add:%\w+]] = OpIAdd
+; CHECK: DebugValue [[dbg_f]] [[add]] [[null_expr]]
+; CHECK: OpLine {{%\w+}} 7 0
+
+; Other blocks do not have DebugDeclare and DebugValue with Deref
+;
+; CHECK: DebugScope [[dbg_fn]]
+; CHECK: OpLine {{%\w+}} 1 1
+; CHECK: OpSLessThan
+; CHECK: OpLine {{%\w+}} 2 0
+; CHECK: OpBranch
+; CHECK: OpLabel
+;
+; CHECK: DebugScope [[dbg_bb]]
+; CHECK-NOT: DebugDeclare [[dbg_f]] [[x]] [[null_expr]]
+; CHECK-NOT: DebugValue [[dbg_i]] [[x]] [[deref_expr]]
+; CHECK: OpLine {{%\w+}} 3 0
+;
+; CHECK: OpLine {{%\w+}} 6 0
+; CHECK: [[add:%\w+]] = OpIAdd
+; CHECK: DebugValue [[dbg_f]] [[add]] [[null_expr]]
+; CHECK: OpLine {{%\w+}} 7 0
+;
+; CHECK-NOT: DebugDeclare [[dbg_f]] [[x]] [[null_expr]]
+; CHECK-NOT: DebugValue [[dbg_i]] [[x]] [[deref_expr]]
+; CHECK: DebugScope [[dbg_fn]]
+; CHECK: OpLine {{%\w+}} 8 0
+; CHECK: OpReturn
+
+%s0 = OpExtInst %6 %ext DebugScope %dbg_main
+%5 = OpVariable %17 Function
+OpLine %file_name 0 0
+OpBranch %24
+%24 = OpLabel
+%s1 = OpExtInst %6 %ext DebugScope %dbg_main
+%35 = OpPhi %8 %10 %23 %34 %26
+%value0 = OpExtInst %6 %ext DebugValue %dbg_f %35 %null_expr
+OpLine %file_name 1 0
+OpLoopMerge %25 %26 Unroll
+OpBranch %27
+%27 = OpLabel
+%s2 = OpExtInst %6 %ext DebugScope %dbg_main
+OpLine %file_name 1 1
+%29 = OpSLessThan %12 %35 %11
+OpLine %file_name 2 0
+OpBranchConditional %29 %30 %25
+%30 = OpLabel
+%s3 = OpExtInst %6 %ext DebugScope %bb
+%decl0 = OpExtInst %6 %ext DebugDeclare %dbg_f %5 %null_expr
+%decl1 = OpExtInst %6 %ext DebugValue %dbg_i %5 %deref_expr
+OpLine %file_name 3 0
+%32 = OpAccessChain %19 %5 %35
+OpLine %file_name 4 0
+OpStore %32 %18
+OpLine %file_name 5 0
+OpBranch %26
+%26 = OpLabel
+%s4 = OpExtInst %6 %ext DebugScope %dbg_main
+OpLine %file_name 6 0
+%34 = OpIAdd %8 %35 %20
+%value1 = OpExtInst %6 %ext DebugValue %dbg_f %34 %null_expr
+OpLine %file_name 7 0
+OpBranch %24
+%25 = OpLabel
+%s5 = OpExtInst %6 %ext DebugScope %dbg_main
+OpLine %file_name 8 0
+OpReturn
+OpFunctionEnd)";
+
+  std::unique_ptr<IRContext> context =
+      BuildModule(SPV_ENV_UNIVERSAL_1_1, nullptr, text,
+                  SPV_TEXT_TO_BINARY_OPTION_PRESERVE_NUMERIC_IDS);
+  Module* module = context->module();
+  EXPECT_NE(nullptr, module) << "Assembling failed for ushader:\n"
+                             << text << std::endl;
+
+  LoopUnroller loop_unroller;
+  SetDisassembleOptions(SPV_BINARY_TO_TEXT_OPTION_NO_HEADER);
+  SinglePassRunAndMatch<LoopUnroller>(text, true);
+}
+
 template <int factor>
 class PartialUnrollerTestPass : public Pass {
  public: