Page MenuHomePhabricator

[DebugInfo][DAG] Always emit DBG_VALUEs of FrameIndexes regardless of their SDNodes
ClosedPublic

Authored by jmorse on Jan 28 2019, 7:32 AM.

Details

Summary

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

Diff Detail

Repository
rL LLVM

Event Timeline

jmorse created this revision.Jan 28 2019, 7:32 AM

We can create FrameIndex SDNodeDbgValue records even if the Value is never lowered to an SDNode in the current block.

That sounds awesome.

If a dbg.value pointing to a frame index and a dbg.value with a constant (both describing the same variable) appear in the same basic block, is the relative order between them lost?

lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
5326 ↗(On Diff #183850)

I'd probably use auto here.

jmorse updated this revision to Diff 183884.Jan 28 2019, 9:10 AM

Make use of auto, add constant-value dbg.value's to test for their correct ordering with FrameIndex dbg.values.

Adrian wrote:

If a dbg.value pointing to a frame index and a dbg.value with a constant (both describing the same variable) appear in the same basic block, is the relative order between them lost?

The order should not be lost -- they will both retain their IR location number, and ScheduleDAGSDNodes::EmitSchedule should emit them in that numbered order. I think the worst case scenario is that intervening instructions are completely optimised out, but even in that case they will still retain their ordering.

I've added a couple of constant dbg.values to the new test to check the ordering is preserved.

aprantl accepted this revision.Jan 28 2019, 11:01 AM

Thanks!

This revision is now accepted and ready to land.Jan 28 2019, 11:01 AM
This revision was automatically updated to reflect the committed changes.

After hitting the buildbots, a bunch of PowerPC builders started reporting XPASS for test/DebugInfo/Generic/incorrect-variable-debugloc1.ll. That XFAIL has an associated bug report:

https://bugs.llvm.org/show_bug.cgi?id=21881

Which appears to precisely document a scenario that this patch would solve. I've removed the XFAIL, written up my view on this in the bug report, and hopefully someone will confirm the PowerPC location list looks correct.

Step change in the LNT debuginfo tracking, marked up as a "regression" though:

http://lnt.llvm.org/db_default/v4/nts/regressions/19927

Step change in the LNT debuginfo tracking, marked up as a "regression" though:

http://lnt.llvm.org/db_default/v4/nts/regressions/19927

Nice!
@cmatthews: What would we need to do to make LNT mark the percent regions covered metric as higher-is-better? (This isn't necessarily true, but most of the time, it's what we expect.)