Set correct scope and line info for DebugValue (#4125)
The existing spirv-opt `DebugInfoManager::AddDebugValueForDecl()` sets
the scope and line info of the new added DebugValue using the scope and
line of DebugDeclare. This is wrong because only a single DebugDeclare
must exist under a scope while we have to add DebugValue for all the
places where the variable's value is updated. Therefore, we have to set
the scope and line of DebugValue based on the places of the variable
updates.
This bug makes
https://github.com/google/amber/blob/main/tests/cases/debugger_hlsl_shadowed_vars.amber
fail. This commit fixes the bug.
diff --git a/source/opt/debug_info_manager.cpp b/source/opt/debug_info_manager.cpp
index 11b5131..e782641 100644
--- a/source/opt/debug_info_manager.cpp
+++ b/source/opt/debug_info_manager.cpp
@@ -500,14 +500,15 @@
insert_before->opcode() == SpvOpVariable) {
insert_before = insert_before->NextNode();
}
- modified |= AddDebugValueForDecl(dbg_decl_or_val, value_id,
- insert_before) != nullptr;
+ modified |= AddDebugValueForDecl(dbg_decl_or_val, value_id, insert_before,
+ scope_and_line) != nullptr;
}
return modified;
}
Instruction* DebugInfoManager::AddDebugValueForDecl(
- Instruction* dbg_decl, uint32_t value_id, Instruction* insert_before) {
+ Instruction* dbg_decl, uint32_t value_id, Instruction* insert_before,
+ Instruction* scope_and_line) {
if (dbg_decl == nullptr || !IsDebugDeclare(dbg_decl)) return nullptr;
std::unique_ptr<Instruction> dbg_val(dbg_decl->Clone(context()));
@@ -517,6 +518,7 @@
dbg_val->SetOperand(kDebugDeclareOperandVariableIndex, {value_id});
dbg_val->SetOperand(kDebugValueOperandExpressionIndex,
{GetEmptyDebugExpression()->result_id()});
+ dbg_val->UpdateDebugInfoFrom(scope_and_line);
auto* added_dbg_val = insert_before->InsertBefore(std::move(dbg_val));
AnalyzeDebugInst(added_dbg_val);
diff --git a/source/opt/debug_info_manager.h b/source/opt/debug_info_manager.h
index 92b3818..776e9ba 100644
--- a/source/opt/debug_info_manager.h
+++ b/source/opt/debug_info_manager.h
@@ -152,11 +152,14 @@
std::unordered_set<Instruction*>* invisible_decls);
// Creates a DebugValue for DebugDeclare |dbg_decl| and inserts it before
- // |insert_before|. The new DebugValue has the same line, scope, and
- // operands with DebugDeclare but it uses |value_id| for value. Returns
- // the added DebugValue, or nullptr if it does not add a DebugValue.
+ // |insert_before|. The new DebugValue has the same line and scope as
+ // |scope_and_line|, or no scope and line information if |scope_and_line|
+ // is nullptr. The new DebugValue has the same operands as DebugDeclare
+ // but it uses |value_id| for the value. Returns the created DebugValue,
+ // or nullptr if fails to create one.
Instruction* AddDebugValueForDecl(Instruction* dbg_decl, uint32_t value_id,
- Instruction* insert_before);
+ Instruction* insert_before,
+ Instruction* scope_and_line);
// Erases |instr| from data structures of this class.
void ClearDebugInfo(Instruction* instr);
diff --git a/source/opt/local_single_store_elim_pass.cpp b/source/opt/local_single_store_elim_pass.cpp
index b8d9091..d322a2f 100644
--- a/source/opt/local_single_store_elim_pass.cpp
+++ b/source/opt/local_single_store_elim_pass.cpp
@@ -175,7 +175,7 @@
for (auto* decl : invisible_decls) {
if (dominator_analysis->Dominates(store_inst, decl)) {
context()->get_debug_info_mgr()->AddDebugValueForDecl(decl, value_id,
- decl);
+ decl, store_inst);
modified = true;
}
}
diff --git a/source/opt/scalar_replacement_pass.cpp b/source/opt/scalar_replacement_pass.cpp
index c8e0da5..ba2d067 100644
--- a/source/opt/scalar_replacement_pass.cpp
+++ b/source/opt/scalar_replacement_pass.cpp
@@ -175,7 +175,8 @@
Instruction* added_dbg_value =
context()->get_debug_info_mgr()->AddDebugValueForDecl(
dbg_decl, /*value_id=*/var->result_id(),
- /*insert_before=*/var->NextNode());
+ /*insert_before=*/var->NextNode(), /*scope_and_line=*/dbg_decl);
+
if (added_dbg_value == nullptr) return false;
added_dbg_value->AddOperand(
{SPV_OPERAND_TYPE_ID,
diff --git a/source/opt/ssa_rewrite_pass.cpp b/source/opt/ssa_rewrite_pass.cpp
index 0489f03..81770d7 100644
--- a/source/opt/ssa_rewrite_pass.cpp
+++ b/source/opt/ssa_rewrite_pass.cpp
@@ -658,8 +658,8 @@
// a = 3;
// foo(3);
// After inlining:
- // a = 3; // we want to specify "DebugValue: %x = %int_3"
- // foo and x disappeared!
+ // a = 3;
+ // foo and x disappeared but we want to specify "DebugValue: %x = %int_3".
//
// We want to specify the value for the variable using |defs_at_block_[bb]|,
// where |bb| is the basic block contains the decl.
@@ -681,16 +681,17 @@
if (value && (pass_->context()->get_instr_block(value) == nullptr ||
dom_tree->Dominates(value, decl))) {
if (pass_->context()->get_debug_info_mgr()->AddDebugValueForDecl(
- decl, value->result_id(), decl) == nullptr) {
+ decl, value->result_id(), decl, value) == nullptr) {
return Pass::Status::Failure;
}
} else {
// If |value| in the same basic block does not dominate |decl|, we can
// assign the value in the immediate dominator.
value_id = GetValueAtBlock(var_id, dom_tree->ImmediateDominator(bb));
+ if (value_id) value = pass_->get_def_use_mgr()->GetDef(value_id);
if (value_id &&
pass_->context()->get_debug_info_mgr()->AddDebugValueForDecl(
- decl, value_id, decl) == nullptr) {
+ decl, value_id, decl, value) == nullptr) {
return Pass::Status::Failure;
}
}
diff --git a/test/opt/debug_info_manager_test.cpp b/test/opt/debug_info_manager_test.cpp
index 911331a..49407fd 100644
--- a/test/opt/debug_info_manager_test.cpp
+++ b/test/opt/debug_info_manager_test.cpp
@@ -31,6 +31,9 @@
static const uint32_t kDebugInlinedAtOperandLineIndex = 4;
static const uint32_t kDebugInlinedAtOperandScopeIndex = 5;
static const uint32_t kDebugInlinedAtOperandInlinedIndex = 6;
+static const uint32_t kOpLineInOperandFileIndex = 0;
+static const uint32_t kOpLineInOperandLineIndex = 1;
+static const uint32_t kOpLineInOperandColumnIndex = 2;
namespace spvtools {
namespace opt {
@@ -609,6 +612,80 @@
EXPECT_FALSE(dbg_info_mgr->IsVariableDebugDeclared(100));
}
+TEST(DebugInfoManager, AddDebugValueForDecl) {
+ const std::string text = R"(
+ OpCapability Shader
+ %1 = OpExtInstImport "OpenCL.DebugInfo.100"
+ OpMemoryModel Logical GLSL450
+ OpEntryPoint Fragment %main "main" %in_var_COLOR
+ OpExecutionMode %main OriginUpperLeft
+ %5 = OpString "ps.hlsl"
+ %14 = OpString "#line 1 \"ps.hlsl\"
+void main(float in_var_color : COLOR) {
+ float color = in_var_color;
+}
+"
+ %17 = OpString "float"
+ %21 = OpString "main"
+ %24 = OpString "color"
+ OpName %in_var_COLOR "in.var.COLOR"
+ OpName %main "main"
+ OpDecorate %in_var_COLOR Location 0
+ %uint = OpTypeInt 32 0
+ %uint_32 = OpConstant %uint 32
+ %float = OpTypeFloat 32
+%_ptr_Input_float = OpTypePointer Input %float
+ %void = OpTypeVoid
+ %27 = OpTypeFunction %void
+%_ptr_Function_float = OpTypePointer Function %float
+%in_var_COLOR = OpVariable %_ptr_Input_float Input
+ %13 = OpExtInst %void %1 DebugExpression
+ %15 = OpExtInst %void %1 DebugSource %5 %14
+ %16 = OpExtInst %void %1 DebugCompilationUnit 1 4 %15 HLSL
+ %18 = OpExtInst %void %1 DebugTypeBasic %17 %uint_32 Float
+ %20 = OpExtInst %void %1 DebugTypeFunction FlagIsProtected|FlagIsPrivate %18 %18
+ %22 = OpExtInst %void %1 DebugFunction %21 %20 %15 1 1 %16 %21 FlagIsProtected|FlagIsPrivate 1 %main
+ %12 = OpExtInst %void %1 DebugInfoNone
+ %25 = OpExtInst %void %1 DebugLocalVariable %24 %18 %15 1 20 %22 FlagIsLocal 0
+ %main = OpFunction %void None %27
+ %28 = OpLabel
+ %100 = OpVariable %_ptr_Function_float Function
+ %31 = OpLoad %float %in_var_COLOR
+ %101 = OpExtInst %void %1 DebugScope %22
+ OpLine %5 13 7
+ OpStore %100 %31
+ OpNoLine
+ %102 = OpExtInst %void %1 DebugNoScope
+ %36 = OpExtInst %void %1 DebugDeclare %25 %100 %13
+ OpReturn
+ OpFunctionEnd
+ )";
+
+ std::unique_ptr<IRContext> context =
+ BuildModule(SPV_ENV_UNIVERSAL_1_1, nullptr, text,
+ SPV_TEXT_TO_BINARY_OPTION_PRESERVE_NUMERIC_IDS);
+ auto* def_use_mgr = context->get_def_use_mgr();
+ auto* dbg_decl = def_use_mgr->GetDef(36);
+ EXPECT_EQ(dbg_decl->GetOpenCL100DebugOpcode(),
+ OpenCLDebugInfo100DebugDeclare);
+
+ auto* dbg_info_mgr = context->get_debug_info_mgr();
+ Instruction* store = dbg_decl->PreviousNode();
+ auto* dbg_value =
+ dbg_info_mgr->AddDebugValueForDecl(dbg_decl, 100, dbg_decl, store);
+
+ EXPECT_EQ(dbg_value->GetOpenCL100DebugOpcode(), OpenCLDebugInfo100DebugValue);
+ EXPECT_EQ(dbg_value->dbg_line_inst()->GetSingleWordInOperand(
+ kOpLineInOperandFileIndex),
+ 5);
+ EXPECT_EQ(dbg_value->dbg_line_inst()->GetSingleWordInOperand(
+ kOpLineInOperandLineIndex),
+ 13);
+ EXPECT_EQ(dbg_value->dbg_line_inst()->GetSingleWordInOperand(
+ kOpLineInOperandColumnIndex),
+ 7);
+}
+
} // namespace
} // namespace analysis
} // namespace opt
diff --git a/test/opt/local_single_store_elim_test.cpp b/test/opt/local_single_store_elim_test.cpp
index d015dfb..c94ff37 100644
--- a/test/opt/local_single_store_elim_test.cpp
+++ b/test/opt/local_single_store_elim_test.cpp
@@ -1403,18 +1403,20 @@
%param_var_pos = OpVariable %_ptr_Function_v4float Function
%param_var_color = OpVariable %_ptr_Function_v4float Function
%55 = OpLoad %v4float %in_var_POSITION
+ OpLine %7 6 23
OpStore %param_var_pos %55
+ OpNoLine
%56 = OpLoad %v4float %in_var_COLOR
;CHECK: DebugNoScope
;CHECK-NOT: OpLine
;CHECK: [[pos:%\w+]] = OpLoad %v4float %in_var_POSITION
;CHECK: [[color:%\w+]] = OpLoad %v4float %in_var_COLOR
- OpStore %param_var_color %56
- %93 = OpExtInst %void %1 DebugScope %48
- OpLine %7 6 23
- %73 = OpExtInst %void %1 DebugDeclare %53 %param_var_pos %52
OpLine %7 7 23
+ OpStore %param_var_color %56
+ OpNoLine
+ %93 = OpExtInst %void %1 DebugScope %48
+ %73 = OpExtInst %void %1 DebugDeclare %53 %param_var_pos %52
%74 = OpExtInst %void %1 DebugDeclare %51 %param_var_color %52
;CHECK: OpLine [[file:%\w+]] 6 23
;CHECK-NEXT: {{%\w+}} = OpExtInst %void {{%\w+}} DebugValue [[dbg_pos]] [[pos]] [[empty_expr:%\w+]]
diff --git a/test/opt/local_ssa_elim_test.cpp b/test/opt/local_ssa_elim_test.cpp
index 9baf8da..ca9aba3 100644
--- a/test/opt/local_ssa_elim_test.cpp
+++ b/test/opt/local_ssa_elim_test.cpp
@@ -2261,9 +2261,13 @@
%69 = OpExtInst %void %1 DebugLexicalBlock %60 7 21 %68
%70 = OpExtInst %void %1 DebugLocalVariable %11 %59 %60 6 26 %67 FlagIsLocal 2
+; CHECK: [[color_name:%\w+]] = OpString "color"
+; CHECK: [[pos_name:%\w+]] = OpString "pos"
; CHECK: [[i_name:%\w+]] = OpString "i"
; CHECK: [[null_expr:%\w+]] = OpExtInst %void [[ext:%\w+]] DebugExpression
; CHECK: [[dbg_i:%\w+]] = OpExtInst %void [[ext]] DebugLocalVariable [[i_name]] {{%\w+}} {{%\w+}} 6 16 {{%\w+}} FlagIsLocal 1
+; CHECK: [[dbg_color:%\w+]] = OpExtInst %void [[ext]] DebugLocalVariable [[color_name]] {{%\w+}} {{%\w+}} 15 23
+; CHECK: [[dbg_pos:%\w+]] = OpExtInst %void [[ext]] DebugLocalVariable [[pos_name]] {{%\w+}} {{%\w+}} 14 23
%71 = OpExtInst %void %1 DebugLocalVariable %15 %65 %60 6 16 %67 FlagIsLocal 1
%72 = OpExtInst %void %1 DebugTypeFunction FlagIsProtected|FlagIsPrivate %62 %59 %59
%73 = OpExtInst %void %1 DebugFunction %16 %72 %60 14 1 %61 %14 FlagIsProtected|FlagIsPrivate 15 %156
@@ -2282,13 +2286,23 @@
%169 = OpExtInst %void %1 DebugNoScope
%param_var_pos = OpVariable %_ptr_Function_v4float Function
%param_var_color = OpVariable %_ptr_Function_v4float Function
+ OpLine %7 100 105
%80 = OpLoad %v4float %in_var_POSITION
OpStore %param_var_pos %80
+ OpNoLine
+ OpLine %7 200 205
%81 = OpLoad %v4float %in_var_COLOR
OpStore %param_var_color %81
+ OpNoLine
%170 = OpExtInst %void %1 DebugScope %73
+
+; CHECK: OpLine {{%\w+}} 100 105
+; CHECK: DebugValue [[dbg_pos]]
%124 = OpExtInst %void %1 DebugDeclare %78 %param_var_pos %77
+; CHECK: OpLine {{%\w+}} 200 205
+; CHECK: DebugValue [[dbg_color]]
%125 = OpExtInst %void %1 DebugDeclare %76 %param_var_color %77
+
%171 = OpExtInst %void %1 DebugScope %74
OpLine %7 17 18
