[Codegen] Have the generic debug skip functions return true on empty.

[iteratee]

Summary

Overall this leads to simpler code even though a few cases are slightly longer.
NFC.

[fhahn]

skipDebugInstructionsForward/Backward take the iterator by value and return the advanced iterator again, because it seemed more in line with C++'s iterator functions (like next and prev), @MatzeB suggested that in https://reviews.llvm.org/D27782#623009 .

In my opinion, it's obvious what the current implementation does by just looking at the call and having the end/beginning checks are in the caller's code, whereas the version proposed in this review is a little bit less obvious and the savings in code size seem quite small.

[iteratee]

In D27963#627614, @fhahn wrote:

skipDebugInstructionsForward/Backward take the iterator by value and return the advanced iterator again, because it seemed more in line with C++'s iterator functions (like next and prev), @MatzeB suggested that in https://reviews.llvm.org/D27782#623009 .

Note that the standard also has ::advance(it, n) which works via side-effect, so the std can't decide one way or the other.

In my opinion, it's obvious what the current implementation does by just looking at the call and having the end/beginning checks are in the caller's code, whereas the version proposed in this review is a little bit less obvious and the savings in code size seem quite small.

Even without returning empty, I prefer the side effect version. It makes nearly all the call sites simpler.

[probinson]

lib/CodeGen/BranchFolding.cpp

1814

Seems like this ought to become

if (skipDebugInstructionsForward(TIB, TIE) ||
    skipDebugInstructionsForward(FIB, FIE))
  break;

lib/CodeGen/IfConversion.cpp

624

These two 'if's can be combined. Or if you prefer to leave them as they are, that's fine too. But put a space between 'if' and paren.

666

As above these could be combined if you like.

[fhahn]

In D27963#628107, @iteratee wrote:

In D27963#627614, @fhahn wrote:

skipDebugInstructionsForward/Backward take the iterator by value and return the advanced iterator again, because it seemed more in line with C++'s iterator functions (like next and prev), @MatzeB suggested that in https://reviews.llvm.org/D27782#623009 .

Note that the standard also has ::advance(it, n) which works via side-effect, so the std can't decide one way or the other.

Oh yes of course! I guess it comes down to personal preference and that's not a call for me to make.

[MatzeB]

I'm not in favor of this. Reference parameters (that actually change a variable) are non obvious and should mostly be avoided in C++.
At least to me it is also not obvious what that return value is when seeing the name skipDebugInstructionsForward.

include/llvm/CodeGen/MachineBasicBlock.h

public:
   MachineBasicBlock::iterator getInitial() { return I; }
 };
 
 /// Increment \p It until it points to a non-debug instruction or to \p End
 /// and return the resulting iterator. This function should only be used
 /// MachineBasicBlock::{iterator, const_iterator, instr_iterator,
 /// const_instr_iterator} and the respective reverse iterators.
 template<typename IterT>
-inline IterT skipDebugInstructionsForward(IterT It, IterT End) {
+inline bool skipDebugInstructionsForward(IterT &It, IterT End) {
   while (It != End && It->isDebugValue())
     It++;
-  return It;
+  return It == End;
 }
 
 /// Decrement \p It until it points to a non-debug instruction or to \p Begin
 /// and return the resulting iterator. This function should only be used
 /// MachineBasicBlock::{iterator, const_iterator, instr_iterator,
 /// const_instr_iterator} and the respective reverse iterators.
 template<class IterT>
-inline IterT skipDebugInstructionsBackward(IterT It, IterT Begin) {
+inline bool skipDebugInstructionsBackward(IterT &It, IterT Begin) {
   while (It != Begin && It->isDebugValue())
     It--;
-  return It;
+  return It == Begin;
 }
 
 } // End llvm namespace
 
 #endif

lib/CodeGen/BranchFolding.cpp

MachineBasicBlock::iterator findHoistingInsertPosAndDeps(MachineBasicBlock *MBB,
 
   if (Uses.empty())
     return Loc;
   if (Loc == MBB->begin())
     return MBB->end();
 
   // The terminator is probably a conditional branch, try not to separate the
   // branch from condition setting instruction.
-  MachineBasicBlock::iterator PI =
-    skipDebugInstructionsBackward(std::prev(Loc), MBB->begin());
+  MachineBasicBlock::iterator PI = std::prev(Loc);
+  skipDebugInstructionsBackward(PI, MBB->begin());
 
   bool IsDef = false;
   for (const MachineOperand &MO : PI->operands()) {
     // If PI has a regmask operand, it is probably a call. Separate away.
     if (MO.isRegMask())
       return Loc;
     if (!MO.isReg() || MO.isUse())
       continue;
bool BranchFolder::HoistCommonCodeInSuccs(MachineBasicBlock *MBB) {
   SmallVector<unsigned, 4> LocalDefs;
   SmallSet<unsigned, 4> LocalDefsSet;
   MachineBasicBlock::iterator TIB = TBB->begin();
   MachineBasicBlock::iterator FIB = FBB->begin();
   MachineBasicBlock::iterator TIE = TBB->end();
   MachineBasicBlock::iterator FIE = FBB->end();
   while (TIB != TIE && FIB != FIE) {
     // Skip dbg_value instructions. These do not count.
-    TIB = skipDebugInstructionsForward(TIB, TIE);
-    FIB = skipDebugInstructionsForward(FIB, FIE);
+    skipDebugInstructionsForward(TIB, TIE);
+    skipDebugInstructionsForward(FIB, FIE);
     if (TIB == TIE || FIB == FIE)
       break;
 
     if (!TIB->isIdenticalTo(*FIB, MachineInstr::CheckKillDead))
       break;
 
     if (TII->isPredicated(*TIB))
       // Hard to reason about register liveness with predicated instruction.
       break;
 

lib/CodeGen/IfConversion.cpp

bool IfConverter::CountDuplicatedInstructions(
     MachineBasicBlock::iterator &TIE,
     MachineBasicBlock::iterator &FIE,
     unsigned &Dups1, unsigned &Dups2,
     MachineBasicBlock &TBB, MachineBasicBlock &FBB,
     bool SkipUnconditionalBranches) const {
 
   while (TIB != TIE && FIB != FIE) {
     // Skip dbg_value instructions. These do not count.
-    TIB = skipDebugInstructionsForward(TIB, TIE);
-    if (TIB == TIE)
+    if(skipDebugInstructionsForward(TIB, TIE))
       break;
-    FIB = skipDebugInstructionsForward(FIB, FIE);
-    if (FIB == FIE)
+    if(skipDebugInstructionsForward(FIB, FIE))
       break;
     if (!TIB->isIdenticalTo(*FIB))
       break;
     // A pred-clobbering instruction in the shared portion prevents
     // if-conversion.
     std::vector<MachineOperand> PredDefs;
     if (TII->DefinesPredicate(*TIB, PredDefs))
       return false;
if (SkipUnconditionalBranches) {
         ++RFIE;
     }
   }
 
   // Count duplicate instructions at the ends of the blocks.
   while (RTIE != RTIB && RFIE != RFIB) {
     // Skip dbg_value instructions. These do not count.
     // Note that these are reverse iterators going forward.
-    RTIE = skipDebugInstructionsForward(RTIE, RTIB);
-    if (RTIE == RTIB)
+    if (skipDebugInstructionsForward(RTIE, RTIB))
       break;
-    RFIE = skipDebugInstructionsForward(RFIE, RFIB);
-    if (RFIE == RFIB)
+    if (skipDebugInstructionsForward(RFIE, RFIB))
       break;
     if (!RTIE->isIdenticalTo(*RFIE))
       break;
     // We have to verify that any branch instructions are the same, and then we
     // don't count them toward the # of duplicate instructions.
     if (!RTIE->isBranch())
       ++Dups2;
     ++RTIE;

lib/CodeGen/MachineBasicBlock.cpp

while (I != B && ((--I)->isTerminator() || I->isDebugValue()))
     ; /*noop */
   while (I != E && !I->isTerminator())
     ++I;
   return I;
 }
 
 MachineBasicBlock::iterator MachineBasicBlock::getFirstNonDebugInstr() {
   // Skip over begin-of-block dbg_value instructions.
-  return skipDebugInstructionsForward(begin(), end());
+  iterator I = begin();
+  skipDebugInstructionsForward(I, end());
+  return I;
 }
 
 MachineBasicBlock::iterator MachineBasicBlock::getLastNonDebugInstr() {
   // Skip over end-of-block dbg_value instructions.
   instr_iterator B = instr_begin(), I = instr_end();
   while (I != B) {
     --I;
     // Return instruction that starts a bundle.
bool MachineBasicBlock::CorrectExtraCFGEdges(MachineBasicBlock *DestA,
   return Changed;
 }
 
 /// Find the next valid DebugLoc starting at MBBI, skipping any DBG_VALUE
 /// instructions.  Return UnknownLoc if there is none.
 DebugLoc
 MachineBasicBlock::findDebugLoc(instr_iterator MBBI) {
   // Skip debug declarations, we don't want a DebugLoc from them.
-  MBBI = skipDebugInstructionsForward(MBBI, instr_end());
-  if (MBBI != instr_end())
-    return MBBI->getDebugLoc();
+  if (skipDebugInstructionsForward(MBBI, instr_end()))
     return {};
+  return MBBI->getDebugLoc();
 }
 
 /// Return probability of the edge from this block to MBB.
 BranchProbability
 MachineBasicBlock::getSuccProbability(const_succ_iterator Succ) const {
   if (Probs.empty())
     return BranchProbability(1, succ_size());
 

lib/CodeGen/MachineCSE.cpp

 
 bool
 MachineCSE::isPhysDefTriviallyDead(unsigned Reg,
                                    MachineBasicBlock::const_iterator I,
                                    MachineBasicBlock::const_iterator E) const {
   unsigned LookAheadLeft = LookAheadLimit;
   while (LookAheadLeft) {
     // Skip over dbg_value's.
-    I = skipDebugInstructionsForward(I, E);
+    skipDebugInstructionsForward(I, E);
 
     if (I == E)
       // Reached end of block, register is obviously dead.
       return true;
 
     bool SeenDef = false;
     for (const MachineOperand &MO : I->operands()) {
       if (MO.isRegMask() && MO.clobbersPhysReg(Reg))

lib/CodeGen/RegisterPressure.cpp

bool RegPressureTracker::isBottomClosed() const {
   if (RequireIntervals)
     return static_cast<IntervalPressure&>(P).BottomIdx.isValid();
   return (static_cast<RegionPressure&>(P).BottomPos ==
           MachineBasicBlock::const_iterator());
 }
 
 
 SlotIndex RegPressureTracker::getCurrSlot() const {
-  MachineBasicBlock::const_iterator IdxPos =
-    skipDebugInstructionsForward(CurrPos, MBB->end());
-  if (IdxPos == MBB->end())
+  auto IdxPos = CurrPos;
+  if(skipDebugInstructionsForward(IdxPos, MBB->end()))
     return LIS->getMBBEndIdx(MBB);
   return LIS->getInstructionIndex(*IdxPos).getRegSlot();
 }
 
 /// Set the boundary for the top of the region and summarize live ins.
 void RegPressureTracker::closeTop() {
   if (RequireIntervals)
     static_cast<IntervalPressure&>(P).TopIdx = getCurrSlot();
void RegPressureTracker::recedeSkipDebugValues() {
   if (!isBottomClosed())
     closeBottom();
 
   // Open the top of the region using block iterators.
   if (!RequireIntervals && isTopClosed())
     static_cast<RegionPressure&>(P).openTop(CurrPos);
 
   // Find the previous instruction.
-  CurrPos = skipDebugInstructionsBackward(std::prev(CurrPos), MBB->begin());
+  skipDebugInstructionsBackward(--CurrPos, MBB->begin());
 
   SlotIndex SlotIdx;
   if (RequireIntervals)
     SlotIdx = LIS->getInstructionIndex(*CurrPos).getRegSlot();
 
   // Open the top of the region using slot indexes.
   if (RequireIntervals && isTopClosed())
     static_cast<IntervalPressure&>(P).openTop(SlotIdx);
for (const RegisterMaskPair &Def : RegOpers.Defs) {
     LaneBitmask NewMask = PreviousMask | Def.LaneMask;
     increaseRegPressure(Def.RegUnit, PreviousMask, NewMask);
   }
 
   // Boost pressure for all dead defs together.
   bumpDeadDefs(RegOpers.DeadDefs);
 
   // Find the next instruction.
-  CurrPos = skipDebugInstructionsForward(std::next(CurrPos), MBB->end());
+  skipDebugInstructionsForward(++CurrPos, MBB->end());
 }
 
 void RegPressureTracker::advance() {
   const MachineInstr &MI = *CurrPos;
   RegisterOperands RegOpers;
   RegOpers.collect(MI, *TRI, *MRI, TrackLaneMasks, false);
   if (TrackLaneMasks) {
     SlotIndex SlotIdx = getCurrSlot();