Skip to content

Remove DebugDeclare only for target variables in ssa-rewrite#3511

Merged
jaebaek merged 5 commits intoKhronosGroup:masterfrom
jaebaek:ssa_rewrite_remove_correct_dbg_decl
Jul 31, 2020
Merged

Remove DebugDeclare only for target variables in ssa-rewrite#3511
jaebaek merged 5 commits intoKhronosGroup:masterfrom
jaebaek:ssa_rewrite_remove_correct_dbg_decl

Conversation

@jaebaek
Copy link
Copy Markdown
Contributor

@jaebaek jaebaek commented Jul 10, 2020

For each local variable, ssa-rewrite should remove its DebugDeclare
if and only if it is replaced by any number of DebugValues for store
and phi instructions.

For example, when we have two variables a whose DebugDeclare
will be replaced to DebugValues by ssa-rewrite pass and b whose
DebugDeclare will not be replaced, we have to remove only DebugDeclare
for a, not b.

@jaebaek jaebaek requested a review from dnovillo July 10, 2020 21:33
@jaebaek jaebaek self-assigned this Jul 10, 2020
@jaebaek
Copy link
Copy Markdown
Contributor Author

jaebaek commented Jul 10, 2020

I missed this case in #3356.

Basically, the existing ssa-rewrite pass removes all DebugDeclare even when it should not.
If some DebugDeclare instructions are not propagated as DebugValue for store and phi instructions, we must not remove them.
For example,

 #version 140
 
 in vec4 BC;
 out float fo;
 
 void main()
 {
     float f = 0.0; 
     for (int j=0; j<4; j++) {
       // BB_inside
       int &i = j; 
       f = f + BC[i];
     }    
     fo = f; 
 }

 %f = OpVariable %_ptr_Function_float Function
 %j = OpVariable %_ptr_Function_int Function
 OpStore %f %float_0
 OpStore %j %int_0
 %decl0 = OpExtInst %void %ext DebugDeclare %dbg_f %f %null_expr
 %decl1 = OpExtInst %void %ext DebugDeclare %dbg_i %j %null_expr   ; %dbg_i == "int &i"

If we use %j for int &i in BB_inside, we must not add a DebugValue for the j++.
In this case, we must not remove %decl1 = OpExtInst %void %ext DebugDeclare %dbg_i %j %null_expr ; %dbg_i == "int &i".

Copy link
Copy Markdown
Contributor Author

@jaebaek jaebaek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I updated the comments and the description of this PR. PTAL.


void DebugInfoManager::AddDebugValue(Instruction* scope_and_line,
bool DebugInfoManager::AddDebugValue(Instruction* scope_and_line,
uint32_t variable_id, uint32_t value_id,
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See my comment in #3461. It's confusing that AddDebugValue doesn't always does what the caller meant (#3461 (comment)).

@s-perron
Copy link
Copy Markdown
Collaborator

I cannot remember if we have talked about this or not. This does not look right. Replacing a debug declare should always be an all or nothing. You can either replace the debug declare with a series of debug value instructions, or you do not add any debug value instructions.

More over, if ssa rewrite is able to rewrite the variable, then it should always be able to replace the debug declare with debug value instructions. So the logic should be: "if ssa-rewrite chooses to rewrite the variable, then the DebugDeclare should be removed." Nothing more complicated should be needed.

jaebaek added 3 commits July 28, 2020 17:02
ssa-rewrite should remove DebugDeclare only when it is replaced by
DebugValue instructions for every store and phi instruction.
@jaebaek jaebaek force-pushed the ssa_rewrite_remove_correct_dbg_decl branch from 8206df2 to eff4278 Compare July 28, 2020 21:12
@jaebaek
Copy link
Copy Markdown
Contributor Author

jaebaek commented Jul 28, 2020

@s-perron Thank you for your comment.

Let me clearly understand what the expected behavior is.
Let's assume that

  • There are two variables a, b
  • We can replace DebugDeclare of a to DebugValue, but we cannot do that for b

In this case, what we should do among the following E1 - E4?

  • E1: Replace only DebugDeclare of a to DebugValue and remove DebugDeclare of b
  • E2: Replace only DebugDeclare of a to DebugValue and keep DebugDeclare of b
  • E3: do not replace any DebugDeclare to DebugValue and just keep both DebugDeclares of a and b
  • E4: Replace DebugDeclare of a to DebugValue and replace DebugDeclare of b to DebugValue with Deref?
    • if b is already declared by DebugValue with Deref, do not update the DebugValue for b

@s-perron
Copy link
Copy Markdown
Collaborator

E2: Replace only DebugDeclare of a to DebugValue and keep DebugDeclare of b

Sorry that I was not clear. I was thinking of a single variable at a time, because logic of ssa-rewrite acts on each variable individually.

@jaebaek
Copy link
Copy Markdown
Contributor Author

jaebaek commented Jul 29, 2020

Thank you for clarifying it. I updated the description of this PR and the code. PTAL.

@jaebaek jaebaek requested a review from s-perron July 29, 2020 15:12
@jaebaek
Copy link
Copy Markdown
Contributor Author

jaebaek commented Jul 30, 2020

I agree. Thank you for the suggestion. I updated the code to simply kill DebugDeclares for target variables. PTAL

@jaebaek jaebaek changed the title Selectively remove DebugDeclare in ssa-rewrite Remove DebugDeclare only for target variables in ssa-rewrite Jul 30, 2020
@jaebaek jaebaek merged commit b78f4b1 into KhronosGroup:master Jul 31, 2020
@jaebaek jaebaek deleted the ssa_rewrite_remove_correct_dbg_decl branch July 31, 2020 14:00
dnovillo pushed a commit to dnovillo/SPIRV-Tools that referenced this pull request Aug 19, 2020
…Group#3511)

For each local variable, ssa-rewrite should remove its DebugDeclare
if and only if it is replaced by any number of DebugValues for store
and phi instructions.

For example, when we have two variables `a` whose DebugDeclare
will be replaced to DebugValues by ssa-rewrite pass and `b` whose
DebugDeclare will not be replaced, we have to remove only DebugDeclare
for `a`, not `b`.
dneto0 pushed a commit to dneto0/SPIRV-Tools that referenced this pull request Sep 14, 2024
Roll third_party/glslang/ 3ee5f2f..b60e067 (8 commits)

KhronosGroup/glslang@3ee5f2f...b60e067

$ git log 3ee5f2f..b60e067 --date=short --no-merges --format='%ad %ae %s'
2020-08-06 john SPV: Fix KhronosGroup#1829: don't emit OpModuleProcessed use-storage-buffer
2020-08-05 john Build/Test: Dropping 2013 allows using the latest googletests.
2020-08-04 john SPV: Standalone; sanity check the client GLSL input semantics option value.
2020-08-04 john SPV: Use more correct SPV-Tools environment, partially addressing KhronosGroup#2290
2020-08-04 john SPV: Fix KhronosGroup#2363: include trailing newline named text SPV output.
2020-07-03 ShabbyX Use GLSLANG_ANGLE to strip features to what ANGLE requires
2020-07-31 bclayton Revert changes that migrate to `thread_local`.
2020-07-27 dneto Avoid spurious warning about uninit var

Created with:
  roll-dep third_party/glslang

Roll third_party/googletest/ a781fe29b..3af06fe16 (12 commits)

google/googletest@a781fe2...3af06fe

$ git log a781fe29b..3af06fe16 --date=short --no-merges --format='%ad %ae %s'
2020-08-05 absl-team Googletest export
2020-08-03 absl-team Googletest export
2020-08-03 absl-team Googletest export
2020-08-05 zumix.cpp fix endif comment
2020-08-02 zumix.cpp fix tests
2020-07-29 franciscogthiesen Removing tiny-dnn from "Who is using.."
2020-07-28 absl-team Googletest export
2020-07-29 zumix.cpp fix GTEST_REMOVE_LEGACY_TEST_CASEAPI_ typo
2020-07-28 absl-team Googletest export
2020-07-26 ofats Googletest export
2020-07-19 jasjuang fix clang tidy modernize-use-equals-default warnings
2020-07-02 siliconearth Fix test failing when simple regex is used

Created with:
  roll-dep third_party/googletest

Roll third_party/spirv-cross/ 0376576d2..82d1c43e4 (7 commits)

KhronosGroup/SPIRV-Cross@0376576...82d1c43

$ git log 0376576d2..82d1c43e4 --date=short --no-merges --format='%ad %ae %s'
2020-08-03 cdavis MSL: Fix handling of matrices and structs in the output control point array.
2020-07-29 post Add some test cases for complex type aliasing scenario.
2020-07-29 post Ensure that we use primary alias type when emitting flattened members.
2020-07-29 post GLSL: Be more aggressive about using type_alias.
2020-07-29 post Only rewrite type aliases for the base type.
2020-07-28 post GLSL: Add option to force flattening IO blocks.
2020-07-23 tommek Adding BuiltInSampleMask in HLSL

Created with:
  roll-dep third_party/spirv-cross

Roll third_party/spirv-headers/ 979924c..3fdabd0 (4 commits)

KhronosGroup/SPIRV-Headers@979924c...3fdabd0

$ git log 979924c..3fdabd0 --date=short --no-merges --format='%ad %ae %s'
2020-08-03 44190824+mmerecki Reserve SPIR-V token range for upcoming Intel extensions. (KhronosGroup#165)
2020-07-29 alanbaker Update BUILD.bazel and BUILD.gn (KhronosGroup#166)
2020-07-29 alanbaker Publish the headers for the clspv embedded reflection non-semantic extended instruction set (KhronosGroup#164)
2020-07-29 johnkslang Update the registry in spir-v.xml to modernize and split out opcodes. (KhronosGroup#156)

Created with:
  roll-dep third_party/spirv-headers

Roll third_party/spirv-tools/ b63f0e5..2990a21 (30 commits)

KhronosGroup/SPIRV-Tools@b63f0e5...2990a21

$ git log b63f0e5..2990a21 --date=short --no-merges --format='%ad %ae %s'
2020-08-10 stevenperron Avoid using /MP4 for clang on windows. (KhronosGroup#3662)
2020-08-06 antonikarp spirv-fuzz: TransformationReplaceAddSubMulWithCarryingExtended (KhronosGroup#3598)
2020-08-06 andreperezmaselco.developer spirv-fuzz: Add TransformationMakeVectorOperationDynamic (KhronosGroup#3597)
2020-08-06 andreperezmaselco.developer spirv-fuzz: iterate over blocks in replace linear algebra pass (KhronosGroup#3654)
2020-08-06 stefanomil spirv-fuzz: make outliner pass use additional transformations (KhronosGroup#3604)
2020-08-05 jaebaek OpenCL.DebugInfo.100 DebugTypeArray with variable size (KhronosGroup#3549)
2020-08-05 andreperezmaselco.developer spirv-opt: Improve the code of the Instruction class (KhronosGroup#3610)
2020-08-05 vasniktel spirv-fuzz: Handle OpPhis in livesafe functions (KhronosGroup#3642)
2020-08-05 vasniktel spirv-fuzz: Handle OpPhi during constant obfuscation (KhronosGroup#3640)
2020-08-05 vasniktel spirv-fuzz: Fix FuzzerPassCopyObjects (KhronosGroup#3638)
2020-08-04 vasniktel spirv-fuzz: Remove OpFunctionCall operands in correct order (KhronosGroup#3630)
2020-08-04 vasniktel spirv-fuzz: Handle capabilities during module donation (KhronosGroup#3651)
2020-08-04 vasniktel spirv-fuzz: Refactor boilerplate in TransformationAddParameter (KhronosGroup#3625)
2020-08-03 vasniktel spirv-fuzz: TransformationMoveInstructionDown (KhronosGroup#3477)
2020-07-31 jaebaek Remove DebugDeclare only for target variables in ssa-rewrite (KhronosGroup#3511)
2020-07-31 vasniktel Fix typo in ASAN CI build (KhronosGroup#3623)
2020-07-30 stefanomil spirv-fuzz: Transformation to add loop preheader (KhronosGroup#3599)
2020-07-30 stefanomil spirv-fuzz: Pass to replace int operands with ints of opposite signedness (KhronosGroup#3612)
2020-07-30 jaebaek Debug info preservation in loop-unroll pass (KhronosGroup#3548)
2020-07-30 alanbaker Validator support for non-semantic clspv reflection (KhronosGroup#3618)
2020-07-30 vasniktel spirv-fuzz: Fix memory bugs (KhronosGroup#3622)
2020-07-29 andreperezmaselco.developer spirv-fuzz: Implement the OpOuterProduct linear algebra case (KhronosGroup#3617)
2020-07-30 vasniktel spirv-fuzz: Compute corollary facts from OpBitcast (KhronosGroup#3538)
2020-07-29 dj2 Update some language usage. (KhronosGroup#3611)
2020-07-29 vasniktel spirv-fuzz: Relax type constraints in DataSynonym facts (KhronosGroup#3602)
2020-07-29 vasniktel spirv-fuzz: Remove non-deterministic behaviour (KhronosGroup#3608)
2020-07-29 afdx Avoid use of 'sanity' and 'sanity check' in the code base (KhronosGroup#3585)
2020-07-27 andreperezmaselco.developer spirv-fuzz: Add condition to make functions livesafe (KhronosGroup#3587)
2020-07-27 rharrison Rolling 4 dependencies (KhronosGroup#3601)
2020-07-27 andreperezmaselco.developer spirv-fuzz: Implement the OpTranspose linear algebra case (KhronosGroup#3589)

Created with:
  roll-dep third_party/spirv-tools
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants