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

[StephenTozer]

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.

[jmorse]

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.

[jmorse]

In D125953#3527758, @jmorse wrote:

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).

[StephenTozer]

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]

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?

[StephenTozer]

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

[Orlando]

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]

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

230

bb.3, DBG_PHI $esi, 5

[Orlando]

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

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

230

Ah right, gottcha.