This is an archive of the discontinued LLVM Phabricator instance.

[DebugInfo][InstrRef] Handle joins PHI+Def values in LiveDebugValues
ClosedPublic

Authored by StephenTozer on May 19 2022, 1:54 AM.

Details

Summary

In the InstrRefBasedImpl for LiveDebugValues, we attempt to propagate debug values through basic blocks in part by checking to see whether all a variable's incoming debug values to a BB "agree", i.e. whether their properties match and they refer to the same underlying value.

Prior to this patch, the check for agreement between incoming values relied on exact equality, which meant that a VPHI and a Def DbgValue that referred to the same underlying value would be seen as disagreeing. This patch changes this behaviour to treat them as referring to the same value, allowing the shared value to propagate into the BB.

Diff Detail

Event Timeline

StephenTozer created this revision.May 19 2022, 1:54 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 19 2022, 1:54 AM
Herald added a subscriber: hiraditya. · View Herald Transcript
StephenTozer requested review of this revision.May 19 2022, 1:54 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 19 2022, 1:54 AM

IIRC this is sound, from our discussion elsewhere the problem is that we do not eliminate a PHI if the incoming values agree, but one value comes from a PHI and the other does not. IMO: this should also get a unit test entry to ease the identification of errors in the future. Additionally, the test file should be MIR and only run livedebugvalues, to reduce the surface of things being tested.

(I think I still need to convince myself that there's not some subtle ordering on PHI elimination that we affect by doing this -- that'll take a little time, but I'm pretty confident already).

llvm/lib/CodeGen/LiveDebugValues/InstrRefBasedImpl.cpp
7–8

IMO this needs a more precise (but still short comment), indicating _what_ about the agreement is acceptable here. i.e., "The difference is only that one value comes from a DBG_PHI, the other from a defining instruction or similar", or something in that vein.

Additionally, the test file should be MIR and only run livedebugvalues, to reduce the surface of things being tested.

(But keep the IR in the MIR file, it's really useful to have tricky IR hanging around for periodic end-to-end testing).

Added a unit test, updated the comment as requested. I haven't converted the IR test to an MIR test, because the reproducer I have does not produce valid intermediate MIR (likely a bug, but one unrelated to this patch) and I haven't been able to find a reproducer without this issue.

jmorse accepted this revision.Jul 25 2022, 10:43 AM

LGTM, with some quibbles.

llvm/lib/CodeGen/LiveDebugValues/InstrRefBasedImpl.cpp
7–8

I'm a terrible person for wanting to copy-edit your comments; but could you explicitly mention VPHI / Def conflict, i.e. "such as the same value from different sources (VPHI / Def)".

The reason is that I'm confident that I or some future developer won't be able to work out what's going on unless it's made extremely obvious. It's also why adding the unit test for this is awesome!

llvm/test/DebugInfo/X86/instr-ref-join-vlocs.ll
52–53 ↗(On Diff #444215)

I believe there's an active request (example: https://reviews.llvm.org/D127492#3577259 ) to not put undef into new tests :(. Can this be replaced with poison?

This revision is now accepted and ready to land.Jul 25 2022, 10:43 AM

Replace existing lit test with a manually created test that addresses the prior concerns.

The new test looks good but I have one question (inline).

llvm/test/DebugInfo/MIR/X86/instr-ref-join-def-vphi.mir
72–74

I think you could delete all these attributes

230

I'm confused; there isn't a debug-instr-number 5 and there are no debugValueSubstitutions - what am I missing here?

StephenTozer added inline comments.Aug 19 2022, 2:30 AM
llvm/test/DebugInfo/MIR/X86/instr-ref-join-def-vphi.mir
230

bb.3, DBG_PHI $esi, 5

Orlando accepted this revision.Aug 19 2022, 2:54 AM

The new test LGTM (& Jeremy has reviewed the rest)

llvm/test/DebugInfo/MIR/X86/instr-ref-join-def-vphi.mir
230

Ah right, gottcha.

This revision was landed with ongoing or failed builds.Aug 22 2022, 6:57 AM
This revision was automatically updated to reflect the committed changes.