This is an archive of the discontinued LLVM Phabricator instance.

[DebugInfo][InstrRef][4/4] Support using DBG_INSTR_REF through all* backend passes
ClosedPublic

Authored by jmorse on Oct 6 2020, 6:45 AM.

Details

Summary

This patch does some cleanup to ensure that a clang stage2 build succeeds when using only DBG_INSTR_REF variable locations (plus the exceptions noted in the previous patch). There are still passes that will need updating, however most variable locations now survive most passes.

The most significant change is in LiveDebugVariables: because there are no DBG_VALUEs referring to virtual registers (which is now asserted), we can get away with just unlinking debug instructions from blocks, then linking them back in after regalloc finishes. This avoids the interval splitting and other operations. Debug instructions are stored with their slot and block, then re-inserted in the order they were removed.

Other passes need a tweak or two to correctly identify debug instructions now that DBG_PHIs can be generated during SelectionDAG.

Parameter variable locations are emitted by directly constructing a DBG_VALUE to be inserted later during SelectionDAGBuilder; I've added a helper lambda there to emit a DBG_INSTR_REF when we're using instruction referencing.

X86InstrInfo.cpp needs to un-set an instruction number: X86 usually creates subtract instructions instead of CMPs, then mutates them to be CMPs later on if no-one uses the result. That causes any debug users referring to the subs register def to now refer to a register use; which is an erronous behaviour.

Finally, I've deleted a bunch of DBG_VALUE %vreg's from the tests for other instruction referencing behaviour: they shouldn't be generated by SelectionDAG now, and shouldn't be in test cases for instruction referencing. None of them affect the tested behaviours, those all involve DBG_INSTR_REFs.

Diff Detail

Event Timeline

jmorse created this revision.Oct 6 2020, 6:45 AM
Herald added a project: Restricted Project. · View Herald TranscriptOct 6 2020, 6:45 AM
djtodoro added inline comments.
llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
5518

MIB.addReg(Reg, RegState::Debug);

Hi,

Full disclosure: I've not looked at the instruction-referencing work for a little while and I don't remember all the details of the full patch-set. That said, I think this patch LGTM by itself.

I've left some comments inline and there is an inline comment from @djtodoro. Does anyone else have any comments (and should this patch have some reviewers assigned to it)? If not I will tentatively accept this soon.

Thanks,
Orlando

llvm/lib/CodeGen/LiveDebugVariables.cpp
1893–1894

nit: maybe it's just me but "insert in front of it" feels a bit ambiguous in this scenario, perhaps "insert before it" is clearer?

1903

nit: putting the declaration in the if feels better to me, but it doesn't really matter much.

if (MachineInstr *Pos = Slots->getInstructionFromIndex(Idx)) {
llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
5705

Shouldn't the MakeVRegDbgValue parameter Indirect here be IsIndirect rather than IsDbgDeclare, or are IsIndirect and IsDbgDeclare equivalent at this point? I'm just looking at the removed code in the diff so could be missing something.

llvm/lib/Target/X86/X86InstrInfo.cpp
4186

Is this covered by any of the tests?

jmorse updated this revision to Diff 356651.Jul 6 2021, 3:11 AM
jmorse marked 3 inline comments as done.

Revisions:

  • Adjusted LiveDebugVariables to handle DBG_VALUE_LISTs in the "normal" way under instruction referencing, we don't attempt currently handle them with instr-ref right now
  • Fixed up an "isIndirect" issue, see inline comments
  • Added a test for SUB -> CMP peephole optimisation.
llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
5705

I think this hinged on an assumption that I made in the past, that DbgDeclares only ever pointed at memory operands or something. That assumption turned out to be false, there was an assertion left hanging that D101594 cleared up.

Switched to passing IsIndirect in for all these things.

llvm/lib/Target/X86/X86InstrInfo.cpp
4186

Added x86-drop-compare-inst.mir to cover this.

jmorse updated this revision to Diff 356702.Jul 6 2021, 6:45 AM

Shoe-horn in a collection of tiny adjustments: in register coalescing and liveintervals I've switched a few "isDebugValue" tests to be "isDebugInstr" instead. In all the modified cases, the code is seeking to avoid interpreting debug instructions or debug uses of a register -- I don't believe it matters whether it's a DBG_VALUE, DBG_VALUE_LIST, or DBG_PHI. Making these tests agnostic as to the instruction kind protects against codegen-changes-with-debugging-enabled.

Orlando accepted this revision.Jul 6 2021, 6:56 AM

LGTM, thanks.

This revision is now accepted and ready to land.Jul 6 2021, 6:56 AM
This revision was landed with ongoing or failed builds.Jul 8 2021, 8:42 AM
This revision was automatically updated to reflect the committed changes.