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

[Jeremy Morse via Phabricator via llvm-commits]

jmorse created this revision.
jmorse added reviewers: rnk, aprantl, vsk, uabelho.
Herald added subscribers: llvm-commits, MatzeB, jholewinski.

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.

[0] https://bugs.llvm.org/show_bug.cgi?id=40427


Repository:
  rL LLVM

https://reviews.llvm.org/D57163

Files:
  lib/CodeGen/SelectionDAG/ScheduleDAGSDNodes.cpp
  test/DebugInfo/NVPTX/debug-info.ll
  test/DebugInfo/X86/pr40427.ll
  test/DebugInfo/X86/sdag-dbgvalue-phi-use-2.ll

-------------- next part --------------
A non-text attachment was scrubbed...
Name: D57163.183324.patch
Type: text/x-patch
Size: 9823 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20190124/b080a829/attachment.bin>