[DbgInfo] Attempt to fix bug 37149

[tyb0807]

Summary

Hello,

This is only for demonstrating my idea. I don't know the LLVM structures really well (yet), so my implementation is quite hackish (make SlotIndex::getIndex() public for example). So please tell me:
1, Whether my *idea* and my understanding of the problem are correct,
2, What should be the right way to implement the solution.

Thank you all for your help

[aprantl]

Could you please update the comment explaining what the new check is checking for?

[tyb0807]

Urgh, sorry but ... what new check? The only check is just transforming SmallSet::count() into SmallDenseMap::find() + comparing with SmallDenseMap::end(), so just to check whether the interval start was trimmed to the lexical scope (which is already commented in L1210).

[aprantl]

So IIUC you are remembering the SlotIndex that the interval was trimmed to instead of assuming that it was the one immediately preceding the beginning to the interval?

I think it would help to add a testcase to illustrate what the situation that this is improving on.

lib/CodeGen/LiveDebugVariables.cpp

166

+ , and the SlotIndex they were trimmed to.?

[rob.lougher]

Have you ran the LLVM regression tests? In particular, does the test added in D35953 (test/DebugInfo/X86/live-debug-variables.ll) still work as expected?

There is a pass called LexicalScopes. This computes the lexical scope for a debug location, consisting of a series of instruction ranges. The code added in D35953 made sure the computed intervals were within the debug location's lexical scope. If it wasn't it was trimmed to the lexical scope.

Looking at PR37149, you are saying that the locations are wrong due to the trimming. In this patch you are remembering the original untrimmed start index, and using that instead of the trimmed one. On the surface this appears to (partially) undo D35953. However, I can see why the test added in D35953 may still pass (this is why I want you to confirm whether you ran the tests). D35953 was concerned with the case where a live interval could be incorrectly split during register allocation. Effectively, this change means we use the trimmed intervals during register allocation, but reinstate the untrimmed starts (not the ends) when re-inserting the debug values (the call to emitDebugValues() actually occurs during the Virtual Register Rewriter).

My main concern is ensuring that D35953 still passes. However, I am worried that for the trimmed locations to be wrong, the lexical scopes must also be wrong. I'll be interested to see what the lexical scopes are for your testcase in PR37149.

[rob.lougher]

In D46599#1093244, @rob.lougher wrote:

I'll be interested to see what the lexical scopes are for your testcase in PR37149.

To be clearer, I'd like a reduced testcase, and instructions how to demonstrate the issue. Thanks! This could be used as the basis for a future test.

[tyb0807]

@aprantl Yes that is exactly what I am doing.
@rob.lougher As you pointed out, I was hoping that this change would not change the splitting in RegAlloc, but apparently it is not the case. The test added in D35953 does not pass anymore. Instead of

[0x000000000000001f, 0x000000000000003c): DW_OP_reg3 RBX

we now have

[0x000000000000000e,  0x0000000000000013): DW_OP_reg2 RCX
[0x0000000000000013,  0x000000000000003c): DW_OP_reg3 RBX

I do not understand why though... RegAlloc should not be broken this way :(. I'll update a reduced test case reflecting the PR37149's issue so you can tell me how it should be fixed.

Thanks for the help!

[tyb0807]

Hi,

I've posted a reduced test case in https://bugs.llvm.org/show_bug.cgi?id=37149.

@rob.lougher Btw, I have taken a look at the test added in D35953 (test/DebugInfo/X86/live-debug-variables.ll). We always have 2 DBG_VALUE instructions:

DBG_VALUE debug-use $ecx, debug-use $noreg, !"i4", ...
and
DBG_VALUE $ebx, debug-use $noreg, !"i4", ...

The only difference is just the placement of the second DBG_VALUE in the MIR. So IIUC, we should always expect 2 location entries in the debug_loc section, shouldn't we?

include/llvm/CodeGen/SlotIndexes.h

IndexListEntry* listEntry() const {
       assert(isValid() && "Attempt to compare reserved index.");
 #ifdef EXPENSIVE_CHECKS
       assert(!lie.getPointer()->isPoisoned() &&
              "Attempt to access deleted list-entry.");
 #endif // EXPENSIVE_CHECKS
       return lie.getPointer();
     }
 
-    unsigned getIndex() const {
-      return listEntry()->getIndex() | getSlot();
-    }
-
     /// Returns the slot for this SlotIndex.
     Slot getSlot() const {
       return static_cast<Slot>(lie.getInt());
     }
 
   public:
     enum {
       /// The default distance between instructions as returned by distance().
       /// This may vary as instructions are inserted and removed.
       InstrDist = 4 * Slot_Count
     };
 
     /// Construct an invalid index.
     SlotIndex() = default;
 
     // Construct a new slot index from the given one, and set the slot.
     SlotIndex(const SlotIndex &li, Slot s) : lie(li.listEntry(), unsigned(s)) {
       assert(lie.getPointer() != nullptr &&
              "Attempt to construct index with 0 pointer.");
     }
 
+    unsigned getIndex() const {
+      return listEntry()->getIndex() | getSlot();
+    }
+
     /// Returns true if this is a valid index. Invalid indices do
     /// not point into an index table, and cannot be compared.
     bool isValid() const {
       return lie.getPointer();
     }
 
     /// Return true for a valid index.
     explicit operator bool() const { return isValid(); }

lib/CodeGen/LiveDebugVariables.cpp

class UserValue {
 
   /// Numbered locations referenced by locmap.
   SmallVector<MachineOperand, 4> locations;
 
   /// 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;
+  SmallDenseMap<unsigned, SlotIndex, 2> trimmedDefs;
 
   /// insertDebugValue - Insert a DBG_VALUE into MBB at Idx for LocNo.
   void insertDebugValue(MachineBasicBlock *MBB, SlotIndex StartIdx,
                         SlotIndex StopIdx,
                         DbgValueLocation Loc, bool Spilled, LiveIntervals &LIS,
                         const TargetInstrInfo &TII,
                         const TargetRegisterInfo &TRI);
 
for (const InsnRange &Range : Scope->getRanges()) {
     }
 
     // Advance I so that I.stop() >= RStart, and check for overlap.
     I.advanceTo(RStart);
     if (!I.valid())
       return;
 
     if (I.start() < RStart) {
+      // Remember that this interval was trimmed.
+      trimmedDefs.insert({RStart.getIndex(), I.start()});
       // 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.
     REnd = REnd.getNextIndex();
 
     // Advance I to first interval outside current range.
for (LocMap::const_iterator I = locInts.begin(); I.valid();) {
     SlotIndex Start = I.start();
     SlotIndex Stop = I.stop();
     DbgValueLocation Loc = I.value();
     bool Spilled = !Loc.isUndef() ? SpilledLocations.test(Loc.locNo()) : false;
 
     // 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();
+    auto trimmedDef = trimmedDefs.find(Start.getIndex());
+    if (trimmedDef != trimmedDefs.end())
+      Start = trimmedDef->second;
 
     DEBUG(dbgs() << "\t[" << Start << ';' << Stop << "):" << Loc.locNo());
     MachineFunction::iterator MBB = LIS.getMBBFromIndex(Start)->getIterator();
     SlotIndex MBBEnd = LIS.getMBBEndIdx(&*MBB);
 
     DEBUG(dbgs() << ' ' << printMBBReference(*MBB) << '-' << MBBEnd);
     insertDebugValue(&*MBB, Start, Stop, Loc, Spilled, LIS, TII, TRI);
     // This interval may span multiple basic blocks.