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.

source/opt/debug_info_manager.cpp

@@ -372,7 +372,7 @@ Instruction* DebugInfoManager::CloneDebugInlinedAt(uint32_t clone_inlined_at_id,
       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 @@ uint32_t DebugInfoManager::GetVariableIdOfDebugValueUsedForDeclare(
   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;
 

source/opt/debug_info_manager.h

@@ -133,10 +133,12 @@ class DebugInfoManager {
   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 @@ class DebugInfoManager {
   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_; }
 

source/opt/instruction.h

@@ -246,6 +246,11 @@ class Instruction : public utils::IntrusiveNodeBase<Instruction> {
   // 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);

source/opt/local_single_block_elim_pass.cpp

@@ -83,7 +83,8 @@ bool LocalSingleBlockLoadStoreElimPass::LocalSingleBlockLoadStoreElim(
             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;
             }

source/opt/local_single_store_elim_pass.cpp

@@ -142,7 +142,7 @@ bool LocalSingleStoreElimPass::ProcessVariable(Instruction* var_inst) {
   // 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();

source/opt/loop_unroller.cpp

@@ -286,6 +286,9 @@ class LoopUnrollerUtilsImpl {
   // 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 @@ void LoopUnrollerUtilsImpl::FullyUnroll(Loop* loop) {
       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 @@ void LoopUnrollerUtilsImpl::CopyBasicBlock(Loop* loop, const BasicBlock* itr,
   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 @@ void LoopUnrollerUtilsImpl::FoldConditionBlock(BasicBlock* condition_block,
   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) {

test/opt/debug_info_manager_test.cpp

@@ -484,7 +484,7 @@ void main(float in_var_color : COLOR) {
   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 @@ void main(float in_var_color : COLOR) {
   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

test/opt/loop_optimizations/unroll_simple.cpp

@@ -194,6 +194,190 @@ OpFunctionEnd
   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: