Clear debug information for kill and replacement (#3459)

For many spirv-opt passes such as simplify-instructions pass, we have to correctly clear the OpenCL.DebugInfo.100 debug information for KillInst() and ReplaceAllUses(). If we keep some debug information that disappeared because of KillInst() and ReplaceAllUses(), adding new DebugValue instructions based on the existing DebugDeclare information will generate incorrect information. This CL update DebugInfoManager and IRContext to correctly clear debug information.
2024-10-18 11:10:05 +00:00 · 2020-06-25 15:48:26 -04:00 · 2020-06-25 15:48:26 -04:00 · efaae24d00
commit efaae24d00
parent a1fb255a2a
5 changed files with 210 additions and 33 deletions
--- a/source/opt/debug_info_manager.cpp
+++ b/source/opt/debug_info_manager.cpp
@ -111,7 +111,7 @@ void DebugInfoManager::RegisterDbgDeclare(uint32_t var_id,
  if (dbg_decl_itr == var_id_to_dbg_decl_.end()) {
    var_id_to_dbg_decl_[var_id] = {dbg_declare};
  } else {
-    dbg_decl_itr->second.push_back(dbg_declare);
+    dbg_decl_itr->second.insert(dbg_declare);
  }
 }

@ -544,10 +544,14 @@ void DebugInfoManager::ClearDebugInfo(Instruction* instr) {
    fn_id_to_dbg_fn_.erase(fn_id);
  }

-  if (instr->GetOpenCL100DebugOpcode() == OpenCLDebugInfo100DebugDeclare) {
-    auto var_id =
+  if (instr->GetOpenCL100DebugOpcode() == OpenCLDebugInfo100DebugDeclare ||
+      instr->GetOpenCL100DebugOpcode() == OpenCLDebugInfo100DebugValue) {
+    auto var_or_value_id =
        instr->GetSingleWordOperand(kDebugDeclareOperandVariableIndex);
-    var_id_to_dbg_decl_.erase(var_id);
+    auto dbg_decl_itr = var_id_to_dbg_decl_.find(var_or_value_id);
+    if (dbg_decl_itr != var_id_to_dbg_decl_.end()) {
+      dbg_decl_itr->second.erase(instr);
+    }
  }

  if (debug_info_none_inst_ == instr) {
@ -555,8 +559,9 @@ void DebugInfoManager::ClearDebugInfo(Instruction* instr) {
    for (auto dbg_instr_itr = context()->module()->ext_inst_debuginfo_begin();
         dbg_instr_itr != context()->module()->ext_inst_debuginfo_end();
         ++dbg_instr_itr) {
-      if (dbg_instr_itr->GetOpenCL100DebugOpcode() ==
-          OpenCLDebugInfo100DebugInfoNone) {
+      if (instr != &*dbg_instr_itr &&
+          dbg_instr_itr->GetOpenCL100DebugOpcode() ==
+              OpenCLDebugInfo100DebugInfoNone) {
        debug_info_none_inst_ = &*dbg_instr_itr;
      }
    }
@ -567,7 +572,7 @@ void DebugInfoManager::ClearDebugInfo(Instruction* instr) {
    for (auto dbg_instr_itr = context()->module()->ext_inst_debuginfo_begin();
         dbg_instr_itr != context()->module()->ext_inst_debuginfo_end();
         ++dbg_instr_itr) {
-      if (IsEmptyDebugExpression(&*dbg_instr_itr)) {
+      if (instr != &*dbg_instr_itr && IsEmptyDebugExpression(&*dbg_instr_itr)) {
        empty_debug_expr_inst_ = &*dbg_instr_itr;
      }
    }
--- a/source/opt/debug_info_manager.h
+++ b/source/opt/debug_info_manager.h
@ -16,7 +16,7 @@
 #define SOURCE_OPT_DEBUG_INFO_MANAGER_H_

 #include <unordered_map>
-#include <vector>
+#include <unordered_set>

 #include "source/opt/instruction.h"
 #include "source/opt/module.h"
@ -157,8 +157,9 @@ class DebugInfoManager {
  // in |inst| must not already be registered.
  void RegisterDbgFunction(Instruction* inst);

-  // Register the DebugDeclare instruction |dbg_declare| into
-  // |var_id_to_dbg_decl_| using OpVariable id |var_id| as a key.
+  // Register the DebugDeclare or DebugValue with Deref operation
+  // |dbg_declare| into |var_id_to_dbg_decl_| using OpVariable id
+  // |var_id| as a key.
  void RegisterDbgDeclare(uint32_t var_id, Instruction* dbg_declare);

  // Returns a DebugExpression instruction without Operation operands.
@ -191,9 +192,10 @@ class DebugInfoManager {
  // operand is the function.
  std::unordered_map<uint32_t, Instruction*> fn_id_to_dbg_fn_;

-  // Mapping from local variable ids to DebugDeclare instructions whose
-  // operand is the local variable.
-  std::unordered_map<uint32_t, std::vector<Instruction*>> var_id_to_dbg_decl_;
+  // Mapping from variable or value ids to DebugDeclare or DebugValue
+  // instructions whose operand is the variable or value.
+  std::unordered_map<uint32_t, std::unordered_set<Instruction*>>
+      var_id_to_dbg_decl_;

  // DebugInfoNone instruction. We need only a single DebugInfoNone.
  // To reuse the existing one, we keep it using this member variable.
--- a/source/opt/ir_context.cpp
+++ b/source/opt/ir_context.cpp
@ -355,6 +355,9 @@ void IRContext::ForgetUses(Instruction* inst) {
      get_decoration_mgr()->RemoveDecoration(inst);
    }
  }
+  if (AreAnalysesValid(kAnalysisDebugInfo)) {
+    get_debug_info_mgr()->ClearDebugInfo(inst);
+  }
  RemoveFromIdToName(inst);
 }

@ -367,6 +370,9 @@ void IRContext::AnalyzeUses(Instruction* inst) {
      get_decoration_mgr()->AddDecoration(inst);
    }
  }
+  if (AreAnalysesValid(kAnalysisDebugInfo)) {
+    get_debug_info_mgr()->AnalyzeDebugInst(inst);
+  }
  if (id_to_name_ &&
      (inst->opcode() == SpvOpName || inst->opcode() == SpvOpMemberName)) {
    id_to_name_->insert({inst->GetSingleWordInOperand(0), inst});
@ -424,10 +430,6 @@ void IRContext::KillOperandFromDebugInstructions(Instruction* inst) {
      }
    }
  }
-  // Notice that we do not need anythings to do for local variables.
-  // DebugLocalVariable does not have an OpVariable operand. Instead,
-  // DebugDeclare/DebugValue has an OpVariable operand for a local
-  // variable. The function inlining pass handles it properly.
 }

 void IRContext::AddCombinatorsForCapability(uint32_t capability) {
--- a/test/opt/ir_context_test.cpp
+++ b/test/opt/ir_context_test.cpp
@ -870,7 +870,7 @@ TEST_F(IRContextTest, AsanErrorTest) {
      << bb->id();  // Make sure asan does not complain about use after free.
 }

-TEST_F(IRContextTest, DebugInstructionReplaceAllUses) {
+TEST_F(IRContextTest, DebugInstructionReplaceSingleUse) {
  const std::string text = R"(
 OpCapability Shader
 OpCapability Linkage
@ -911,8 +911,10 @@ OpFunctionEnd)";
  std::unique_ptr<IRContext> ctx =
      BuildModule(SPV_ENV_UNIVERSAL_1_1, nullptr, text,
                  SPV_TEXT_TO_BINARY_OPTION_PRESERVE_NUMERIC_IDS);
+  ctx->BuildInvalidAnalyses(IRContext::kAnalysisDebugInfo);
  DummyPassPreservesAll pass(Pass::Status::SuccessWithChange);
  pass.Run(ctx.get());
+  EXPECT_TRUE(ctx->AreAnalysesValid(IRContext::kAnalysisDebugInfo));

  auto* dbg_value = ctx->get_def_use_mgr()->GetDef(24);
  EXPECT_TRUE(dbg_value->GetSingleWordOperand(kDebugValueOperandValueIndex) ==
@ -931,6 +933,172 @@ OpFunctionEnd)";
      dbg_decl->GetSingleWordOperand(kDebugDeclareOperandVariableIndex) == 20);
 }

+TEST_F(IRContextTest, DebugInstructionReplaceAllUses) {
+  const std::string text = R"(
+OpCapability Shader
+OpCapability Linkage
+%1 = OpExtInstImport "OpenCL.DebugInfo.100"
+OpMemoryModel Logical GLSL450
+%2 = OpString "test"
+%3 = OpTypeVoid
+%4 = OpTypeFunction %3
+%5 = OpTypeFloat 32
+%6 = OpTypePointer Function %5
+%7 = OpConstant %5 0
+%8 = OpTypeInt 32 0
+%9 = OpConstant %8 32
+%10 = OpExtInst %3 %1 DebugExpression
+%11 = OpExtInst %3 %1 DebugSource %2
+%12 = OpExtInst %3 %1 DebugCompilationUnit 1 4 %11 HLSL
+%13 = OpExtInst %3 %1 DebugTypeFunction FlagIsProtected|FlagIsPrivate %3
+%14 = OpExtInst %3 %1 DebugFunction %2 %13 %11 0 0 %12 %2 FlagIsProtected|FlagIsPrivate 0 %17
+%15 = OpExtInst %3 %1 DebugTypeBasic %2 %9 Float
+%16 = OpExtInst %3 %1 DebugLocalVariable %2 %15 %11 0 0 %14 FlagIsLocal
+%27 = OpExtInst %3 %1 DebugLocalVariable %2 %15 %11 1 0 %14 FlagIsLocal
+%17 = OpFunction %3 None %4
+%18 = OpLabel
+%19 = OpExtInst %3 %1 DebugScope %14
+%20 = OpVariable %6 Function
+%26 = OpVariable %6 Function
+OpBranch %21
+%21 = OpLabel
+%22 = OpPhi %5 %7 %18
+OpBranch %23
+%23 = OpLabel
+OpLine %2 0 0
+OpStore %20 %7
+%24 = OpExtInst %3 %1 DebugValue %16 %22 %10
+%25 = OpExtInst %3 %1 DebugDeclare %16 %26 %10
+%28 = OpExtInst %3 %1 DebugValue %27 %22 %10
+%29 = OpExtInst %3 %1 DebugDeclare %27 %26 %10
+OpReturn
+OpFunctionEnd)";
+
+  std::unique_ptr<IRContext> ctx =
+      BuildModule(SPV_ENV_UNIVERSAL_1_1, nullptr, text,
+                  SPV_TEXT_TO_BINARY_OPTION_PRESERVE_NUMERIC_IDS);
+  ctx->BuildInvalidAnalyses(IRContext::kAnalysisDebugInfo);
+  DummyPassPreservesAll pass(Pass::Status::SuccessWithChange);
+  pass.Run(ctx.get());
+  EXPECT_TRUE(ctx->AreAnalysesValid(IRContext::kAnalysisDebugInfo));
+
+  auto* dbg_value0 = ctx->get_def_use_mgr()->GetDef(24);
+  auto* dbg_value1 = ctx->get_def_use_mgr()->GetDef(28);
+  EXPECT_TRUE(dbg_value0->GetSingleWordOperand(kDebugValueOperandValueIndex) ==
+              22);
+  EXPECT_TRUE(dbg_value1->GetSingleWordOperand(kDebugValueOperandValueIndex) ==
+              22);
+  EXPECT_TRUE(ctx->ReplaceAllUsesWith(22, 7));
+  dbg_value0 = ctx->get_def_use_mgr()->GetDef(24);
+  dbg_value1 = ctx->get_def_use_mgr()->GetDef(28);
+  EXPECT_TRUE(dbg_value0->GetSingleWordOperand(kDebugValueOperandValueIndex) ==
+              7);
+  EXPECT_TRUE(dbg_value1->GetSingleWordOperand(kDebugValueOperandValueIndex) ==
+              7);
+
+  auto* dbg_decl0 = ctx->get_def_use_mgr()->GetDef(25);
+  auto* dbg_decl1 = ctx->get_def_use_mgr()->GetDef(29);
+  EXPECT_TRUE(
+      dbg_decl0->GetSingleWordOperand(kDebugDeclareOperandVariableIndex) == 26);
+  EXPECT_TRUE(
+      dbg_decl1->GetSingleWordOperand(kDebugDeclareOperandVariableIndex) == 26);
+  EXPECT_TRUE(ctx->ReplaceAllUsesWith(26, 20));
+  dbg_decl0 = ctx->get_def_use_mgr()->GetDef(25);
+  dbg_decl1 = ctx->get_def_use_mgr()->GetDef(29);
+  EXPECT_TRUE(
+      dbg_decl0->GetSingleWordOperand(kDebugDeclareOperandVariableIndex) == 20);
+  EXPECT_TRUE(
+      dbg_decl1->GetSingleWordOperand(kDebugDeclareOperandVariableIndex) == 20);
+}
+
+TEST_F(IRContextTest, AddDebugValueAfterReplaceUse) {
+  const std::string text = R"(
+OpCapability Shader
+OpCapability Linkage
+%1 = OpExtInstImport "OpenCL.DebugInfo.100"
+OpMemoryModel Logical GLSL450
+%2 = OpString "test"
+%3 = OpTypeVoid
+%4 = OpTypeFunction %3
+%5 = OpTypeFloat 32
+%6 = OpTypePointer Function %5
+%7 = OpConstant %5 0
+%8 = OpTypeInt 32 0
+%9 = OpConstant %8 32
+%10 = OpExtInst %3 %1 DebugExpression
+%11 = OpExtInst %3 %1 DebugSource %2
+%12 = OpExtInst %3 %1 DebugCompilationUnit 1 4 %11 HLSL
+%13 = OpExtInst %3 %1 DebugTypeFunction FlagIsProtected|FlagIsPrivate %3
+%14 = OpExtInst %3 %1 DebugFunction %2 %13 %11 0 0 %12 %2 FlagIsProtected|FlagIsPrivate 0 %17
+%15 = OpExtInst %3 %1 DebugTypeBasic %2 %9 Float
+%16 = OpExtInst %3 %1 DebugLocalVariable %2 %15 %11 0 0 %14 FlagIsLocal
+%17 = OpFunction %3 None %4
+%18 = OpLabel
+%19 = OpExtInst %3 %1 DebugScope %14
+%20 = OpVariable %6 Function
+%26 = OpVariable %6 Function
+OpBranch %21
+%21 = OpLabel
+%27 = OpExtInst %3 %1 DebugScope %14
+%22 = OpPhi %5 %7 %18
+OpBranch %23
+%23 = OpLabel
+%28 = OpExtInst %3 %1 DebugScope %14
+OpLine %2 0 0
+OpStore %20 %7
+%24 = OpExtInst %3 %1 DebugValue %16 %22 %10
+%25 = OpExtInst %3 %1 DebugDeclare %16 %26 %10
+OpReturn
+OpFunctionEnd)";
+
+  std::unique_ptr<IRContext> ctx =
+      BuildModule(SPV_ENV_UNIVERSAL_1_1, nullptr, text,
+                  SPV_TEXT_TO_BINARY_OPTION_PRESERVE_NUMERIC_IDS);
+  ctx->BuildInvalidAnalyses(IRContext::kAnalysisDebugInfo);
+  DummyPassPreservesAll pass(Pass::Status::SuccessWithChange);
+  pass.Run(ctx.get());
+  EXPECT_TRUE(ctx->AreAnalysesValid(IRContext::kAnalysisDebugInfo));
+
+  // Replace all uses of result it '26' with '20'
+  auto* dbg_decl = ctx->get_def_use_mgr()->GetDef(25);
+  EXPECT_EQ(dbg_decl->GetSingleWordOperand(kDebugDeclareOperandVariableIndex),
+            26);
+  EXPECT_TRUE(ctx->ReplaceAllUsesWith(26, 20));
+  dbg_decl = ctx->get_def_use_mgr()->GetDef(25);
+  EXPECT_EQ(dbg_decl->GetSingleWordOperand(kDebugDeclareOperandVariableIndex),
+            20);
+
+  // No DebugValue should be added because result id '26' is not used for
+  // DebugDeclare.
+  ctx->get_debug_info_mgr()->AddDebugValue(dbg_decl, 26, 22, dbg_decl);
+  EXPECT_EQ(dbg_decl->NextNode()->opcode(), SpvOpReturn);
+
+  // DebugValue should be added because result id '20' is used for DebugDeclare.
+  ctx->get_debug_info_mgr()->AddDebugValue(dbg_decl, 20, 22, dbg_decl);
+  EXPECT_EQ(dbg_decl->NextNode()->GetOpenCL100DebugOpcode(),
+            OpenCLDebugInfo100DebugValue);
+
+  // Replace all uses of result it '20' with '26'
+  EXPECT_EQ(dbg_decl->GetSingleWordOperand(kDebugDeclareOperandVariableIndex),
+            20);
+  EXPECT_TRUE(ctx->ReplaceAllUsesWith(20, 26));
+  EXPECT_EQ(dbg_decl->GetSingleWordOperand(kDebugDeclareOperandVariableIndex),
+            26);
+
+  // No DebugValue should be added because result id '20' is not used for
+  // DebugDeclare.
+  ctx->get_debug_info_mgr()->AddDebugValue(dbg_decl, 20, 7, dbg_decl);
+  Instruction* dbg_value = dbg_decl->NextNode();
+  EXPECT_EQ(dbg_value->GetOpenCL100DebugOpcode(), OpenCLDebugInfo100DebugValue);
+  EXPECT_EQ(dbg_value->GetSingleWordOperand(kDebugValueOperandValueIndex), 22);
+
+  // DebugValue should be added because result id '26' is used for DebugDeclare.
+  ctx->get_debug_info_mgr()->AddDebugValue(dbg_decl, 26, 7, dbg_decl);
+  dbg_value = dbg_decl->NextNode();
+  EXPECT_EQ(dbg_value->GetOpenCL100DebugOpcode(), OpenCLDebugInfo100DebugValue);
+  EXPECT_EQ(dbg_value->GetSingleWordOperand(kDebugValueOperandValueIndex), 7);
+}
+
 }  // namespace
 }  // namespace opt
 }  // namespace spvtools
--- a/test/opt/local_ssa_elim_test.cpp
+++ b/test/opt/local_ssa_elim_test.cpp
@ -2190,9 +2190,9 @@ TEST_F(LocalSSAElimTest, DebugValueForReferenceVariable) {
 ; CHECK: [[dbg_x:%\w+]] = OpExtInst %void [[ext]] DebugLocalVariable [[x_name]]

 ; CHECK:      OpStore %f %float_0
-; CHECK-NEXT: OpExtInst %void [[ext]] DebugValue [[dbg_x]] %float_0
-; CHECK-NEXT: OpExtInst %void [[ext]] DebugValue [[dbg_f]] %float_0
-; CHECK-NEXT: OpStore %i %int_0
+; CHECK-DAG:  OpExtInst %void [[ext]] DebugValue [[dbg_f]] %float_0
+; CHECK-DAG:  OpExtInst %void [[ext]] DebugValue [[dbg_x]] %float_0
+; CHECK:      OpStore %i %int_0
 ; CHECK-NEXT: OpExtInst %void [[ext]] DebugValue [[dbg_i]] %int_0

 ; CHECK-NOT:  DebugDeclare
@ -2200,9 +2200,9 @@ TEST_F(LocalSSAElimTest, DebugValueForReferenceVariable) {
 ; CHECK:      [[loop_head:%\w+]] = OpLabel
 ; CHECK:      [[phi0:%\w+]] = OpPhi %float %float_0
 ; CHECK:      [[phi1:%\w+]] = OpPhi %int %int_0
-; CHECK-NEXT: OpExtInst %void [[ext]] DebugValue [[dbg_x]] [[phi0]]
-; CHECK-NEXT: OpExtInst %void [[ext]] DebugValue [[dbg_f]] [[phi0]]
-; CHECK-NEXT: OpExtInst %void [[ext]] DebugValue [[dbg_i]] [[phi1]]
+; CHECK-DAG:  OpExtInst %void [[ext]] DebugValue [[dbg_f]] [[phi0]]
+; CHECK-DAG:  OpExtInst %void [[ext]] DebugValue [[dbg_x]] [[phi0]]
+; CHECK:      OpExtInst %void [[ext]] DebugValue [[dbg_i]] [[phi1]]
 ; CHECK:      OpLoopMerge [[loop_merge:%\w+]] [[loop_cont:%\w+]] None
 ; CHECK-NEXT: OpBranch [[loop_body:%\w+]]

@ -2211,9 +2211,9 @@ TEST_F(LocalSSAElimTest, DebugValueForReferenceVariable) {

 ; CHECK:      [[bb]] = OpLabel
 ; CHECK:      OpStore %f [[f_val:%\w+]]
-; CHECK-NEXT: OpExtInst %void [[ext]] DebugValue [[dbg_x]] [[f_val]]
-; CHECK-NEXT: OpExtInst %void [[ext]] DebugValue [[dbg_f]] [[f_val]]
-; CHECK-NEXT: OpBranch [[loop_cont]]
+; CHECK-DAG:  OpExtInst %void [[ext]] DebugValue [[dbg_f]] [[f_val]]
+; CHECK-DAG:  OpExtInst %void [[ext]] DebugValue [[dbg_x]] [[f_val]]
+; CHECK:      OpBranch [[loop_cont]]

 ; CHECK:      [[loop_cont]] = OpLabel
 ; CHECK:      OpStore %i [[i_val:%\w+]]
@ -2368,16 +2368,16 @@ TEST_F(LocalSSAElimTest, DebugValueForReferenceVariableInBB) {
 ; CHECK:      [[bb]] = OpLabel
 ; CHECK:      OpExtInst %void [[ext]] DebugScope [[dbg_bb]]
 ; CHECK:      OpStore %f [[f_val:%\w+]]
-; CHECK-NEXT: OpExtInst %void [[ext]] DebugValue [[dbg_x]] [[f_val]]
-; CHECK-NEXT: OpExtInst %void [[ext]] DebugValue [[dbg_f]] [[f_val]]
-; CHECK-NEXT: OpBranch [[bb_child:%\w+]]
+; CHECK-DAG:  OpExtInst %void [[ext]] DebugValue [[dbg_f]] [[f_val]]
+; CHECK-DAG:  OpExtInst %void [[ext]] DebugValue [[dbg_x]] [[f_val]]
+; CHECK:      OpBranch [[bb_child:%\w+]]

 ; CHECK:      [[bb_child]] = OpLabel
 ; CHECK:      OpExtInst %void [[ext]] DebugScope [[dbg_bb_child]]
 ; CHECK:      OpStore %f [[new_f_val:%\w+]]
-; CHECK-NEXT: OpExtInst %void [[ext]] DebugValue [[dbg_x]] [[new_f_val]]
-; CHECK-NEXT: OpExtInst %void [[ext]] DebugValue [[dbg_f]] [[new_f_val]]
-; CHECK-NEXT: OpBranch [[loop_cont]]
+; CHECK-DAG:  OpExtInst %void [[ext]] DebugValue [[dbg_f]] [[new_f_val]]
+; CHECK-DAG:  OpExtInst %void [[ext]] DebugValue [[dbg_x]] [[new_f_val]]
+; CHECK:      OpBranch [[loop_cont]]

 ; CHECK:      [[loop_cont]] = OpLabel
 ; CHECK:      OpStore %i [[i_val:%\w+]]