Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add DebugValue for invisible store in single_store_elim #4002

Merged
merged 3 commits into from
Nov 4, 2020
Merged

Add DebugValue for invisible store in single_store_elim #4002

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
84260fa
Add DebugValue for invisible store in single_store_elim
jaebaek Oct 28, 2020
eb58fd2
Code review
jaebaek Oct 30, 2020
026fb29
Code review
jaebaek Nov 4, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 13 additions & 7 deletions source/opt/debug_info_manager.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -384,7 +384,8 @@ bool DebugInfoManager::IsVariableDebugDeclared(uint32_t variable_id) {
return dbg_decl_itr != var_id_to_dbg_decl_.end();
}

void DebugInfoManager::KillDebugDeclares(uint32_t variable_id) {
bool DebugInfoManager::KillDebugDeclares(uint32_t variable_id) {
bool modified = false;
auto dbg_decl_itr = var_id_to_dbg_decl_.find(variable_id);
if (dbg_decl_itr != var_id_to_dbg_decl_.end()) {
// We intentionally copy the list of DebugDeclare instructions because
Expand All @@ -394,9 +395,11 @@ void DebugInfoManager::KillDebugDeclares(uint32_t variable_id) {

for (auto* dbg_decl : copy_dbg_decls) {
context()->KillInst(dbg_decl);
modified = true;
}
var_id_to_dbg_decl_.erase(dbg_decl_itr);
}
return modified;
}

uint32_t DebugInfoManager::GetParentScope(uint32_t child_scope) {
Expand Down Expand Up @@ -514,16 +517,18 @@ Instruction* DebugInfoManager::AddDebugValueWithIndex(
return added_dbg_value;
}

void DebugInfoManager::AddDebugValueIfVarDeclIsVisible(
bool DebugInfoManager::AddDebugValueIfVarDeclIsVisible(
Instruction* scope_and_line, uint32_t variable_id, uint32_t value_id,
Instruction* insert_pos,
std::unordered_set<Instruction*>* invisible_decls) {
assert(scope_and_line != nullptr);

auto dbg_decl_itr = var_id_to_dbg_decl_.find(variable_id);
if (dbg_decl_itr == var_id_to_dbg_decl_.end()) return;
if (dbg_decl_itr == var_id_to_dbg_decl_.end()) return false;

bool modified = false;
for (auto* dbg_decl_or_val : dbg_decl_itr->second) {
if (scope_and_line &&
!IsDeclareVisibleToInstr(dbg_decl_or_val, scope_and_line)) {
if (!IsDeclareVisibleToInstr(dbg_decl_or_val, scope_and_line)) {
dnovillo marked this conversation as resolved.
Show resolved Hide resolved
if (invisible_decls) invisible_decls->insert(dbg_decl_or_val);
continue;
}
Expand All @@ -547,10 +552,11 @@ void DebugInfoManager::AddDebugValueIfVarDeclIsVisible(
kDebugValueOperandLocalVariableIndex),
value_id, 0, index_id, insert_before);
assert(added_dbg_value != nullptr);
added_dbg_value->UpdateDebugInfoFrom(scope_and_line ? scope_and_line
: dbg_decl_or_val);
added_dbg_value->UpdateDebugInfoFrom(scope_and_line);
AnalyzeDebugInst(added_dbg_value);
modified = true;
}
return modified;
}

bool DebugInfoManager::AddDebugValueForDecl(Instruction* dbg_decl,
Expand Down
9 changes: 5 additions & 4 deletions source/opt/debug_info_manager.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -138,14 +138,15 @@ class DebugInfoManager {
bool IsVariableDebugDeclared(uint32_t variable_id);

// Kills all debug declaration instructions with Deref whose 'Local Variable'
// operand is |variable_id|.
void KillDebugDeclares(uint32_t variable_id);
// operand is |variable_id|. Returns whether it kills an instruction or not.
bool KillDebugDeclares(uint32_t variable_id);

// Generates a DebugValue instruction with value |value_id| for every local
// variable that is in the scope of |scope_and_line| and whose memory is
// |variable_id| and inserts it after the instruction |insert_pos|.
// |invisible_decls| returns DebugDeclares invisible to |scope_and_line|.
void AddDebugValueIfVarDeclIsVisible(
// Returns whether a DebugValue is added or not. |invisible_decls| returns
// DebugDeclares invisible to |scope_and_line|.
bool AddDebugValueIfVarDeclIsVisible(
Instruction* scope_and_line, uint32_t variable_id, uint32_t value_id,
Instruction* insert_pos,
std::unordered_set<Instruction*>* invisible_decls);
Expand Down
33 changes: 29 additions & 4 deletions source/opt/local_single_store_elim_pass.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -148,16 +148,41 @@ bool LocalSingleStoreElimPass::ProcessVariable(Instruction* var_inst) {
context()->get_type_mgr()->GetType(var_inst->type_id());
const analysis::Type* store_type = var_type->AsPointer()->pointee_type();
if (!(store_type->AsStruct() || store_type->AsArray())) {
context()->get_debug_info_mgr()->AddDebugValueIfVarDeclIsVisible(
nullptr, var_id, store_inst->GetSingleWordInOperand(1), store_inst,
nullptr);
context()->get_debug_info_mgr()->KillDebugDeclares(var_id);
modified |= RewriteDebugDeclares(store_inst, var_id);
}
}

return modified;
}

bool LocalSingleStoreElimPass::RewriteDebugDeclares(Instruction* store_inst,
uint32_t var_id) {
std::unordered_set<Instruction*> invisible_decls;
uint32_t value_id = store_inst->GetSingleWordInOperand(1);
bool modified =
context()->get_debug_info_mgr()->AddDebugValueIfVarDeclIsVisible(
store_inst, var_id, value_id, store_inst, &invisible_decls);

// For cases like the argument passing for an inlined function, the value
// assignment is out of DebugDeclare's scope, but we have to preserve the
// value assignment information using DebugValue. Generally, we need
// ssa-rewrite analysis to decide a proper value assignment but at this point
// we confirm that |var_id| has a single store. We can safely add DebugValue.
if (!invisible_decls.empty()) {
BasicBlock* store_block = context()->get_instr_block(store_inst);
DominatorAnalysis* dominator_analysis =
context()->GetDominatorAnalysis(store_block->GetParent());
for (auto* decl : invisible_decls) {
if (dominator_analysis->Dominates(store_inst, decl)) {
context()->get_debug_info_mgr()->AddDebugValueForDecl(decl, value_id);
modified = true;
}
dnovillo marked this conversation as resolved.
Show resolved Hide resolved
}
}
modified |= context()->get_debug_info_mgr()->KillDebugDeclares(var_id);
dnovillo marked this conversation as resolved.
Show resolved Hide resolved
return modified;
}

Instruction* LocalSingleStoreElimPass::FindSingleStoreAndCheckUses(
Instruction* var_inst, const std::vector<Instruction*>& users) const {
// Make sure there is exactly 1 store.
Expand Down
4 changes: 4 additions & 0 deletions source/opt/local_single_store_elim_pass.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -94,6 +94,10 @@ class LocalSingleStoreElimPass : public Pass {
bool RewriteLoads(Instruction* store_inst,
const std::vector<Instruction*>& uses, bool* all_rewritten);

// Replaces DebugDeclares of |var_id| with DebugValues using the value
// assignment of |store_inst|.
bool RewriteDebugDeclares(Instruction* store_inst, uint32_t var_id);

// Extensions supported by this pass.
std::unordered_set<std::string> extensions_allowlist_;
};
Expand Down
231 changes: 231 additions & 0 deletions test/opt/local_single_store_elim_test.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1211,6 +1211,237 @@ TEST_F(LocalSingleStoreElimTest, DebugValueTest) {
SinglePassRunAndMatch<LocalSingleStoreElimPass>(text, false);
}

TEST_F(LocalSingleStoreElimTest, UseStoreLineInfoForDebugValueLine) {
// When the store is in the scope of OpenCL.DebugInfo.100 DebugDeclare,
// the OpLine of the added OpenCL.DebugInfo.100 DebugValue must be the
// same with the OpLine of the store.
const std::string text = R"(
OpCapability Shader
%1 = OpExtInstImport "OpenCL.DebugInfo.100"
OpMemoryModel Logical GLSL450
OpEntryPoint Vertex %main "main" %in_var_POSITION %in_var_COLOR %gl_Position %out_var_COLOR
%7 = OpString "simple.hlsl"
%8 = OpString "float"
%9 = OpString "VS_OUTPUT"
%10 = OpString "color"
%11 = OpString "pos"
%12 = OpString "main"
%13 = OpString ""
%14 = OpString "vout"
OpName %in_var_POSITION "in.var.POSITION"
OpName %in_var_COLOR "in.var.COLOR"
OpName %out_var_COLOR "out.var.COLOR"
OpName %main "main"
OpName %VS_OUTPUT "VS_OUTPUT"
OpMemberName %VS_OUTPUT 0 "pos"
OpMemberName %VS_OUTPUT 1 "color"
OpDecorate %gl_Position BuiltIn Position
OpDecorate %in_var_POSITION Location 0
OpDecorate %in_var_COLOR Location 1
OpDecorate %out_var_COLOR Location 0
%int = OpTypeInt 32 1
%int_0 = OpConstant %int 0
%int_1 = OpConstant %int 1
%uint = OpTypeInt 32 0
%uint_32 = OpConstant %uint 32
%float = OpTypeFloat 32
%v4float = OpTypeVector %float 4
%_ptr_Input_v4float = OpTypePointer Input %v4float
%_ptr_Output_v4float = OpTypePointer Output %v4float
%void = OpTypeVoid
%uint_256 = OpConstant %uint 256
%uint_128 = OpConstant %uint 128
%uint_0 = OpConstant %uint 0
%36 = OpTypeFunction %void
%_ptr_Function_v4float = OpTypePointer Function %v4float
%VS_OUTPUT = OpTypeStruct %v4float %v4float
%_ptr_Function_VS_OUTPUT = OpTypePointer Function %VS_OUTPUT
%in_var_POSITION = OpVariable %_ptr_Input_v4float Input
%in_var_COLOR = OpVariable %_ptr_Input_v4float Input
%gl_Position = OpVariable %_ptr_Output_v4float Output
%out_var_COLOR = OpVariable %_ptr_Output_v4float Output
%85 = OpExtInst %void %1 DebugOperation Deref
%81 = OpExtInst %void %1 DebugInfoNone
%52 = OpExtInst %void %1 DebugExpression
%40 = OpExtInst %void %1 DebugTypeBasic %8 %uint_32 Float
%41 = OpExtInst %void %1 DebugTypeVector %40 4
%42 = OpExtInst %void %1 DebugSource %7
%43 = OpExtInst %void %1 DebugCompilationUnit 1 4 %42 HLSL
%44 = OpExtInst %void %1 DebugTypeComposite %9 Structure %42 1 8 %43 %9 %uint_256 FlagIsProtected|FlagIsPrivate %45 %46
%46 = OpExtInst %void %1 DebugTypeMember %10 %41 %42 3 10 %44 %uint_128 %uint_128 FlagIsProtected|FlagIsPrivate
%45 = OpExtInst %void %1 DebugTypeMember %11 %41 %42 2 10 %44 %uint_0 %uint_128 FlagIsProtected|FlagIsPrivate
%47 = OpExtInst %void %1 DebugTypeFunction FlagIsProtected|FlagIsPrivate %44 %41 %41
%48 = OpExtInst %void %1 DebugFunction %12 %47 %42 6 1 %43 %13 FlagIsProtected|FlagIsPrivate 7 %81
%49 = OpExtInst %void %1 DebugLexicalBlock %42 7 38 %48
%50 = OpExtInst %void %1 DebugLocalVariable %14 %44 %42 8 13 %49 FlagIsLocal
%84 = OpExtInst %void %1 DebugExpression %85
%main = OpFunction %void None %36
%54 = OpLabel
%91 = OpExtInst %void %1 DebugScope %49
OpLine %7 7 23
%83 = OpVariable %_ptr_Function_v4float Function
OpLine %7 8 13
%87 = OpExtInst %void %1 DebugValue %50 %83 %84 %int_1
OpLine %7 7 23
%82 = OpVariable %_ptr_Function_v4float Function
OpLine %7 8 13
%86 = OpExtInst %void %1 DebugValue %50 %82 %84 %int_0
OpNoLine
%92 = OpExtInst %void %1 DebugNoScope
%55 = OpLoad %v4float %in_var_POSITION
%56 = OpLoad %v4float %in_var_COLOR
;CHECK: [[pos:%\w+]] = OpLoad %v4float %in_var_POSITION
;CHECK: [[color:%\w+]] = OpLoad %v4float %in_var_COLOR

%94 = OpExtInst %void %1 DebugScope %49
OpLine %7 9 3
OpStore %82 %55
;CHECK: OpLine [[file:%\w+]] 9 3
;CHECK: OpStore {{%\w+}} [[pos]]
;CHECK: {{%\w+}} = OpExtInst %void {{%\w+}} DebugValue [[vout:%\w+]] [[pos]] [[empty_expr:%\w+]] %int_0
;CHECK: OpLine [[file]] 10 3
;CHECK: OpStore {{%\w+}} [[color]]
;CHECK: {{%\w+}} = OpExtInst %void {{%\w+}} DebugValue [[vout]] [[color]] [[empty_expr]] %int_1

OpLine %7 10 3
OpStore %83 %56
OpLine %7 11 10
%90 = OpCompositeConstruct %VS_OUTPUT %55 %56
OpNoLine
%95 = OpExtInst %void %1 DebugNoScope
%58 = OpCompositeExtract %v4float %90 0
OpStore %gl_Position %58
%59 = OpCompositeExtract %v4float %90 1
OpStore %out_var_COLOR %59
OpReturn
OpFunctionEnd
)";

SetAssembleOptions(SPV_TEXT_TO_BINARY_OPTION_PRESERVE_NUMERIC_IDS);
SinglePassRunAndMatch<LocalSingleStoreElimPass>(text, false);
}

TEST_F(LocalSingleStoreElimTest, AddDebugValueforStoreOutOfDebugDeclareScope) {
const std::string text = R"(
OpCapability Shader
%1 = OpExtInstImport "OpenCL.DebugInfo.100"
OpMemoryModel Logical GLSL450
OpEntryPoint Vertex %main "main" %in_var_POSITION %in_var_COLOR %gl_Position %out_var_COLOR
%7 = OpString "simple.hlsl"
%8 = OpString "float"
%9 = OpString "VS_OUTPUT"
%10 = OpString "color"
%11 = OpString "pos"
%12 = OpString "main"
%13 = OpString ""
%14 = OpString "vout"
OpName %in_var_POSITION "in.var.POSITION"
OpName %in_var_COLOR "in.var.COLOR"
OpName %out_var_COLOR "out.var.COLOR"
OpName %main "main"
OpName %VS_OUTPUT "VS_OUTPUT"
OpMemberName %VS_OUTPUT 0 "pos"
OpMemberName %VS_OUTPUT 1 "color"
OpDecorate %gl_Position BuiltIn Position
OpDecorate %in_var_POSITION Location 0
OpDecorate %in_var_COLOR Location 1
OpDecorate %out_var_COLOR Location 0
%int = OpTypeInt 32 1
%int_0 = OpConstant %int 0
%int_1 = OpConstant %int 1
%uint = OpTypeInt 32 0
%uint_32 = OpConstant %uint 32
%float = OpTypeFloat 32
%v4float = OpTypeVector %float 4
%_ptr_Input_v4float = OpTypePointer Input %v4float
%_ptr_Output_v4float = OpTypePointer Output %v4float
%void = OpTypeVoid
%uint_256 = OpConstant %uint 256
%uint_128 = OpConstant %uint 128
%uint_0 = OpConstant %uint 0
%36 = OpTypeFunction %void
%_ptr_Function_v4float = OpTypePointer Function %v4float
%VS_OUTPUT = OpTypeStruct %v4float %v4float
%_ptr_Function_VS_OUTPUT = OpTypePointer Function %VS_OUTPUT
%in_var_POSITION = OpVariable %_ptr_Input_v4float Input
%in_var_COLOR = OpVariable %_ptr_Input_v4float Input
%gl_Position = OpVariable %_ptr_Output_v4float Output
%out_var_COLOR = OpVariable %_ptr_Output_v4float Output
%85 = OpExtInst %void %1 DebugOperation Deref
%81 = OpExtInst %void %1 DebugInfoNone
%52 = OpExtInst %void %1 DebugExpression
%40 = OpExtInst %void %1 DebugTypeBasic %8 %uint_32 Float
%41 = OpExtInst %void %1 DebugTypeVector %40 4
%42 = OpExtInst %void %1 DebugSource %7
%43 = OpExtInst %void %1 DebugCompilationUnit 1 4 %42 HLSL
%44 = OpExtInst %void %1 DebugTypeComposite %9 Structure %42 1 8 %43 %9 %uint_256 FlagIsProtected|FlagIsPrivate %45 %46
%46 = OpExtInst %void %1 DebugTypeMember %10 %41 %42 3 10 %44 %uint_128 %uint_128 FlagIsProtected|FlagIsPrivate
%45 = OpExtInst %void %1 DebugTypeMember %11 %41 %42 2 10 %44 %uint_0 %uint_128 FlagIsProtected|FlagIsPrivate
%47 = OpExtInst %void %1 DebugTypeFunction FlagIsProtected|FlagIsPrivate %44 %41 %41
%48 = OpExtInst %void %1 DebugFunction %12 %47 %42 6 1 %43 %13 FlagIsProtected|FlagIsPrivate 7 %81
%49 = OpExtInst %void %1 DebugLexicalBlock %42 7 38 %48
%50 = OpExtInst %void %1 DebugLocalVariable %14 %44 %42 8 13 %49 FlagIsLocal
%51 = OpExtInst %void %1 DebugLocalVariable %10 %41 %42 7 23 %48 FlagIsLocal 2
%53 = OpExtInst %void %1 DebugLocalVariable %11 %41 %42 6 23 %48 FlagIsLocal 1
;CHECK: [[dbg_color:%\w+]] = OpExtInst %void {{%\w+}} DebugLocalVariable {{%\w+}} {{%\w+}} {{%\w+}} 7 23 {{%\w+}} FlagIsLocal 2
;CHECK: [[dbg_pos:%\w+]] = OpExtInst %void {{%\w+}} DebugLocalVariable {{%\w+}} {{%\w+}} {{%\w+}} 6 23 {{%\w+}} FlagIsLocal 1

%84 = OpExtInst %void %1 DebugExpression %85
%main = OpFunction %void None %36
%54 = OpLabel
%91 = OpExtInst %void %1 DebugScope %49
OpLine %7 7 23
%83 = OpVariable %_ptr_Function_v4float Function
OpLine %7 8 13
%87 = OpExtInst %void %1 DebugValue %50 %83 %84 %int_1
OpLine %7 7 23
%82 = OpVariable %_ptr_Function_v4float Function
OpLine %7 8 13
%86 = OpExtInst %void %1 DebugValue %50 %82 %84 %int_0
OpNoLine
%92 = OpExtInst %void %1 DebugNoScope
%param_var_pos = OpVariable %_ptr_Function_v4float Function
%param_var_color = OpVariable %_ptr_Function_v4float Function
%55 = OpLoad %v4float %in_var_POSITION
OpStore %param_var_pos %55
%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
%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+]]
;CHECK: OpLine [[file]] 7 23
;CHECK-NEXT: {{%\w+}} = OpExtInst %void {{%\w+}} DebugValue [[dbg_color]] [[color]] [[empty_expr]]

%94 = OpExtInst %void %1 DebugScope %49
OpLine %7 9 3
OpStore %82 %55
OpLine %7 10 3
OpStore %83 %56
OpLine %7 11 10
%90 = OpCompositeConstruct %VS_OUTPUT %55 %56
OpNoLine
%95 = OpExtInst %void %1 DebugNoScope
%58 = OpCompositeExtract %v4float %90 0
OpStore %gl_Position %58
%59 = OpCompositeExtract %v4float %90 1
OpStore %out_var_COLOR %59
OpReturn
OpFunctionEnd
)";

SetAssembleOptions(SPV_TEXT_TO_BINARY_OPTION_PRESERVE_NUMERIC_IDS);
SinglePassRunAndMatch<LocalSingleStoreElimPass>(text, false);
}

// TODO(greg-lunarg): Add tests to verify handling of these cases:
//
// Other types
Expand Down