Skip to content

Commit

Permalink
Add DebugValue for function param regardless of scope (KhronosGroup#3923
Browse files Browse the repository at this point in the history 
)

`DebugInfoManager::AddDebugValueIfVarDeclIsVisible()` adds
OpenCL.DebugInfo.100 DebugValue from DebugDeclare only when the
DebugDeclare is visible to the give scope. It helps us correctly
handle a reference variable e.g.,

{ // scope #1.
  int foo = /* init */;
  { // scope #2.
    int& bar = foo;
    ...

in the above code, we must not propagate DebugValue of `int& bar` for
store instructions in the scope #1 because it is alive only in
the scope #2.

We have an exception: If the given DebugDeclare is used for a function
parameter, `DebugInfoManager::AddDebugValueIfVarDeclIsVisible()` has
to always add DebugValue instruction regardless
of the scope. It is because the initializer (store instruction) for
the function parameter can be out of the function parameter's scope
(the function) in particular when the function was inlined.

Without this change, the function parameter value information always
disappears whenever we run the function inlining pass and
`DebugInfoManager::AddDebugValueIfVarDeclIsVisible()`.
  • Loading branch information
@jaebaek
jaebaek authored Oct 16, 2020
1 parent 663d050 commit fd3948e
Show file tree
Hide file tree
Showing 3 changed files with 116 additions and 12 deletions.
39 changes: 31 additions & 8 deletions source/opt/debug_info_manager.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@ static const uint32_t kDebugLocalVariableOperandParentIndex = 9;
static const uint32_t kExtInstInstructionInIdx = 1;
static const uint32_t kDebugGlobalVariableOperandFlagsIndex = 12;
static const uint32_t kDebugLocalVariableOperandFlagsIndex = 10;
static const uint32_t kDebugLocalVariableOperandArgNumberIndex = 11;

namespace spvtools {
namespace opt {
Expand Down Expand Up @@ -440,15 +441,27 @@ bool DebugInfoManager::IsAncestorOfScope(uint32_t scope, uint32_t ancestor) {
return false;
}

bool DebugInfoManager::IsDeclareVisibleToInstr(Instruction* dbg_declare,
uint32_t instr_scope_id) {
if (instr_scope_id == kNoDebugScope) return false;

Instruction* DebugInfoManager::GetDebugLocalVariableFromDeclare(
Instruction* dbg_declare) {
assert(dbg_declare);
uint32_t dbg_local_var_id =
dbg_declare->GetSingleWordOperand(kDebugDeclareOperandLocalVariableIndex);
auto dbg_local_var_itr = id_to_dbg_inst_.find(dbg_local_var_id);
assert(dbg_local_var_itr != id_to_dbg_inst_.end());
uint32_t decl_scope_id = dbg_local_var_itr->second->GetSingleWordOperand(
return dbg_local_var_itr->second;
}

bool DebugInfoManager::IsFunctionParameter(Instruction* dbg_local_var) const {
// If a DebugLocalVariable has ArgNumber operand, it is a function parameter.
return dbg_local_var->NumOperands() >
kDebugLocalVariableOperandArgNumberIndex;
}

bool DebugInfoManager::IsLocalVariableVisibleToInstr(Instruction* dbg_local_var,
uint32_t instr_scope_id) {
if (instr_scope_id == kNoDebugScope) return false;

uint32_t decl_scope_id = dbg_local_var->GetSingleWordOperand(
kDebugLocalVariableOperandParentIndex);

// If the scope of DebugDeclare is an ancestor scope of the instruction's
Expand Down Expand Up @@ -502,11 +515,20 @@ void DebugInfoManager::AddDebugValueIfVarDeclIsVisible(

uint32_t instr_scope_id = scope_and_line->GetDebugScope().GetLexicalScope();
for (auto* dbg_decl_or_val : dbg_decl_itr->second) {
if (!IsDeclareVisibleToInstr(dbg_decl_or_val, instr_scope_id)) continue;
// If it declares a function parameter, the store instruction for the
// function parameter can exist out of the function parameter's scope
// because of the function inlining. We always add DebugValue for a
// function parameter next to the DebugDeclare regardless of the scope.
auto* dbg_local_var = GetDebugLocalVariableFromDeclare(dbg_decl_or_val);
bool is_function_param = IsFunctionParameter(dbg_local_var);
if (!is_function_param &&
!IsLocalVariableVisibleToInstr(dbg_local_var, instr_scope_id))
continue;

// Avoid inserting the new DebugValue between OpPhi or OpVariable
// instructions.
Instruction* insert_before = insert_pos->NextNode();
Instruction* insert_before = is_function_param ? dbg_decl_or_val->NextNode()
: insert_pos->NextNode();
while (insert_before->opcode() == SpvOpPhi ||
insert_before->opcode() == SpvOpVariable) {
insert_before = insert_before->NextNode();
Expand All @@ -523,7 +545,8 @@ void DebugInfoManager::AddDebugValueIfVarDeclIsVisible(
kDebugValueOperandLocalVariableIndex),
value_id, 0, index_id, insert_before);
assert(added_dbg_value != nullptr);
added_dbg_value->UpdateDebugInfoFrom(scope_and_line);
added_dbg_value->UpdateDebugInfoFrom(is_function_param ? dbg_decl_or_val
: scope_and_line);
AnalyzeDebugInst(added_dbg_value);
}
}
Expand Down
15 changes: 11 additions & 4 deletions source/opt/debug_info_manager.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -215,10 +215,17 @@ class DebugInfoManager {
// of |scope|.
bool IsAncestorOfScope(uint32_t scope, uint32_t ancestor);

// Returns true if the declaration of a local variable |dbg_declare|
// is visible in the scope of an instruction |instr_scope_id|.
bool IsDeclareVisibleToInstr(Instruction* dbg_declare,
uint32_t instr_scope_id);
// Returns the DebugLocalVariable declared by |dbg_declare|.
Instruction* GetDebugLocalVariableFromDeclare(Instruction* dbg_declare);

// Returns true if the DebugLocalVariable |dbg_local_var| is a function
// parameter.
bool IsFunctionParameter(Instruction* dbg_local_var) const;

// Returns true if the DebugLocalVariable |dbg_local_var| is visible
// in the scope of an instruction |instr_scope_id|.
bool IsLocalVariableVisibleToInstr(Instruction* dbg_local_var,
uint32_t instr_scope_id);

// Returns the parent scope of the scope |child_scope|.
uint32_t GetParentScope(uint32_t child_scope);
Expand Down
74 changes: 74 additions & 0 deletions test/opt/local_single_store_elim_test.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1211,6 +1211,80 @@ TEST_F(LocalSingleStoreElimTest, DebugValueTest) {
SinglePassRunAndMatch<LocalSingleStoreElimPass>(text, false);
}

TEST_F(LocalSingleStoreElimTest, DebugDeclareForFunctionParameter) {
// We have to add DebugValue for DebugDeclare regardless of the scope
// if it declares a function parameter.
const std::string text = R"(
OpCapability Shader
%1 = OpExtInstImport "OpenCL.DebugInfo.100"
OpMemoryModel Logical GLSL450
OpEntryPoint GLCompute %main "main"
OpExecutionMode %main LocalSize 1 1 1
%3 = OpString "fn_call.hlsl"
%4 = OpString "int"
%5 = OpString ""
%6 = OpString "x"
%7 = OpString "A"
%8 = OpString "main"
OpName %type_StructuredBuffer_int "type.StructuredBuffer.int"
OpName %data "data"
OpName %main "main"
OpDecorate %data DescriptorSet 0
OpDecorate %data Binding 0
OpDecorate %_runtimearr_int ArrayStride 4
OpMemberDecorate %type_StructuredBuffer_int 0 Offset 0
OpMemberDecorate %type_StructuredBuffer_int 0 NonWritable
OpDecorate %type_StructuredBuffer_int BufferBlock
%int = OpTypeInt 32 1
%uint = OpTypeInt 32 0
%uint_0 = OpConstant %uint 0
%uint_32 = OpConstant %uint 32
%_runtimearr_int = OpTypeRuntimeArray %int
%type_StructuredBuffer_int = OpTypeStruct %_runtimearr_int
%_ptr_Uniform_type_StructuredBuffer_int = OpTypePointer Uniform %type_StructuredBuffer_int
%void = OpTypeVoid
%18 = OpTypeFunction %void
%_ptr_Function_int = OpTypePointer Function %int
%_ptr_Uniform_int = OpTypePointer Uniform %int
%data = OpVariable %_ptr_Uniform_type_StructuredBuffer_int Uniform
%21 = OpExtInst %void %1 DebugInfoNone
%22 = OpExtInst %void %1 DebugExpression
%23 = OpExtInst %void %1 DebugTypeBasic %4 %uint_32 Signed
%24 = OpExtInst %void %1 DebugTypeFunction FlagIsProtected|FlagIsPrivate %23 %23
%25 = OpExtInst %void %1 DebugSource %3
%26 = OpExtInst %void %1 DebugCompilationUnit 1 4 %25 HLSL
%27 = OpExtInst %void %1 DebugFunction %7 %24 %25 17 1 %26 %5 FlagIsProtected|FlagIsPrivate 18 %21
%28 = OpExtInst %void %1 DebugLocalVariable %6 %23 %25 17 11 %27 FlagIsLocal 1
%29 = OpExtInst %void %1 DebugTypeFunction FlagIsProtected|FlagIsPrivate %void
%30 = OpExtInst %void %1 DebugFunction %8 %29 %25 25 1 %26 %5 FlagIsProtected|FlagIsPrivate 25 %21
%31 = OpExtInst %void %1 DebugLexicalBlock %25 25 13 %30
%32 = OpExtInst %void %1 DebugInlinedAt 26 %31
;CHECK: [[fn_param:%\w+]] = OpExtInst %void [[ext:%\w+]] DebugLocalVariable
;CHECK: [[bb:%\w+]] = OpExtInst %void [[ext]] DebugLexicalBlock
;CHECK: [[inlined_at:%\w+]] = OpExtInst %void [[ext]] DebugInlinedAt 26 [[bb]]
%main = OpFunction %void None %18
%bb = OpLabel
%33 = OpExtInst %void %1 DebugScope %31
%34 = OpVariable %_ptr_Function_int Function
OpLine %3 26 15
%35 = OpAccessChain %_ptr_Uniform_int %data %uint_0 %uint_0
%36 = OpLoad %int %35
;CHECK: DebugScope [[bb]]
;CHECK: OpLine {{%\w+}} 26 15
;CHECK: OpStore {{%\w+}} [[val:%\w+]]
OpStore %34 %36
%37 = OpExtInst %void %1 DebugScope %27 %32
%38 = OpExtInst %void %1 DebugDeclare %28 %34 %22
;CHECK: DebugScope {{%\w+}} [[inlined_at:%\w+]]
;CHECK: DebugValue [[fn_param]] [[val]]
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

0 comments on commit fd3948e

Please sign in to comment.