Page MenuHomePhabricator

[DebugInfo] Factor out SalvageDebugInfoForDbgValues() from InstCombine
Needs ReviewPublic

Authored by chrisjackson on Apr 17 2020, 12:55 PM.

Details

Summary

The SalvageDebugInfoForDbgValues() function is exposed and used externally only in InstCombine. Refactor InstCombine to instead use SalvageDebugInfo() and keep SalvageDebugInfoForDbgValues() internal.
However, this relies on the internal behaviour of SalvageDbgInfo(), specifically the way findDbgValues() behaves when it encounters undef. This patch builds on top of D78369, which also simplifies the exposed debuginfo salvaging functions.

The guard in InstructionCombining.cpp:3477

if(!DIIClones.empty())

is used to prevent a salvage occurring in a scenario not possible in the current implementation. Without this a salvage can be executed without having cloned any Users. Doing so may not be incorrect, but it certainly would be different to the current behaviour.

Diff Detail

Event Timeline

chrisjackson created this revision.Apr 17 2020, 12:55 PM
chrisjackson edited the summary of this revision. (Show Details)Apr 17 2020, 12:55 PM
vsk added a comment.Apr 17 2020, 3:14 PM

I personally find the existing code easier to read/understand. Is there a strong motivation for hiding salvageDebugInfoForDbgValues?

In D78398#1989878, @vsk wrote:

I personally find the existing code easier to read/understand. Is there a strong motivation for hiding salvageDebugInfoForDbgValues?

The intention was to reduce the variety of salvage calls that are made, making salvaging easier to understand and reducing errors by reducing the choice of calls. In the original InstCombine code we make repeated salvage calls against the instruction instead of just one, which is unusual.

Assuming D78369 lands as it is right now; IMO it might make sense to push the OrUndef behaviour of salvageDebugInfo into salvageDebugInfoForDbgValues, and have salvageDebugInfo simply call salvageDebugInfoForDbgValues(I, findDbgUsers(I)).

This is pretty similar to what happens pre-D78369, except that salvageDebugInfo and salvageDebugInfoForDbgValues both MarkUndef if they fail to salvage. I'm fairly confident you could then simplify the instcombine sink salvaging with this setup.

What do you both think?

If this relies on D78369, please make a stack of these patches.

llvm/lib/Transforms/Utils/Local.cpp
1649

These functions need comments describing the intention.

vsk added a comment.Apr 21 2020, 2:50 PM
In D78398#1989878, @vsk wrote:

I personally find the existing code easier to read/understand. Is there a strong motivation for hiding salvageDebugInfoForDbgValues?

The intention was to reduce the variety of salvage calls that are made, making salvaging easier to understand and reducing errors by reducing the choice of calls. In the original InstCombine code we make repeated salvage calls against the instruction instead of just one, which is unusual.

That's fair, that's a good goal. I admit the formatting of the patch really made it harder to get a sense for what it's doing -- could you please clang-format the diff?

Assuming D78369 lands as it is right now; IMO it might make sense to push the OrUndef behaviour of salvageDebugInfo into salvageDebugInfoForDbgValues, and have salvageDebugInfo simply call salvageDebugInfoForDbgValues(I, findDbgUsers(I)).

This is pretty similar to what happens pre-D78369, except that salvageDebugInfo and salvageDebugInfoForDbgValues both MarkUndef if they fail to salvage. I'm fairly confident you could then simplify the instcombine sink salvaging with this setup.

What do you both think?

Makes sense to me.

llvm/lib/Transforms/InstCombine/InstructionCombining.cpp
3463

@jmorse Is the reverse really needed? Not sure the iteration order in findDbgUsers has any meaning.

3465

Please use early-exits where you can to reduce nesting.

Applied clang-format-diff.

jmorse added inline comments.Apr 22 2020, 8:57 AM
llvm/lib/Transforms/InstCombine/InstructionCombining.cpp
3463

Looks like I mentioned that on the last line of this comment: https://reviews.llvm.org/D56788#1394898

It's not something that inspires confidence; I think in retrospect that additional salvaging meant additional sinking, giving the test mentioned in that comment an extra dbg.value that isn't tested for. There are three dbg.values now, and only two are tested for. At the time, I probably mistook a new / extra dbg.value as an accidental change in ordering.

I agree the order in findDbgUsers isn't meaningful, the reverse can go.

chrisjackson added inline comments.Apr 27 2020, 1:11 AM
llvm/lib/Transforms/InstCombine/InstructionCombining.cpp
3463

I considered removing the reverse but doing so changed the ordering of some output and required a change in test/Transforms/InstCombine/debuginfo_add.ll. I will update the code and test.

Herald added a project: Restricted Project. · View Herald TranscriptApr 27 2020, 1:11 AM
  • Removed the call to reverse() in InstCombine.cpp and updated the debuginfo_add.ll test accordingly.
  • Moved some comments in InstCombine to a more appropriate spot.
  • Simplified an iteration in InstCombine as suggested.
vsk added a comment.Thu, May 7, 11:53 AM

I think this is in good shape.

llvm/lib/Transforms/InstCombine/InstructionCombining.cpp
3442

nit, 'arguments of' -> 'dbg.declare arguments'?

3445

nit, (if I'm reading this correctly) it should be possible to remove a level of nesting by doing both isa checks in the first if

3465

nit: !=

Assuming D78369 lands as it is right now; IMO it might make sense to push the OrUndef behaviour of salvageDebugInfo into salvageDebugInfoForDbgValues, and have salvageDebugInfo simply call salvageDebugInfoForDbgValues(I, findDbgUsers(I)).

This is pretty similar to what happens pre-D78369, except that salvageDebugInfo and salvageDebugInfoForDbgValues both MarkUndef if they fail to salvage. I'm fairly confident you could then simplify the instcombine sink salvaging with this setup.

What do you both think?

This seems reasonable. I'll complete the requested changes to this diff then I'll submit one with @Orlando's suggested approach and we can see which is looks better.

Assuming D78369 lands as it is right now; IMO it might make sense to push the OrUndef behaviour of salvageDebugInfo into salvageDebugInfoForDbgValues, and have salvageDebugInfo simply call salvageDebugInfoForDbgValues(I, findDbgUsers(I)).

This is pretty similar to what happens pre-D78369, except that salvageDebugInfo and salvageDebugInfoForDbgValues both MarkUndef if they fail to salvage. I'm fairly confident you could then simplify the instcombine sink salvaging with this setup.

What do you both think?

I have uploaded a diff implementing this suggested alternative, D79863.