Page MenuHomePhabricator

[DebugInfo] Re-instate scope trimming in LiveDebugVariables

Authored by jmorse on Jan 30 2020, 4:19 AM.



In D62650 / rL362750, Alexey removed some code that trimmed the beginning range of variable locations, as this was accidentally cutting ranges too far. At the time, no-one could think up a good reason for it to continue existing. However, we've since run across an (internal) example of where this trimming is necessary:

  • Have a massive basic block in a global constructor, with lots of functions inlined into it,
  • Somewhat due to PR41583 [0], SelectionDAG places large volumes of DBG_VALUEs at the start of the block, almost all out of their lexical scope,
  • LiveDebugVariables ends up trying to track every variable (including all inlined) for every instruction in the block,

The net effect of which is that a (large) 24,000 instruction block gets expanded to 770,000 instructions, mostly DBG_VALUEs. This ends up swallowing huge amounts of compile time, for variable locations that are all out of scope. With this patch applied to re-instate the trimming, this blow-up does not occur, and variables are only tracked inside their lexical scope.

To address the fault Alexey encountered, I've added a clause that, if a variable location starts at the first instruction of a block, uses the block-start SlotIndex instead of the instructions SlotIndex. Consider the example in the regression test from D62650, where before regalloc in the loop we have:

512B    bb.2.for.body:
        ; predecessors: %bb.0, %bb.2
          successors: %bb.1(0x04000000), %bb.2(0x7c000000); %bb.1(3.12%), %bb.2(96.88%)

          DBG_VALUE somethingorother
544B      STRWroX %38:gpr32, %36:gpr64common, %35:gpr64common, 0, 1, debug-location !50 :: (store 4 into %ir.scevgep); test_debug_val.cpp:16:17

In the faulty behaviour, a DBG_VALUE before the STRWroX would attach itself to the SlotIndex 544B, and then after regalloc we would see:

512B    bb.2.for.body:
        ; predecessors: %bb.0, %bb.2
          successors: %bb.1(0x04000000), %bb.2(0x7c000000); %bb.1(3.12%), %bb.2(96.88%)

520B      %36:gpr64common = MOVaddr target-flags(aarch64-page) @array, target-flags(aarch64-pageoff, aarch64-nc) @array, debug-location !50; test_debug_val.cpp:16:17
524B      %38:gpr32 = MOVi32imm 56, debug-location !50; test_debug_val.cpp:16:17
          DBG_VALUE somethingorother
544B      STRWroX %38:gpr32, %36:gpr64common, %35:gpr64common, 0, 1, debug-location !50 :: (store 4 into %ir.scevgep); test_debug_val.cpp:16:17

The DBG_VALUE would be placed before the STRWroX, and despite being at the start of the block pre-regalloc, would not be afterwards, meaning a debugger stepping into this block would not see this location.

My fix for this moves any variable location pointing at 544B (i.e. the first instruction) to 512B, the block start. That way anything inserted during regalloc is covered by the DBG_VALUE.

I haven't added a test (does LLVM have compile-time performance testing?), my proof that this works is that Alexeys regression test keeps on working. The change to live-debug-variables.ll is because a location list is trimmed to not cover the prologue of a function.


Diff Detail

Event Timeline

jmorse created this revision.Jan 30 2020, 4:19 AM
jmorse marked an inline comment as done.Jan 30 2020, 4:21 AM
jmorse added inline comments.

^^^ this hunk is my addition, everything else here is a revert of D62650

avl added a comment.Jan 30 2020, 7:00 AM

@jmorse this lgtm for me. Thank you for doing it right.

vsk resigned from this revision.Jan 30 2020, 5:17 PM

Seems reasonable to me, but I don’t have much experience in this area. Orlando has looked at this more recently and is probably more qualified to review ;)

We (llvm) used to have bots tracking CTMark performance but they’ve been dead for a while, I think. I run CTMark through llvm-test-suite all the time though and find it quite useful.

jmorse edited reviewers, added: Orlando; removed: vsk.Jan 31 2020, 3:28 AM
Orlando accepted this revision.Jan 31 2020, 6:18 AM

Orlando has looked at this more recently and is probably more qualified to review ;)

Ha I'm not sure about that, but as far as I understand what's going on here this looks good.

This revision is now accepted and ready to land.Jan 31 2020, 6:18 AM
This revision was automatically updated to reflect the committed changes.