Skip to content

Commit

Permalink
Clear debug information for kill and replacement (#3459)
Browse files Browse the repository at this point in the history 
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.
  • Loading branch information
@jaebaek
jaebaek authored Jun 25, 2020
1 parent a1fb255 commit efaae24
Show file tree
Hide file tree
Showing 5 changed files with 210 additions and 33 deletions.
19 changes: 12 additions & 7 deletions source/opt/debug_info_manager.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
}

Expand Down Expand Up @@ -544,19 +544,24 @@ 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) {
debug_info_none_inst_ = nullptr;
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;
}
}
Expand All @@ -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;
}
}
Expand Down
14 changes: 8 additions & 6 deletions source/opt/debug_info_manager.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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.
Expand Down Expand Up @@ -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.
Expand Down
10 changes: 6 additions & 4 deletions source/opt/ir_context.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}

Expand All @@ -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});
Expand Down Expand Up @@ -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) {
Expand Down
170 changes: 169 additions & 1 deletion test/opt/ir_context_test.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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) ==
Expand All @@ -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
30 changes: 15 additions & 15 deletions test/opt/local_ssa_elim_test.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2190,19 +2190,19 @@ 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
; 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+]]
Expand All @@ -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+]]
Expand Down Expand Up @@ -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+]]
Expand Down

0 comments on commit efaae24

Please sign in to comment.