CodeGen: Give MachineBasicBlock::reverse_iterator a handle to the current MI

Now that MachineBasicBlock::reverse_instr_iterator knows when it's at
the end (since r281168 and r281170), implement
MachineBasicBlock::reverse_iterator directly on top of an
ilist::reverse_iterator by adding an IsReverse template parameter to
MachineInstrBundleIterator.  This replaces another hard-to-reason-about
use of std::reverse_iterator on list iterators, matching the changes for
ilist::reverse_iterator from r280032 (see the "out of scope" section at
the end of that commit message).  MachineBasicBlock::reverse_iterator
now has a handle to the current node and has obvious invalidation
semantics.

r280032 has a more detailed explanation of how list-style reverse
iterators (invalidated when the pointed-at node is deleted) are
different from vector-style reverse iterators like std::reverse_iterator
(invalidated on every operation).  A great motivating example is this
commit's changes to lib/CodeGen/DeadMachineInstructionElim.cpp.

Note: If your out-of-tree backend deletes instructions while iterating
on a MachineBasicBlock::reverse_iterator or converts between
MachineBasicBlock::iterator and MachineBasicBlock::reverse_iterator,
you'll need to update your code in similar ways to r280032.  The
following table might help:

                  [Old]              ==>             [New]
        delete &*RI, RE = end()                   delete &*RI++
        RI->erase(), RE = end()                   RI++->erase()
      reverse_iterator(I)                 std::prev(I).getReverse()
      reverse_iterator(I)                          ++I.getReverse()
    --reverse_iterator(I)                            I.getReverse()
      reverse_iterator(std::next(I))                 I.getReverse()
                RI.base()                std::prev(RI).getReverse()
                RI.base()                         ++RI.getReverse()
              --RI.base()                           RI.getReverse()
     std::next(RI).base()                           RI.getReverse()

(For more details, have a look at r280032.)

llvm-svn: 281172

include/llvm/CodeGen/MachineBasicBlock.h

@@ -155,9 +155,9 @@ public:
 
   typedef MachineInstrBundleIterator<MachineInstr> iterator;
   typedef MachineInstrBundleIterator<const MachineInstr> const_iterator;
-  typedef std::reverse_iterator<const_iterator>          const_reverse_iterator;
+  typedef MachineInstrBundleIterator<MachineInstr, true> reverse_iterator;
-  typedef std::reverse_iterator<iterator>                      reverse_iterator;
+  typedef MachineInstrBundleIterator<const MachineInstr, true>
-
+      const_reverse_iterator;
 
   unsigned size() const { return (unsigned)Insts.size(); }
   bool empty() const { return Insts.empty(); }
@@ -192,13 +192,15 @@ public:
   const_iterator          begin() const { return instr_begin();  }
   iterator                end  ()       { return instr_end();    }
   const_iterator          end  () const { return instr_end();    }
-  reverse_iterator rbegin() { return reverse_iterator(end()); }
+  reverse_iterator rbegin() {
-  const_reverse_iterator rbegin() const {
+    return reverse_iterator::getAtBundleBegin(instr_rbegin());
-    return const_reverse_iterator(end());
   }
-  reverse_iterator rend() { return reverse_iterator(begin()); }
+  const_reverse_iterator rbegin() const {
+    return const_reverse_iterator::getAtBundleBegin(instr_rbegin());
+  }
+  reverse_iterator rend() { return reverse_iterator(instr_rend()); }
   const_reverse_iterator rend() const {
-    return const_reverse_iterator(begin());
+    return const_reverse_iterator(instr_rend());
   }
 
   /// Support for MachineInstr::getNextNode().

include/llvm/CodeGen/MachineInstrBundleIterator.h

@@ -19,23 +19,92 @@
 
 namespace llvm {
 
-template <class T> struct MachineInstrBundleIteratorTraits {
+template <class T, bool IsReverse> struct MachineInstrBundleIteratorTraits;
+template <class T> struct MachineInstrBundleIteratorTraits<T, false> {
   typedef simple_ilist<T, ilist_sentinel_tracking<true>> list_type;
   typedef typename list_type::iterator instr_iterator;
   typedef typename list_type::iterator nonconst_instr_iterator;
   typedef typename list_type::const_iterator const_instr_iterator;
 };
-template <class T> struct MachineInstrBundleIteratorTraits<const T> {
+template <class T> struct MachineInstrBundleIteratorTraits<T, true> {
+  typedef simple_ilist<T, ilist_sentinel_tracking<true>> list_type;
+  typedef typename list_type::reverse_iterator instr_iterator;
+  typedef typename list_type::reverse_iterator nonconst_instr_iterator;
+  typedef typename list_type::const_reverse_iterator const_instr_iterator;
+};
+template <class T> struct MachineInstrBundleIteratorTraits<const T, false> {
   typedef simple_ilist<T, ilist_sentinel_tracking<true>> list_type;
   typedef typename list_type::const_iterator instr_iterator;
   typedef typename list_type::iterator nonconst_instr_iterator;
   typedef typename list_type::const_iterator const_instr_iterator;
 };
+template <class T> struct MachineInstrBundleIteratorTraits<const T, true> {
+  typedef simple_ilist<T, ilist_sentinel_tracking<true>> list_type;
+  typedef typename list_type::const_reverse_iterator instr_iterator;
+  typedef typename list_type::reverse_iterator nonconst_instr_iterator;
+  typedef typename list_type::const_reverse_iterator const_instr_iterator;
+};
+
+template <bool IsReverse> struct MachineInstrBundleIteratorHelper;
+template <> struct MachineInstrBundleIteratorHelper<false> {
+  /// Get the beginning of the current bundle.
+  template <class Iterator> static Iterator getBundleBegin(Iterator I) {
+    if (!I.isEnd())
+      while (I->isBundledWithPred())
+        --I;
+    return I;
+  }
+
+  /// Get the final node of the current bundle.
+  template <class Iterator> static Iterator getBundleFinal(Iterator I) {
+    if (!I.isEnd())
+      while (I->isBundledWithSucc())
+        ++I;
+    return I;
+  }
+
+  /// Increment forward ilist iterator.
+  template <class Iterator> static void increment(Iterator &I) {
+    I = std::next(getBundleFinal(I));
+  }
+
+  /// Decrement forward ilist iterator.
+  template <class Iterator> static void decrement(Iterator &I) {
+    I = getBundleBegin(std::prev(I));
+  }
+};
+
+template <> struct MachineInstrBundleIteratorHelper<true> {
+  /// Get the beginning of the current bundle.
+  template <class Iterator> static Iterator getBundleBegin(Iterator I) {
+    return MachineInstrBundleIteratorHelper<false>::getBundleBegin(
+               I.getReverse())
+        .getReverse();
+  }
+
+  /// Get the final node of the current bundle.
+  template <class Iterator> static Iterator getBundleFinal(Iterator I) {
+    return MachineInstrBundleIteratorHelper<false>::getBundleFinal(
+               I.getReverse())
+        .getReverse();
+  }
+
+  /// Increment reverse ilist iterator.
+  template <class Iterator> static void increment(Iterator &I) {
+    I = getBundleBegin(std::next(I));
+  }
+
+  /// Decrement reverse ilist iterator.
+  template <class Iterator> static void decrement(Iterator &I) {
+    I = std::prev(getBundleFinal(I));
+  }
+};
 
 /// MachineBasicBlock iterator that automatically skips over MIs that are
 /// inside bundles (i.e. walk top level MIs only).
-template <typename Ty> class MachineInstrBundleIterator {
+template <typename Ty, bool IsReverse = false>
-  typedef MachineInstrBundleIteratorTraits<Ty> Traits;
+class MachineInstrBundleIterator : MachineInstrBundleIteratorHelper<IsReverse> {
+  typedef MachineInstrBundleIteratorTraits<Ty, IsReverse> Traits;
   typedef typename Traits::instr_iterator instr_iterator;
   instr_iterator MII;
 
@@ -53,8 +122,9 @@ private:
   typedef typename Traits::nonconst_instr_iterator nonconst_instr_iterator;
   typedef typename Traits::const_instr_iterator const_instr_iterator;
   typedef MachineInstrBundleIterator<
-      typename nonconst_instr_iterator::value_type>
+      typename nonconst_instr_iterator::value_type, IsReverse>
       nonconst_iterator;
+  typedef MachineInstrBundleIterator<Ty, !IsReverse> reverse_iterator;
 
 public:
   MachineInstrBundleIterator(instr_iterator MI) : MII(MI) {
@@ -77,12 +147,17 @@ public:
   // Template allows conversion from const to nonconst.
   template <class OtherTy>
   MachineInstrBundleIterator(
-      const MachineInstrBundleIterator<OtherTy> &I,
+      const MachineInstrBundleIterator<OtherTy, IsReverse> &I,
       typename std::enable_if<std::is_convertible<OtherTy *, Ty *>::value,
                               void *>::type = nullptr)
       : MII(I.getInstrIterator()) {}
   MachineInstrBundleIterator() : MII(nullptr) {}
 
+  /// Get the bundle iterator for the given instruction's bundle.
+  static MachineInstrBundleIterator getAtBundleBegin(instr_iterator MI) {
+    return MachineInstrBundleIteratorHelper<IsReverse>::getBundleBegin(MI);
+  }
+
   reference operator*() const { return *MII; }
   pointer operator->() const { return &operator*(); }
 
@@ -161,15 +236,11 @@ public:
 
   // Increment and decrement operators...
   MachineInstrBundleIterator &operator--() {
-    do
+    this->decrement(MII);
-      --MII;
-    while (MII->isBundledWithPred());
     return *this;
   }
   MachineInstrBundleIterator &operator++() {
-    while (MII->isBundledWithSucc())
+    this->increment(MII);
-      ++MII;
-    ++MII;
     return *this;
   }
   MachineInstrBundleIterator operator--(int) {
@@ -186,6 +257,8 @@ public:
   instr_iterator getInstrIterator() const { return MII; }
 
   nonconst_iterator getNonConstIterator() const { return MII.getNonConst(); }
+
+  reverse_iterator getReverse() const { return MII.getReverse(); }
 };
 
 } // end namespace llvm

lib/CodeGen/AsmPrinter/DbgValueHistoryCalculator.cpp

@@ -134,7 +134,7 @@ static const MachineInstr *getFirstEpilogueInst(const MachineBasicBlock &MBB) {
   // as the return instruction.
   DebugLoc LastLoc = LastMI->getDebugLoc();
   auto Res = LastMI;
-  for (MachineBasicBlock::const_reverse_iterator I(std::next(LastMI)),
+  for (MachineBasicBlock::const_reverse_iterator I = LastMI.getReverse(),
                                                  E = MBB.rend();
        I != E; ++I) {
     if (I->getDebugLoc() != LastLoc)

lib/CodeGen/DeadMachineInstructionElim.cpp

@@ -122,7 +122,7 @@ bool DeadMachineInstructionElim::runOnMachineFunction(MachineFunction &MF) {
     // liveness as we go.
     for (MachineBasicBlock::reverse_iterator MII = MBB.rbegin(),
          MIE = MBB.rend(); MII != MIE; ) {
-      MachineInstr *MI = &*MII;
+      MachineInstr *MI = &*MII++;
 
       // If the instruction is dead, delete it!
       if (isDead(MI)) {
@@ -133,9 +133,6 @@ bool DeadMachineInstructionElim::runOnMachineFunction(MachineFunction &MF) {
         MI->eraseFromParentAndMarkDBGValuesForRemoval();
         AnyChanges = true;
         ++NumDeletes;
-        MIE = MBB.rend();
-        // MII is now pointing to the next instruction to process,
-        // so don't increment it.
         continue;
       }
 
@@ -169,10 +166,6 @@ bool DeadMachineInstructionElim::runOnMachineFunction(MachineFunction &MF) {
           }
         }
       }
-
-      // We didn't delete the current instruction, so increment MII to
-      // the next one.
-      ++MII;
     }
   }
 

lib/CodeGen/IfConversion.cpp

@@ -1802,8 +1802,7 @@ bool IfConverter::IfConvertDiamondCommon(
   // This is everything used+live in BB2 after the duplicated instructions. We
   // can compute this set by simulating liveness backwards from the end of BB2.
   DontKill.init(TRI);
-  for (const MachineInstr &MI :
+  for (const MachineInstr &MI : make_range(MBB2.rbegin(), ++DI2.getReverse()))
-       make_range(MBB2.rbegin(), MachineBasicBlock::reverse_iterator(DI2)))
     DontKill.stepBackward(MI);
 
   for (const MachineInstr &MI : make_range(MBB1.begin(), DI1)) {

lib/Target/AArch64/AArch64InstrInfo.cpp

@@ -920,9 +920,9 @@ static bool areCFlagsAccessedBetweenInstrs(
     return true;
 
   // From must be above To.
-  assert(std::find_if(MachineBasicBlock::reverse_iterator(To),
+  assert(std::find_if(++To.getReverse(), To->getParent()->rend(),
-                      To->getParent()->rend(), [From](MachineInstr &MI) {
+                      [From](MachineInstr &MI) {
-                        return MachineBasicBlock::iterator(MI) == From;
+                        return MI.getIterator() == From;
                       }) != To->getParent()->rend());
 
   // We iterate backward starting \p To until we hit \p From.

lib/Target/AMDGPU/R600InstrInfo.cpp

@@ -726,7 +726,7 @@ MachineBasicBlock::iterator FindLastAluClause(MachineBasicBlock &MBB) {
       It != E; ++It) {
     if (It->getOpcode() == AMDGPU::CF_ALU ||
         It->getOpcode() == AMDGPU::CF_ALU_PUSH_BEFORE)
-      return std::prev(It.base());
+      return It.getReverse();
   }
   return MBB.end();
 }

lib/Target/ARM/MLxExpansionPass.cpp

@@ -334,18 +334,15 @@ bool MLxExpansion::ExpandFPMLxInstructions(MachineBasicBlock &MBB) {
   unsigned Skip = 0;
   MachineBasicBlock::reverse_iterator MII = MBB.rbegin(), E = MBB.rend();
   while (MII != E) {
-    MachineInstr *MI = &*MII;
+    MachineInstr *MI = &*MII++;
 
-    if (MI->isPosition() || MI->isImplicitDef() || MI->isCopy()) {
+    if (MI->isPosition() || MI->isImplicitDef() || MI->isCopy())
-      ++MII;
       continue;
-    }
 
     const MCInstrDesc &MCID = MI->getDesc();
     if (MI->isBarrier()) {
       clearStack();
       Skip = 0;
-      ++MII;
       continue;
     }
 
@@ -365,13 +362,9 @@ bool MLxExpansion::ExpandFPMLxInstructions(MachineBasicBlock &MBB) {
         pushStack(MI);
       else {
         ExpandFPMLxInstruction(MBB, MI, MulOpc, AddSubOpc, NegAcc, HasLane);
-        E = MBB.rend(); // May have changed if MI was the 1st instruction.
         Changed = true;
-        continue;
       }
     }
-
-    ++MII;
   }
 
   return Changed;

lib/Target/Hexagon/HexagonInstrInfo.cpp

@@ -983,7 +983,7 @@ void HexagonInstrInfo::loadRegFromStackSlot(
 static void getLiveRegsAt(LivePhysRegs &Regs, const MachineInstr &MI) {
   const MachineBasicBlock &B = *MI.getParent();
   Regs.addLiveOuts(B);
-  auto E = MachineBasicBlock::const_reverse_iterator(MI.getIterator());
+  auto E = ++MachineBasicBlock::const_iterator(MI.getIterator()).getReverse();
   for (auto I = B.rbegin(); I != E; ++I)
     Regs.stepBackward(*I);
 }

lib/Target/Mips/MipsDelaySlotFiller.cpp

@@ -242,7 +242,7 @@ namespace {
 
     /// This function searches in the backward direction for an instruction that
     /// can be moved to the delay slot. Returns true on success.
-    bool searchBackward(MachineBasicBlock &MBB, Iter Slot) const;
+    bool searchBackward(MachineBasicBlock &MBB, MachineInstr &Slot) const;
 
     /// This function searches MBB in the forward direction for an instruction
     /// that can be moved to the delay slot. Returns true on success.
@@ -594,7 +594,7 @@ bool Filler::runOnMachineBasicBlock(MachineBasicBlock &MBB) {
 
       if (MipsCompactBranchPolicy.getValue() != CB_Always ||
            !TII->getEquivalentCompactForm(I)) {
-        if (searchBackward(MBB, I)) {
+        if (searchBackward(MBB, *I)) {
           Filled = true;
         } else if (I->isTerminator()) {
           if (searchSuccBBs(MBB, I)) {
@@ -659,8 +659,6 @@ template<typename IterTy>
 bool Filler::searchRange(MachineBasicBlock &MBB, IterTy Begin, IterTy End,
                          RegDefsUses &RegDU, InspectMemInstr& IM, Iter Slot,
                          IterTy &Filler) const {
-  bool IsReverseIter = std::is_convertible<IterTy, ReverseIter>::value;
-
   for (IterTy I = Begin; I != End;) {
     IterTy CurrI = I;
     ++I;
@@ -677,12 +675,6 @@ bool Filler::searchRange(MachineBasicBlock &MBB, IterTy Begin, IterTy End,
 
     if (CurrI->isKill()) {
       CurrI->eraseFromParent();
-
-      // This special case is needed for reverse iterators, because when we
-      // erase an instruction, the iterators are updated to point to the next
-      // instruction.
-      if (IsReverseIter && I != End)
-        I = CurrI;
       continue;
     }
 
@@ -722,7 +714,7 @@ bool Filler::searchRange(MachineBasicBlock &MBB, IterTy Begin, IterTy End,
   return false;
 }
 
-bool Filler::searchBackward(MachineBasicBlock &MBB, Iter Slot) const {
+bool Filler::searchBackward(MachineBasicBlock &MBB, MachineInstr &Slot) const {
   if (DisableBackwardSearch)
     return false;
 
@@ -731,14 +723,15 @@ bool Filler::searchBackward(MachineBasicBlock &MBB, Iter Slot) const {
   MemDefsUses MemDU(Fn->getDataLayout(), &Fn->getFrameInfo());
   ReverseIter Filler;
 
-  RegDU.init(*Slot);
+  RegDU.init(Slot);
 
-  if (!searchRange(MBB, ReverseIter(Slot), MBB.rend(), RegDU, MemDU, Slot,
+  MachineBasicBlock::iterator SlotI = Slot;
+  if (!searchRange(MBB, ++SlotI.getReverse(), MBB.rend(), RegDU, MemDU, Slot,
                    Filler))
     return false;
 
-  MBB.splice(std::next(Slot), &MBB, std::next(Filler).base());
+  MBB.splice(std::next(SlotI), &MBB, Filler.getReverse());
-  MIBundleBuilder(MBB, Slot, std::next(Slot, 2));
+  MIBundleBuilder(MBB, SlotI, std::next(SlotI, 2));
   ++UsefulSlots;
   return true;
 }

lib/Target/Mips/MipsInstrInfo.cpp

@@ -147,14 +147,16 @@ unsigned MipsInstrInfo::InsertBranch(MachineBasicBlock &MBB,
 
 unsigned MipsInstrInfo::RemoveBranch(MachineBasicBlock &MBB) const {
   MachineBasicBlock::reverse_iterator I = MBB.rbegin(), REnd = MBB.rend();
-  MachineBasicBlock::reverse_iterator FirstBr;
   unsigned removed;
 
   // Skip all the debug instructions.
   while (I != REnd && I->isDebugValue())
     ++I;
 
-  FirstBr = I;
+  if (I == REnd)
+    return 0;
+
+  MachineBasicBlock::iterator FirstBr = ++I.getReverse();
 
   // Up to 2 branches are removed.
   // Note that indirect branches are not removed.
@@ -162,7 +164,7 @@ unsigned MipsInstrInfo::RemoveBranch(MachineBasicBlock &MBB) const {
     if (!getAnalyzableBrOpc(I->getOpcode()))
       break;
 
-  MBB.erase(I.base(), FirstBr.base());
+  MBB.erase((--I).getReverse(), FirstBr);
 
   return removed;
 }

lib/Target/Mips/MipsLongBranch.cpp

@@ -157,7 +157,7 @@ void MipsLongBranch::splitMBB(MachineBasicBlock *MBB) {
   MBB->addSuccessor(Tgt);
   MF->insert(std::next(MachineFunction::iterator(MBB)), NewMBB);
 
-  NewMBB->splice(NewMBB->end(), MBB, (++LastBr).base(), MBB->end());
+  NewMBB->splice(NewMBB->end(), MBB, LastBr.getReverse(), MBB->end());
 }
 
 // Fill MBBInfos.
@@ -187,7 +187,7 @@ void MipsLongBranch::initMBBInfo() {
 
     if ((Br != End) && !Br->isIndirectBranch() &&
         (Br->isConditionalBranch() || (Br->isUnconditionalBranch() && IsPIC)))
-      MBBInfos[I].Br = &*(++Br).base();
+      MBBInfos[I].Br = &*Br;
   }
 }
 

lib/Target/X86/X86InstrInfo.cpp

@@ -5349,9 +5349,9 @@ bool X86InstrInfo::optimizeCompareInstr(MachineInstr &CmpInstr, unsigned SrcReg,
   // If the definition is in this basic block, RE points to the definition;
   // otherwise, RE is the rend of the basic block.
   MachineBasicBlock::reverse_iterator
-      RI = MachineBasicBlock::reverse_iterator(I),
+      RI = ++I.getReverse(),
       RE = CmpInstr.getParent() == MI->getParent()
-               ? MachineBasicBlock::reverse_iterator(++Def) /* points to MI */
+               ? Def.getReverse() /* points to MI */
                : CmpInstr.getParent()->rend();
   MachineInstr *Movr0Inst = nullptr;
   for (; RI != RE; ++RI) {
@@ -5497,8 +5497,7 @@ bool X86InstrInfo::optimizeCompareInstr(MachineInstr &CmpInstr, unsigned SrcReg,
   if (Movr0Inst) {
     // Look backwards until we find a def that doesn't use the current EFLAGS.
     Def = Sub;
-    MachineBasicBlock::reverse_iterator
+    MachineBasicBlock::reverse_iterator InsertI = Def.getReverse(),
-      InsertI = MachineBasicBlock::reverse_iterator(++Def),
                                         InsertE = Sub->getParent()->rend();
     for (; InsertI != InsertE; ++InsertI) {
       MachineInstr *Instr = &*InsertI;

unittests/CodeGen/MachineInstrBundleIteratorTest.cpp

@@ -22,25 +22,47 @@ struct MyBundledInstr
 };
 typedef MachineInstrBundleIterator<MyBundledInstr> bundled_iterator;
 typedef MachineInstrBundleIterator<const MyBundledInstr> const_bundled_iterator;
+typedef MachineInstrBundleIterator<MyBundledInstr, true>
+    reverse_bundled_iterator;
+typedef MachineInstrBundleIterator<const MyBundledInstr, true>
+    const_reverse_bundled_iterator;
 
 #ifdef GTEST_HAS_DEATH_TEST
 #ifndef NDEBUG
 TEST(MachineInstrBundleIteratorTest, CheckForBundles) {
   MyBundledInstr MBI;
+  auto I = MBI.getIterator();
+  auto RI = I.getReverse();
 
   // Confirm that MBI is always considered bundled.
   EXPECT_TRUE(MBI.isBundledWithPred());
   EXPECT_TRUE(MBI.isBundledWithSucc());
 
   // Confirm that iterators check in their constructor for bundled iterators.
+  EXPECT_DEATH((void)static_cast<bundled_iterator>(I),
+               "not legal to initialize");
   EXPECT_DEATH((void)static_cast<bundled_iterator>(MBI),
                "not legal to initialize");
   EXPECT_DEATH((void)static_cast<bundled_iterator>(&MBI),
                "not legal to initialize");
+  EXPECT_DEATH((void)static_cast<const_bundled_iterator>(I),
+               "not legal to initialize");
   EXPECT_DEATH((void)static_cast<const_bundled_iterator>(MBI),
                "not legal to initialize");
   EXPECT_DEATH((void)static_cast<const_bundled_iterator>(&MBI),
                "not legal to initialize");
+  EXPECT_DEATH((void)static_cast<reverse_bundled_iterator>(RI),
+               "not legal to initialize");
+  EXPECT_DEATH((void)static_cast<reverse_bundled_iterator>(MBI),
+               "not legal to initialize");
+  EXPECT_DEATH((void)static_cast<reverse_bundled_iterator>(&MBI),
+               "not legal to initialize");
+  EXPECT_DEATH((void)static_cast<const_reverse_bundled_iterator>(RI),
+               "not legal to initialize");
+  EXPECT_DEATH((void)static_cast<const_reverse_bundled_iterator>(MBI),
+               "not legal to initialize");
+  EXPECT_DEATH((void)static_cast<const_reverse_bundled_iterator>(&MBI),
+               "not legal to initialize");
 }
 #endif
 #endif