LiveIntervalAnalysis: Support moving of subregister defs in handleMove

[MatzeB]

Summary

If two definitions write to independent subregisters then they can be
put in any order. LiveIntervalAnalysis::handleMove() did not support
this previously because it looks like moving a definition of a vreg past
another one.

This is a modified version of a patch proposed (two years ago) by
Vincent Lejeune! This version does not touch the read-undef flags and is
extended for the case of moving a subregister def behind all uses - this
can happen for subregister defs that are completely unused.

[MatzeB]

Thanks for your feedback in D9069 Tom. Your testcase exposed an issue when moving a killing use across (an unrelated) subregister def. This updated version passes your testcase.

[MatzeB]

Another revision which fixes some regressions introduced by the previous revision here.

[qcolombet]

Hi Matthias,

You mentioned that you found and fixed several regressions. Yet, this patch does not have any test cases added. Am I correctly assuming this is because the related test cases are already in the repository?
If not, could you please add them.

You also have a couple of missing period at the end of the comments. I’ve highlighted the first few ones. Please double check.

The patch mostly LGTM, though the comments would benefits from more examples (like you did with “ where I = [xi…”). Please double check the potential overlapping issue I pointed out.

Thanks,
-Quentin

lib/CodeGen/LiveIntervalAnalysis.cpp

1016 ↗

(On Diff #24086)

Period.

1033 ↗

(On Diff #24086)

The phrasing does not make sense.
I am guessing we want something like:
If <…>, check <…>.
This happens <….>.

1033 ↗

(On Diff #24086)

Period.

1041 ↗

(On Diff #24086)

At this point, NewI must be NewIdx, right?
So, this will create an overlap from base index to reg slot, right?

What I am missing?

1077 ↗

(On Diff #24086)

Period.

1089 ↗

(On Diff #24086)

Period.

1096 ↗

(On Diff #24086)

Period.

1099 ↗

(On Diff #24086)

Period.

1099 ↗

(On Diff #24086)

Assert on Next.

1175 ↗

(On Diff #24086)

Period.

1230 ↗

(On Diff #24086)

Period.

1233 ↗

(On Diff #24086)

Period.

1240 ↗

(On Diff #24086)

Period.

[MatzeB]

In D9067#242052, @qcolombet wrote:

Hi Matthias,

You mentioned that you found and fixed several regressions. Yet, this patch does not have any test cases added. Am I correctly assuming this is because the related test cases are already in the repository?
If not, could you please add them.

You also have a couple of missing period at the end of the comments. I’ve highlighted the first few ones. Please double check.

The patch mostly LGTM, though the comments would benefits from more examples (like you did with “ where I = [xi…”). Please double check the potential overlapping issue I pointed out.

Thanks,
-Quentin

This patch was part of a series of 4 related changes (see dependencies). This patch alone does not regress anything but simply introduces new capabilities to handleMove(). All patches combined regressed some existing testcases so no need for additional ones. Anyway I plan to only commit this change after the other 3 patches are reworked.

[MatzeB]

New version which handles a case that was missed in the previous version of the patch (moving a subregister def above unrelated definitions into a lifetime hole).

[atrick]

I'm just commenting here, not blocking the commit.

After staring for a long time, I basically understand what you're fixing.
But I can't review your changes without critiquing the code as a whole--it's too hard to reason about.

First off--and I realize you're not in a position to fix this--the API is not general enough. A repairIntervalsInRange-like API is what's needed. The design should strive to reuse the interval construction logic, rather than stringing together a number of special cases. I imagine scanning an range of instructions rebuilding live intervals along the way, with a filter that ignores "untouched" virtual registers.

Given that we're stuck at present with a string of special cases, it's good that each case to be handled is listed in the function comment. But those comments do not specify precisely the conditions that need to be checked and aren't obvisouly associated with the implementation in the function body. At a particular point in the function, it isn't clear what assumptions are being made.

For example, these are the kind of questions I have when examining handleMoveDown:

• What conditions are associated with cases 1-6. How can we reason about the completeness?

• Where exactly are cases 1-6 handled inside the function body?

• When subregisters are involved how to you know whether the moved instruction is a use or def?

• How do you know the extended live interval should end at RegSlot? What about early clobber, dead register?

The new code that directly updates start, end, valno is too low-level in the context of handleMoveDown.

[axeldavy]

Could we have some update on this patch ?

The fix is very welcome and fixes some crash issues when changing the order of instructions several times.

[MatzeB]

In D9067#323339, @axeldavy wrote:

Could we have some update on this patch ?

The fix is very welcome and fixes some crash issues when changing the order of instructions several times.

This is surprising, I would have expected this patch to only have an effect for http://reviews.llvm.org/D14969. Do you have out-of-tree code which reschedules independent subregister definitions? Apart from that this patch is not accepted yet, I wanted to not apply this until the rest of the subreg scheduling patches is accepted.

[axeldavy]

In D9067#323823, @MatzeB wrote:

This is surprising, I would have expected this patch to only have an effect for http://reviews.llvm.org/D14969. Do you have out-of-tree code which reschedules independent subregister definitions? Apart from that this patch is not accepted yet, I wanted to not apply this until the rest of the subreg scheduling patches is accepted.

Improvements to http://reviews.llvm.org/D11885 needs this patch (see the part "#if 0 // To enable when handleMove fix lands").

Note that this patch doesn't solve all handleMove bug.
I have a part "// Restore old ordering (which prevents a LIS->handleMove bug)." which cannot be removed either with this patch.

[MatzeB]

In D9067#323889, @axeldavy wrote:

In D9067#323823, @MatzeB wrote:

This is surprising, I would have expected this patch to only have an effect for http://reviews.llvm.org/D14969. Do you have out-of-tree code which reschedules independent subregister definitions? Apart from that this patch is not accepted yet, I wanted to not apply this until the rest of the subreg scheduling patches is accepted.

Improvements to http://reviews.llvm.org/D11885 needs this patch (see the part "#if 0 // To enable when handleMove fix lands").

Note that this patch doesn't solve all handleMove bug.
I have a part "// Restore old ordering (which prevents a LIS->handleMove bug)." which cannot be removed either with this patch.

I didn't read all the code in that patch. As long as you respect all the dependencies generated by ScheduleDAGInstructions today you should not run into any situations where handleMove fails, The changes here should only be necessary if subregister definitions become independent of each other. If you still require this patch then we may have an unrelated bug in handleMove(). Would be interesting to see the liveinterval before and for a handleMove() that fails for you.

[axeldavy]

In D9067#324263, @MatzeB wrote:

I didn't read all the code in that patch. As long as you respect all the dependencies generated by ScheduleDAGInstructions today you should not run into any situations where handleMove fails, The changes here should only be necessary if subregister definitions become independent of each other. If you still require this patch then we may have an unrelated bug in handleMove(). Would be interesting to see the liveinterval before and for a handleMove() that fails for you.

The ordering constraints of ScheduleDAGInstructions are definitely preserved.

The scheduler does make blocks of instructions, and does a first schedule such that instructions inside a block are all consecutive (handleMove is used first there).
Then the blocks order is changed, and that results in second handleMove for the final schedule.

The first problem I face is that if before the second handleMove I don't restore the original ordering, then the second handleMove will crash.
With or without your patch, it is needed.

Step 1) Order instructions by blocks
Step 1 bis) Restore old ordering
Step 2) Put final instruction order

That works with and without your patch (bis step required, else it crashes)

The second problem I face is that as improvement to the scheduler, I want it to try other strategies if the previous result was not satisfying enough.
So this becomes:
Step 1) Order instructions by blocks
Step 1 bis) Restore old ordering
Step 2) Order instructions by blocks (different blocks)
Step 2 bis) Restore old ordering
Step 3) Put final instruction order

Without your patch, Step 2 crashes. Your patch makes the whole work.
The bis steps are still required.

[MatzeB]

New revision based on http://reviews.llvm.org/D16379 which hopefully makes the changes here more comprehensible.

[MatzeB]

ping

[MatzeB]

ping2

[qcolombet]

Hi Matthias,

LGTM.
Few nitpicks inlined.

Cheers,
-Quentin

lib/CodeGen/LiveIntervalAnalysis.cpp

1052 ↗

(On Diff #45753)

s/we//

1112 ↗

(On Diff #45753)

Period.

1263 ↗

(On Diff #45753)

Period.

llvm/trunk/lib/CodeGen/LiveIntervalAnalysis.cpp

if (SlotIndex::isEarlierInstr(OldIdxIn->start, OldIdx)) {
         return;
       // Aggressively remove all kill flags from the old kill point.
       // Kill flags shouldn't be used while live intervals exist, they will be
       // reinserted by VirtRegRewriter.
       if (MachineInstr *KillMI = LIS.getInstructionFromIndex(OldIdxIn->end))
         for (MIBundleOperands MO(KillMI); MO.isValid(); ++MO)
           if (MO->isReg() && MO->isUse())
             MO->setIsKill(false);
+
+      // Is there a def before NewIdx which is not OldIdx?
+      LiveRange::iterator Next = std::next(OldIdxIn);
+      if (Next != E && !SlotIndex::isSameInstr(OldIdx, Next->start) &&
+          SlotIndex::isEarlierInstr(Next->start, NewIdx)) {
+        // If we are here then OldIdx was just a use but not a def. We only have
+        // to ensure liveness extends to NewIdx.
+        LiveRange::iterator NewIdxIn =
+          LR.advanceTo(Next, NewIdx.getBaseIndex());
+        // Extend the segment before NewIdx if necessary.
+        if (NewIdxIn == E ||
+            !SlotIndex::isEarlierInstr(NewIdxIn->start, NewIdx)) {
+          LiveRange::iterator Prev = std::prev(NewIdxIn);
+          Prev->end = NewIdx.getRegSlot();
+        }
+        return;
+      }
+
       // Adjust OldIdxIn->end to reach NewIdx. This may temporarily make LR
       // invalid by overlapping ranges.
       bool isKill = SlotIndex::isSameInstr(OldIdx, OldIdxIn->end);
       OldIdxIn->end = NewIdx.getRegSlot(OldIdxIn->end.isEarlyClobber());
       // If this was not a kill, then there was no def and we're done.
       if (!isKill)
         return;
 
       // Did we have a Def at OldIdx?
-      OldIdxOut = std::next(OldIdxIn);
+      OldIdxOut = Next;
       if (OldIdxOut == E || !SlotIndex::isSameInstr(OldIdx, OldIdxOut->start))
         return;
     } else {
       OldIdxOut = OldIdxIn;
     }
 
     // If we are here then there is a Definition at OldIdx. OldIdxOut points
     // to the segment starting there.
     assert(OldIdxOut != E && SlotIndex::isSameInstr(OldIdx, OldIdxOut->start) &&
            "No def?");
     VNInfo *OldIdxVNI = OldIdxOut->valno;
     assert(OldIdxVNI->def == OldIdxOut->start && "Inconsistent def");
 
     // If the defined value extends beyond NewIdx, just move the beginning
     // of the segment to NewIdx.
     SlotIndex NewIdxDef = NewIdx.getRegSlot(OldIdxOut->start.isEarlyClobber());
     if (SlotIndex::isEarlierInstr(NewIdxDef, OldIdxOut->end)) {
       OldIdxVNI->def = NewIdxDef;
       OldIdxOut->start = OldIdxVNI->def;
       return;
     }
 
     // If we are here then we have a Definition at OldIdx which ends before
-    // NewIdx. Moving across unrelated defs is not allowed; That means we either
-    // had a dead-def at OldIdx or the OldIdxOut segment ends at NewIdx.
-    assert((OldIdxOut->end == OldIdx.getDeadSlot() ||
-            SlotIndex::isSameInstr(OldIdxOut->end, NewIdxDef)) &&
-            "Cannot move def below kill");
+    // NewIdx.
+
     // Is there an existing Def at NewIdx?
     LiveRange::iterator AfterNewIdx
       = LR.advanceTo(OldIdxOut, NewIdx.getRegSlot());
+    bool OldIdxDefIsDead = OldIdxOut->end.isDead();
+    if (!OldIdxDefIsDead &&
+        SlotIndex::isEarlierInstr(OldIdxOut->end, NewIdxDef)) {
+      // OldIdx is not a dead def, and NewIdxDef is inside a new interval.
+      VNInfo *DefVNI;
+      if (OldIdxOut != LR.begin() &&
+          !SlotIndex::isEarlierInstr(std::prev(OldIdxOut)->end,
+                                     OldIdxOut->start)) {
+        // There is no gap between OldIdxOut and its predecessor anymore,
+        // merge them
+        LiveRange::iterator IPrev = std::prev(OldIdxOut);
+        DefVNI = OldIdxVNI;
+        IPrev->end = OldIdxOut->end;
+      } else {
+        // The value is live in to OldIdx
+        LiveRange::iterator INext = std::next(OldIdxOut);
+        assert(INext != E && "Must have following segment");
+        // We merge OldIdxOut and its successor. As we're dealing with subreg
+        // reordering, there is always a successor to OldIdxOut in the same BB
+        // We don't need INext->valno anymore and will reuse for the new segment
+        // we create later.
+        DefVNI = INext->valno;
+        INext->start = OldIdxOut->end;
+        INext->valno = OldIdxVNI;
+        INext->valno->def = INext->start;
+      }
+      // If NewIdx is behind the last segment, extend that and append a new one.
+      if (AfterNewIdx == E) {
+        // OldIdxOut is undef at this point, Slide (OldIdxOut;AfterNewIdx] up
+        // one position.
+        //    |-  ?/OldIdxOut -| |- X0 -| ... |- Xn -| end
+        // => |- X0/OldIdxOut -| ... |- Xn -| |- undef/NewS -| end
+        std::copy(std::next(OldIdxOut), E, OldIdxOut);
+        // The last segment is undefined now, reuse it for a dead def.
+        LiveRange::iterator NewSegment = std::prev(E);
+        *NewSegment = LiveRange::Segment(NewIdxDef, NewIdxDef.getDeadSlot(),
+                                         DefVNI);
+        DefVNI->def = NewIdxDef;
+
+        LiveRange::iterator Prev = std::prev(NewSegment);
+        Prev->end = NewIdxDef;
+      } else {
+        // OldIdxOut is undef at this point, Slide (OldIdxOut;AfterNewIdx] up
+        // one position.
+        //    |-  ?/OldIdxOut -| |- X0 -| ... |- Xn/AfterNewIdx -| |- Next -|
+        // => |- X0/OldIdxOut -| ... |- Xn -| |- Xn/AfterNewIdx -| |- Next -|
+        std::copy(std::next(OldIdxOut), std::next(AfterNewIdx), OldIdxOut);
+        LiveRange::iterator Prev = std::prev(AfterNewIdx);
+        // We have two cases:
+        if (SlotIndex::isEarlierInstr(Prev->start, NewIdxDef)) {
+          // Case 1: NewIdx is inside a liverange. Split this liverange at
+          // NewIdxDef into the segment "Prev" followed by "NewSegment".
+          LiveRange::iterator NewSegment = AfterNewIdx;
+          *NewSegment = LiveRange::Segment(NewIdxDef, Prev->end, Prev->valno);
+          Prev->valno->def = NewIdxDef;
+
+          *Prev = LiveRange::Segment(Prev->start, NewIdxDef, DefVNI);
+          DefVNI->def = Prev->start;
+        } else {
+          // Case 2: NewIdx is in a lifetime hole. Keep AfterNewIdx as is and
+          // turn Prev into a segment from NewIdx to AfterNewIdx->start.
+          *Prev = LiveRange::Segment(NewIdxDef, AfterNewIdx->start, DefVNI);
+          DefVNI->def = NewIdxDef;
+          assert(DefVNI != AfterNewIdx->valno);
+        }
+      }
+      return;
+    }
+
     if (AfterNewIdx != E &&
         SlotIndex::isSameInstr(AfterNewIdx->start, NewIdxDef)) {
       // There is an existing def at NewIdx. The def at OldIdx is coalesced into
       // that value.
       assert(AfterNewIdx->valno != OldIdxVNI && "Multiple defs of value?");
       LR.removeValNo(OldIdxVNI);
     } else {
       // There was no existing def at NewIdx. We need to create a dead def
if (SlotIndex::isEarlierInstr(OldIdxIn->start, OldIdx)) {
       // If the live-in value isn't killed here, then we have no Def at
       // OldIdx, moreover the value must be live at NewIdx so there is nothing
       // to do.
       bool isKill = SlotIndex::isSameInstr(OldIdx, OldIdxIn->end);
       if (!isKill)
         return;
 
       // At this point we have to move OldIdxIn->end back to the nearest
-      // previous use but no further than NewIdx. Moreover OldIdx is a Def then
-      // we cannot have any intermediate uses or the move would be illegal.
+      // previous use or (dead-)def but no further than NewIdx.
+      SlotIndex DefBeforeOldIdx = std::max(OldIdxIn->start.getDeadSlot(),
+                                           NewIdx.getRegSlot());
+      OldIdxIn->end = findLastUseBefore(DefBeforeOldIdx, Reg, LaneMask);
 
+      // Did we have a Def at OldIdx? If not we are done now.
       OldIdxOut = std::next(OldIdxIn);
-      // Did we have a Def at OldIdx?
-      if (OldIdxOut == E || !SlotIndex::isSameInstr(OldIdx, OldIdxOut->start)) {
-        // No def, search for the nearest previous use.
-        // This can never be an early clobber kill since there is no def.
-        OldIdxIn->end = findLastUseBefore(Reg, LaneMask).getRegSlot();
-        // We are done if there is no def at OldIdx.
+      if (OldIdxOut == E || !SlotIndex::isSameInstr(OldIdx, OldIdxOut->start))
         return;
-      } else {
-        // There can't have been any intermediate uses or defs, so move
-        // OldIdxIn->end to NewIdx.
-        OldIdxIn->end = NewIdx.getRegSlot(OldIdxIn->end.isEarlyClobber());
-      }
-    } else {
       OldIdxOut = OldIdxIn;
+      OldIdxIn = OldIdxOut != LR.begin() ? std::prev(OldIdxOut) : E;
     }
 
     // If we are here then there is a Definition at OldIdx. OldIdxOut points
     // to the segment starting there.
     assert(OldIdxOut != E && SlotIndex::isSameInstr(OldIdx, OldIdxOut->start) &&
            "No def?");
     VNInfo *OldIdxVNI = OldIdxOut->valno;
     assert(OldIdxVNI->def == OldIdxOut->start && "Inconsistent def");
if (SlotIndex::isSameInstr(NewIdxOut->start, NewIdx)) {
       } else {
         // Simply remove the dead def at OldIdx.
         LR.removeValNo(OldIdxVNI);
       }
     } else {
       // Previously nothing was live after NewIdx, so all we have to do now is
       // move the begin of OldIdxOut to NewIdx.
       if (!OldIdxDefIsDead) {
+        // Do we have any intermediate Defs between OldIdx and NewIdx?
+        if (OldIdxIn != E &&
+            SlotIndex::isEarlierInstr(NewIdxDef, OldIdxIn->start)) {
+          // OldIdx is not a dead def and NewIdx is before predecessor start
+          LiveRange::iterator NewIdxIn = NewIdxOut;
+          assert(NewIdxIn == LR.find(NewIdx.getBaseIndex()));
+          const SlotIndex SplitPos = NewIdxDef;
+
+          // Merge the OldIdxIn and OldIdxOut segments into OldIdxOut.
+          *OldIdxOut = LiveRange::Segment(OldIdxIn->start, OldIdxOut->end,
+                                          OldIdxIn->valno);
+          // OldIdxIn and OldIdxVNI are now undef and can be overridden.
+          // We Slide [NewIdxIn, OldIdxIn) down one position.
+          //    |- X0/NewIdxIn -| ... |- Xn-1 -||- Xn/OldIdxIn -||- OldIdxOut -|
+          // => |- undef/NexIdxIn -| |- X0 -| ... |- Xn-1 -| |- Xn/OldIdxOut -|
+          std::copy_backward(NewIdxIn, OldIdxIn, OldIdxOut);
+          // NewIdxIn is now considered undef so we can reuse it for the moved
+          // value.
+          LiveRange::iterator NewSegment = NewIdxIn;
+          LiveRange::iterator Next = std::next(NewSegment);
+          NewSegment->valno = OldIdxVNI;
+          if (SlotIndex::isEarlierInstr(Next->start, NewIdx)) {
+            // There is no gap between NewSegment and its predecessor
+            *NewSegment = LiveRange::Segment(Next->start, SplitPos,
+                                             NewSegment->valno);
+            NewSegment->valno->def = Next->start;
+
+            *Next = LiveRange::Segment(SplitPos, Next->end, Next->valno);
+            Next->valno->def = SplitPos;
+          } else {
+            // There is a gap between NewSegment and its predecessor
+            // Value become live in
+            *NewSegment = LiveRange::Segment(SplitPos, Next->start,
+                                             NewSegment->valno);
+            NewSegment->valno->def = SplitPos;
+          }
+        } else {
           // Leave the end point of a live def.
-        OldIdxVNI->def = NewIdxDef;
           OldIdxOut->start = NewIdxDef;
+          OldIdxVNI->def = NewIdxDef;
+          if (OldIdxIn != E && SlotIndex::isEarlierInstr(NewIdx, OldIdxIn->end))
+            OldIdxIn->end = NewIdx.getRegSlot();
+        }
       } else {
         // OldIdxVNI is a dead def. It may have been moved across other values
         // in LR, so move OldIdxOut up to NewIdxOut. Slide [NewIdxOut;OldIdxOut)
         // down one position.
         //    |- X0/NewIdxOut -| ... |- Xn-1 -| |- Xn/OldIdxOut -| |- next - |
         // => |- undef/NewIdxOut -| |- X0 -| ... |- Xn-1 -| |- next -|
         std::copy_backward(NewIdxOut, OldIdxOut, std::next(OldIdxOut));
         // OldIdxVNI can be reused now to build a new dead def segment.
assert((RI == LIS.RegMaskSlots.begin() ||
             SlotIndex::isEarlierInstr(*std::prev(RI), *RI)) &&
            "Cannot move regmask instruction above another call");
     assert((std::next(RI) == LIS.RegMaskSlots.end() ||
             SlotIndex::isEarlierInstr(*RI, *std::next(RI))) &&
            "Cannot move regmask instruction below another call");
   }
 
   // Return the last use of reg between NewIdx and OldIdx.
-  SlotIndex findLastUseBefore(unsigned Reg, LaneBitmask LaneMask) {
-
+  SlotIndex findLastUseBefore(SlotIndex Before, unsigned Reg,
+                              LaneBitmask LaneMask) {
     if (TargetRegisterInfo::isVirtualRegister(Reg)) {
-      SlotIndex LastUse = NewIdx;
+      SlotIndex LastUse = Before;
       for (MachineOperand &MO : MRI.use_nodbg_operands(Reg)) {
         unsigned SubReg = MO.getSubReg();
         if (SubReg != 0 && LaneMask != 0
             && (TRI.getSubRegIndexLaneMask(SubReg) & LaneMask) == 0)
           continue;
 
         const MachineInstr *MI = MO.getParent();
         SlotIndex InstSlot = LIS.getSlotIndexes()->getInstructionIndex(MI);
         if (InstSlot > LastUse && InstSlot < OldIdx)
-          LastUse = InstSlot;
+          LastUse = InstSlot.getRegSlot();
       }
       return LastUse;
     }
 
     // This is a regunit interval, so scanning the use list could be very
     // expensive. Scan upwards from OldIdx instead.
-    assert(NewIdx < OldIdx && "Expected upwards move");
+    assert(Before < OldIdx && "Expected upwards move");
     SlotIndexes *Indexes = LIS.getSlotIndexes();
-    MachineBasicBlock *MBB = Indexes->getMBBFromIndex(NewIdx);
+    MachineBasicBlock *MBB = Indexes->getMBBFromIndex(Before);
 
     // OldIdx may not correspond to an instruction any longer, so set MII to
     // point to the next instruction after OldIdx, or MBB->end().
     MachineBasicBlock::iterator MII = MBB->end();
     if (MachineInstr *MI = Indexes->getInstructionFromIndex(
                            Indexes->getNextNonNullIndex(OldIdx)))
       if (MI->getParent() == MBB)
         MII = MI;
 
     MachineBasicBlock::iterator Begin = MBB->begin();
     while (MII != Begin) {
       if ((--MII)->isDebugValue())
         continue;
       SlotIndex Idx = Indexes->getInstructionIndex(MII);
 
-      // Stop searching when NewIdx is reached.
-      if (!SlotIndex::isEarlierInstr(NewIdx, Idx))
-        return NewIdx;
+      // Stop searching when Before is reached.
+      if (!SlotIndex::isEarlierInstr(Before, Idx))
+        return Before;
 
       // Check if MII uses Reg.
       for (MIBundleOperands MO(MII); MO.isValid(); ++MO)
         if (MO->isReg() &&
             TargetRegisterInfo::isPhysicalRegister(MO->getReg()) &&
             TRI.hasRegUnit(MO->getReg(), Reg))
-          return Idx;
+          return Idx.getRegSlot();
     }
-    // Didn't reach NewIdx. It must be the first instruction in the block.
-    return NewIdx;
+    // Didn't reach Before. It must be the first instruction in the block.
+    return Before;
   }
 };
 
 void LiveIntervals::handleMove(MachineInstr* MI, bool UpdateFlags) {
   assert(!MI->isBundled() && "Can't handle bundled instructions yet.");
   SlotIndex OldIndex = Indexes->getInstructionIndex(MI);
   Indexes->removeMachineInstrFromMaps(MI);
   SlotIndex NewIndex = Indexes->insertMachineInstrInMaps(MI);