Page MenuHomePhabricator

[DebugInfo][DAG] PR40427: Avoid accidentally re-ordering DBG_VALUEs due to assumptions about inst creation

Authored by jmorse on Jan 24 2019, 8:49 AM.



This patch improves the placement of DBG_VALUEs when created by SelectionDAG by collecting more precise placement information. As documented in PR40427 [0] SelectionDAGs currently muddles which MIR instructions correspond to which IR instructions, leading to off-by-ones and debug-use-before-defs. At the core of this is ProcessSourceNode, which assumes the last instruction in a BB is the start of the last processed IR instruction, which isn't always true.

The 'Orders' mapping in EmitSchedule is supposed to (AFAICT) identify the first MIR instruction of an IR instruction. This patch adds an EmitNode helper that explicitly collects this information, by:

  • Storing the last instruction in the BB,
  • Emitting the designated SDNode,
  • Comparing the last-instruction-in-bb after emission, to what it was before,
  • Returning the first of any new instructions.

This is an awkwardly verbose way of finding the first instruction that an SDNode produces, however I'm not aware of any facility in InstrEmitter that provides this information, thus it must be determined by observation.

This patch also adjusts ProcessSourceNode: it no longer always records an IR instruction (identified by Order number) as having been seen whenever an SDNode is lowered, it now only does it if a MIR instruction is generated too. We also don't enter 'nullptr' records for SDNodes at the start of the function: the debuginfo code at the end of EmitSchedule is already designed to cope with missing IR Order numbers, rely on that rather than trying to second guess it.

With a stage2 build of llvm/clang 3.4 we get a fractional tick up in variable locations, and a fractional tick down in bytes coverage. This is probably due to a few use-before-defs being eliminated, and the combined effect of many DBG_VALUEs no longer being off-by-one'd.

The NVPTX test change is because a DBG_VALUE shifts down one instruction, creating a new local label in the output file, re-numbering others. pr40427.ll tests for regression and has some useful ordering properties. Three DBG_VALUE orders change in sdag-dbgvalue-phi-use-2.ll, happily all originally marked with 'XXX' comments indicating the order was wrong. One 'XXX' remains: IMO this is because of the first conditional block in ProcessSourceNode bypassing various checks by passing in a zero order number. I haven't investigated this matter though.


Diff Detail


Event Timeline

jmorse created this revision.Jan 24 2019, 8:49 AM
aprantl added inline comments.Jan 24 2019, 9:35 AM
823 ↗(On Diff #183324)

if BB->begin() == BB->end() doesn't that imply I == BB->begin(), too?

844 ↗(On Diff #183324)

early return instead?

jmorse updated this revision to Diff 183502.Jan 25 2019, 2:45 AM

Refactor EmitNode helper to early-return if no insts inserted

jmorse updated this revision to Diff 183504.Jan 25 2019, 2:54 AM
jmorse marked an inline comment as done.

Simplify condition testing for existence of earlier iterators

jmorse marked an inline comment as done.Jan 25 2019, 2:54 AM
uabelho resigned from this revision.Jan 25 2019, 3:41 AM

I don't know this code at all.

aprantl accepted this revision.Jan 25 2019, 8:49 AM

I think I could convince myself that this change is correct. Thanks!

746 ↗(On Diff #183504)

if (!Order || Seen.count(Order)) {

This revision is now accepted and ready to land.Jan 25 2019, 8:49 AM
This revision was automatically updated to reflect the committed changes.

@jmorse pr40427.ll uses x86-specific bits in the CHECK lines and is causing failures in builds that do not target by default x86 (e.g. )

Would it be possible to add a target triple = "x86_64-unknown-linux-gnu" line in the testcase?


Hi Roger

@jmorse pr40427.ll uses x86-specific bits in the CHECK lines and is causing failures in builds that do not target by default x86 (e.g. )

Hopefully fixed in r352460, sorry for the bother and thanks for the heads-up. I'd incorrectly assumed being in the X86 directory was a sufficient filter.

Curiously I didn't get a buildbot email related to this, but that's probably because they were down for much of yesterday.