Skip to content

Commit

Permalink
Revert "Add DebugValue for function param regardless of scope (Khrono…
Browse files Browse the repository at this point in the history 
…sGroup#3923)"

This reverts commit fd3948e.
  • Loading branch information
@jaebaek
jaebaek committed Oct 27, 2020
1 parent 34ae8a4 commit 838a2d2
Show file tree
Hide file tree
Showing 3 changed files with 12 additions and 116 deletions.
39 changes: 8 additions & 31 deletions source/opt/debug_info_manager.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,6 @@ 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 @@ -441,27 +440,15 @@ bool DebugInfoManager::IsAncestorOfScope(uint32_t scope, uint32_t ancestor) {
return false;
}

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

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());
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(
uint32_t decl_scope_id = dbg_local_var_itr->second->GetSingleWordOperand(
kDebugLocalVariableOperandParentIndex);

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

uint32_t instr_scope_id = scope_and_line->GetDebugScope().GetLexicalScope();
for (auto* dbg_decl_or_val : dbg_decl_itr->second) {
// 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;
if (!IsDeclareVisibleToInstr(dbg_decl_or_val, instr_scope_id)) continue;

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

// 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 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 parent scope of the scope |child_scope|.
uint32_t GetParentScope(uint32_t child_scope);
Expand Down
74 changes: 0 additions & 74 deletions test/opt/local_single_store_elim_test.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1211,80 +1211,6 @@ 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 838a2d2

Please sign in to comment.