[PATCH] D58453: [DebugInfo][CGP] Limit placeDbgValues movement of dbg.value intrinsics

Wed Feb 20 08:23:11 PST 2019

jmorse created this revision.
jmorse added reviewers: aprantl, bjope, vsk.
Herald added subscribers: llvm-commits, jdoerfert, sunfish, aheejin, dschuff, jholewinski.
Herald added a project: LLVM.

This patch limits the activity of CodeGenPrepare::placeDbgValues, which originally helped preserve variable debug locations by placing them near Value definitions. Unfortunately, this now causes more problems than it solves -- see PRs 31878, 35194, 38754. One example problem is that conditional variable assignments can be hoisted and appear to debuggers unconditionally. This patch only allows dbg.value movement when the dbg.value would otherwise be a use-before-def: this is temporarily necessary because the rest of CodeGenPrepare makes no effort to put dbg.values in the right place. Eventually the whole placeDbgValues function should be deleted.

Here are some statistics from a clang-3.4 build with various options twiddled. Note that these were built with the other patches I've uploaded (which I'll add as dependencies) that soften the blow a bit. 'normal' means placeDbgValues is enabled, 'limited' is with this patch, 'deleted' is placeDbgValues completely removed.

  normal placeDbgValues:  77.6% variable locations, 45.9% scope bytes
  limited placeDbgValues: 76.1% variable locations, 46.4% scope bytes
  deleted placeDbgValues: 75.8% variable locations, 45.8% scope bytes

Clearly there's a big decrease (1.8%) of variable coverage happening, and temporarily allowing non-dominated dbg.values to be moved doesn't make a large difference. I've now reached the point where I can't find other (CodeGen) bugs that make a serious impact on these statistics. My current belief, based on variable locations I've traced, is:

- That coverage drop is almost entirely caused by DBG_VALUEs whose operands have gone out of liveness, and
- LLVM previously hasn't paid this liveness penalty, because it's always moved dbg.values to be immediately after definitions.

I believe we *should* IMHO take this coverage hit. We only avoid it right now by cheating and forcefully bringing variables back into liveness, often with stupid effects.

An example of liveness misery is the "XRayThreshold" variable in this [0] function, ignoring the constant assignment on line 150. XRayThreshold gets a value assigned to it by the getAsInteger call on line 154. That function gets inlined and XRayThresholds use (to test early return) gets hoisted, the effect of which is that XRayThreshold has a location for two instructions, neither of which get a line number in the body of [0], and thus the variable is never visible. With placeDbgValues enabled we move the dbg.value for XRayThreshold so that it covers those two instructions -- with placeDbgValues disabled the calculation for XRayThreshold is dead by the time we reach its dbg.value. IMHO the location determined by placeDbgValues isn't useful to the developer, and skews statistics.

Another case I previously reported is [1] -- here the 'var' variable assignment has its dbg.value hoisted by placeDbgValues to above the switch, somewhere it hasn't been declared yet, and its location gets lost by the time it reaches the relevant portion of the switch statement in [1].

In summary, while the variable coverage statistic is going down here, what we don't measure (and possibly can't) is whether the variable locations are defined over instructions in the program that make sense. Which placeDbgValues actively damages IMO.

On to the three changing tests:

- register-variables.ll: The first three deleted DEBUG_VALUE lines are caused by placeDbgValues hoisting locations to places they originally weren't, they're absent with this patch, and other DEBUG_VALUE lines covers the correct behaviour. The deleted 'DEBUG_VALUE' for 'b' drops due to the branch-folding pass (which it previously avoided through placeDbgValues hoisting). The DEBUG_VALUE line for 'c' has sunk back to its original (correct) location.
- NVPTX test: a DBG_VALUE $noreg shifts one instruction down in this test, renumbering all the labels.
- PR37234.ll: another case of hoisting over a block.

This patch effectively invalidates the behaviour of MachineInstr::collectDebugValues because DBG_VALUEs no longer come immediately after the vreg definition they refer to. The patches this one depends on kill off most of the users, but in the WebAssembly backend some target code calls collectDebugValues. I know nothing of WebAssembly -- I suspect this means mailing the backend owner, as it's not clear what their use case is.

I haven't added a test for the absence of this behaviour -- it doesn't strike me as something that anyone would think to re-introduce.

[0] https://github.com/llvm-mirror/llvm/blob/5926c6d6fe85f6623d2d14f473873bc72c72d482/lib/CodeGen/XRayInstrumentation.cpp#L154
[1] https://github.com/llvm-mirror/clang/blob/aa528ab4a083268edddd038d0f251afe792cfa36/lib/CodeGen/CGBlocks.cpp#L1858

Repository:
  rL LLVM

https://reviews.llvm.org/D58453

Files:
  lib/CodeGen/CodeGenPrepare.cpp
  test/DebugInfo/COFF/register-variables.ll
  test/DebugInfo/NVPTX/debug-info.ll
  test/DebugInfo/X86/PR37234.ll

-------------- next part --------------
A non-text attachment was scrubbed...
Name: D58453.187587.patch
Type: text/x-patch
Size: 6377 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20190220/5356d245/attachment.bin>