[PATCH] D88896: [DebugInfo][InstrRef][3/4] Produce DBG_INSTR_REFs for all* variable locations

[Orlando Cazalet-Hyams via Phabricator via llvm-commits]

Orlando accepted this revision.
Orlando added a comment.
This revision is now accepted and ready to land.

In D88896#2858425 <https://reviews.llvm.org/D88896#2858425>, @jmorse wrote:

> Reshuffle this patch -- the "fixup half done instruction references" code moves to MachineFunction::finalizeDebugInstrRefs, and I've removed the ignore-widening-subreg-substitutions code. It didn't have any significant effect on variable locations, I think it was just dropping information.

SGTM

> Paging @StephenTozer , I've reshuffled debug instruction emission a little after the variadic-locations work landed -- I've moved single operand emission to `InstrEmitter::EmitDbgValueFromSingleOp`, and no-location emission to `InstrEmitter::EmitDbgNoLocation`. Now, empty variadic variable locations cannonicalise to DBG_VALUE $noreg which casuses a few test changes. arg-dbg-value-list.ll caught my eye -- can variadic locations not recover unused argument values right now? (I imagine this is tied up in the nest of special casing for arguments in SelectionDAG).

I've left an inline comment about the empty variable location canonicalization (why it was the way it was before, plus a nit).

LGTM - though there are some clang-format nits plus my inline nit that need addressing before this lands.



================
Comment at: llvm/lib/CodeGen/SelectionDAG/InstrEmitter.cpp:700-704
     if (SD->isInvalidated()) {
       // At least one node is no longer computed so we are unable to emit the
       // location. Simply mark all of the operands as undef.
       for (size_t i = 0; i < LocationOps.size(); ++i)
         MIB.addReg(0U, RegState::Debug);
----------------
You can remove this bit of code now that you've hoisted the `if (SD->isInvalidated())` outside of the `if (SD->isVariadic())`. 

FWIW invalidated DBG_VALUE_LIST were emitted in this way (`$noreg` all the operands rather than emit a `DBG_VALUE $noreg`) with the aim of helping track down debug-info issues - the thinking being that the number of operands might hint at the identity of the DBG_VALUE when looking at print-after-all traces. I don't know whether that's a compelling enough reason to keep that way so I thought I'd just mention it in case you didn't' know, and let you decide.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D88896/new/

https://reviews.llvm.org/D88896