[PATCH] D73691: [DebugInfo] Re-instate scope trimming in LiveDebugVariables

Thu Jan 30 04:21:23 PST 2020

jmorse created this revision.
jmorse added reviewers: aprantl, vsk, rob.lougher, avl.
Herald added subscribers: llvm-commits, javed.absar, hiraditya, kristof.beyls.
Herald added a project: LLVM.

In D62650 <https://reviews.llvm.org/D62650> / rL362750 <https://reviews.llvm.org/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 <https://reviews.llvm.org/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.

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


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D73691

Files:
  llvm/lib/CodeGen/LiveDebugVariables.cpp
  llvm/test/DebugInfo/X86/live-debug-variables.ll


Index: llvm/test/DebugInfo/X86/live-debug-variables.ll
===================================================================

--- llvm/test/DebugInfo/X86/live-debug-variables.ll
+++ llvm/test/DebugInfo/X86/live-debug-variables.ll
@@ -25,7 +25,7 @@
 ; CHECK:      .debug_loc contents:
 ; CHECK-NEXT: 0x00000000:
 ;   We currently emit an entry for the function prologue, too, which could be optimized away.
-; CHECK:              (0x0000000000000010, 0x0000000000000072): DW_OP_reg3 RBX
+; CHECK:              (0x0000000000000018, 0x0000000000000072): DW_OP_reg3 RBX
 ;   We should only have one entry inside the function.
 ; CHECK-NOT: :
 
Index: llvm/lib/CodeGen/LiveDebugVariables.cpp
===================================================================
--- llvm/lib/CodeGen/LiveDebugVariables.cpp
+++ llvm/lib/CodeGen/LiveDebugVariables.cpp
@@ -166,6 +166,10 @@
   /// Map of slot indices where this value is live.
   LocMap locInts;
 
+  /// Set of interval start indexes that have been trimmed to the
+  /// lexical scope.
+  SmallSet<SlotIndex, 2> trimmedDefs;
+
   /// Insert a DBG_VALUE into MBB at Idx for LocNo.
   void insertDebugValue(MachineBasicBlock *MBB, SlotIndex StartIdx,
                         SlotIndex StopIdx, DbgValueLocation Loc, bool Spilled,
@@ -910,6 +914,11 @@
     SlotIndex RStart = LIS.getInstructionIndex(*Range.first);
     SlotIndex REnd = LIS.getInstructionIndex(*Range.second);
 
+    // Variable locations at the first instruction of a block should be
+    // based on the block's SlotIndex, not the first instruction's index.
+    if (Range.first == Range.first->getParent()->begin())
+      RStart = LIS.getSlotIndexes()->getIndexBefore(*Range.first);
+
     // At the start of each iteration I has been advanced so that
     // I.stop() >= PrevEnd. Check for overlap.
     if (PrevEnd && I.start() < PrevEnd) {
@@ -922,7 +931,8 @@
       ++I;
 
       // If the interval also overlaps the start of the "next" (i.e.
-      // current) range create a new interval for the remainder
+      // current) range create a new interval for the remainder (which
+      // may be further trimmed).
       if (RStart < IStop)
         I.insert(RStart, IStop, Loc);
     }
@@ -932,6 +942,13 @@
     if (!I.valid())
       return;
 
+    if (I.start() < RStart) {
+      // Interval start overlaps range - trim to the scope range.
+      I.setStartUnchecked(RStart);
+      // Remember that this interval was trimmed.
+      trimmedDefs.insert(RStart);
+    }
+
     // The end of a lexical scope range is the last instruction in the
     // range. To convert to an interval we need the index of the
     // instruction after it.
@@ -1345,6 +1362,12 @@
     bool Spilled = SpillIt != SpillOffsets.end();
     unsigned SpillOffset = Spilled ? SpillIt->second : 0;
 
+    // If the interval start was trimmed to the lexical scope insert the
+    // DBG_VALUE at the previous index (otherwise it appears after the
+    // first instruction in the range).
+    if (trimmedDefs.count(Start))
+      Start = Start.getPrevIndex();
+
     LLVM_DEBUG(dbgs() << "\t[" << Start << ';' << Stop << "):" << Loc.locNo());
     MachineFunction::iterator MBB = LIS.getMBBFromIndex(Start)->getIterator();
     SlotIndex MBBEnd = LIS.getMBBEndIdx(&*MBB);


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D73691.241397.patch
Type: text/x-patch
Size: 3261 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20200130/c2e2280b/attachment-0001.bin>
