[PATCH] D57328: [DebugInfo][DAG] Always emit DBG_VALUEs of FrameIndexes regardless of their SDNodes

[Jeremy Morse via Phabricator via llvm-commits]

jmorse created this revision.
jmorse added reviewers: vsk, aprantl, bjope.
Herald added subscribers: llvm-commits, arphaman, jholewinski.

This patch recognises dbg.values of Alloca/FrameIndexes and processes them independently of other dbg.value lowering rules. There are two advantages:

- We can create FrameIndex SDNodeDbgValue records even if the Value is never lowered to an SDNode in the current block.
- We do not attach the SDNodeDbgValue to an SDNode: the variable location is still valid even if the SDNode is invalidated.

This catches two sets of circumstances where dbg.values are currently dropped, respectively:

- dbg.values of frame index Values that are not referenced in the current block,
- dbg.values of frame index Values where corresponding SDNode is folded into the operand of a memory instruction: such nodes are marked Invalidated and emitted as undef DBG_VALUEs.

The attached new test replicates both of these circumstances, with current trunk both dbg.values become undef DBG_VALUEs. There's a corresponding DebugInfo test (without the '2' suffix) that tests for FrameIndexes being lowered, however it tests a circumstance where the SDNode for the frameindex is not invalidated.

The NVPTX test changes because DBG_VALUEs are now emitted in the correct order. In the source file the order of dbg.values have types {const, const, vreg, frameindex}, but the test was originally getting {frameindex, const, const, vreg} as output. This patch restores it to the correct order. I'm 99% certain the reason it was originally misordered is the ProcessSourceNode matter mentioned at the end of D57163 <https://reviews.llvm.org/D57163>, which is now bypassed by the FrameIndex SDNodeDbgValue not being attached to an SDNode.

The assumption of this patch is that FrameIndexes are always a valid target of DBG_VALUEs: it bypasses the various ways in which SDNodes might not be emitted to machine instructions. There's a risk that the patch is incorrect if FrameIndexes are later garbage collected and the DBG_VALUEs not erased, however I don't currently know where to look to check for this.

There's an explanation for why getFrameIndexDbgValue gets passed 'false' for IsIndirect in SelectionDAGBuilder::getDbgValue, it might be worth copying that comment into the patched code?

Finally, using r351324 llvm/clang and building clang-3.4 with and without this patch, I get improvements:

- %variable coverage, 66% -> 75%
- %scope bytes covered, 39% -> 44%

Which... seems a little too good to be true, thus extra scrutiny may be in order. (My numbers have never lined up with the series on lnt.llvm.org).


Repository:
  rL LLVM

https://reviews.llvm.org/D57328

Files:
  lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
  test/DebugInfo/NVPTX/dbg-value-const-byref.ll
  test/DebugInfo/X86/dbg-value-frame-index-2.ll

-------------- next part --------------
A non-text attachment was scrubbed...
Name: D57328.183850.patch
Type: text/x-patch
Size: 4428 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20190128/9445b4e1/attachment.bin>