[CSSPGO] Flip SkipPseudoOp to true for MIR APIs.

[hoy]

Summary

Flipping the default value of SkipPseudoOp to true for those MIR APIs to favor maximum performance. Note that certain spots like branch folding and MIR if-conversion is are disabled for better counts quality. For these two optimizations, this is a no-diff change.

The counts quality with SPEC2017 before/after this change is unchanged.

[wmi]

llvm/include/llvm/CodeGen/MachineBasicBlock.h

683–684

TODO can be removed now. It maybe better to list the optimizations like branch folding and if-conversion here which set SkipPseudoOp to false as a reference.

[hoy]

llvm/include/llvm/CodeGen/MachineBasicBlock.h

683–684

Good point. Comment changed.

[hoy]

Addressing Wei's comment.

[wmi]

LGTM, thanks.

[chill]

Apologies if you're already aware.

It appears with this change an expensive checks buildbot started to fail: https://lab.llvm.org/buildbot/#/builders/16/builds/9542

[hoy]

In D100332#2701572, @chill wrote:

Apologies if you're already aware.

It appears with this change an expensive checks buildbot started to fail: https://lab.llvm.org/buildbot/#/builders/16/builds/9542

Thanks for letting me know. The failures should be fixed by D100334.

[thakis]

In D100332#2701895, @hoy wrote:

In D100332#2701572, @chill wrote:

Apologies if you're already aware.

It appears with this change an expensive checks buildbot started to fail: https://lab.llvm.org/buildbot/#/builders/16/builds/9542

Thanks for letting me know. The failures should be fixed by D100334.

Looks like it's still failing at today's trunk even with that change: https://lab.llvm.org/buildbot/#/builders/16/builds/9678

[hoy]

In D100332#2705180, @thakis wrote:

In D100332#2701895, @hoy wrote:

In D100332#2701572, @chill wrote:

Apologies if you're already aware.

It appears with this change an expensive checks buildbot started to fail: https://lab.llvm.org/buildbot/#/builders/16/builds/9542

Thanks for letting me know. The failures should be fixed by D100334.

Looks like it's still failing at today's trunk even with that change: https://lab.llvm.org/buildbot/#/builders/16/builds/9678

I see. It's failing with -DLLVM_ENABLE_EXPENSIVE_CHECKS=ON. I'm taking a look. Thanks.

[hoy]

In D100332#2705447, @hoy wrote:

In D100332#2705180, @thakis wrote:

In D100332#2701895, @hoy wrote:

In D100332#2701572, @chill wrote:

Apologies if you're already aware.

It appears with this change an expensive checks buildbot started to fail: https://lab.llvm.org/buildbot/#/builders/16/builds/9542

Thanks for letting me know. The failures should be fixed by D100334.

Looks like it's still failing at today's trunk even with that change: https://lab.llvm.org/buildbot/#/builders/16/builds/9678

I see. It's failing with -DLLVM_ENABLE_EXPENSIVE_CHECKS=ON. I'm taking a look. Thanks.

I just pushed in a fix: https://reviews.llvm.org/rGb6db6f5530d2 .

llvm/include/llvm/CodeGen/MachineBasicBlock.h

#endif
   /// Return the first instruction in MBB after I that is not a PHI or a label.
   /// This is the correct point to insert lowered copies at the beginning of a
   /// basic block that must be before any debugging information.
   iterator SkipPHIsAndLabels(iterator I);
 
   /// Return the first instruction in MBB after I that is not a PHI, label or
   /// debug.  This is the correct point to insert copies at the beginning of a
   /// basic block.
-  iterator SkipPHIsLabelsAndDebug(iterator I);
+  iterator SkipPHIsLabelsAndDebug(iterator I, bool SkipPseudoOp = true);
 
   /// Returns an iterator to the first terminator instruction of this basic
   /// block. If a terminator does not exist, it returns end().
   iterator getFirstTerminator();
   const_iterator getFirstTerminator() const {
     return const_cast<MachineBasicBlock *>(this)->getFirstTerminator();
   }
 
   /// Same getFirstTerminator but it ignores bundles and return an
   /// instr_iterator instead.
   instr_iterator getFirstInstrTerminator();
 
   /// Returns an iterator to the first non-debug instruction in the basic block,
   /// or end(). Skip any pseudo probe operation if \c SkipPseudoOp is true.
   /// Pseudo probes are like debug instructions which do not turn into real
   /// machine code. We try to use the function to skip both debug instructions
   /// and pseudo probe operations to avoid API proliferation. This should work
   /// most of the time when considering optimizing the rest of code in the
   /// block, except for certain cases where pseudo probes are designed to block
   /// the optimizations. For example, code merge like optimizations are supposed
   /// to be blocked by pseudo probes for better AutoFDO profile quality.
   /// Therefore, they should be considered as a valid instruction when this
   /// function is called in a context of such optimizations. On the other hand,
   /// \c SkipPseudoOp should be true when it's used in optimizations that
-  /// unlikely hurt profile quality, e.g., without block merging.
-  /// TODO: flip the default value of \c SkipPseudoOp to maximize code quality
-  /// with pseudo probes.
-  iterator getFirstNonDebugInstr(bool SkipPseudoOp = false);
-  const_iterator getFirstNonDebugInstr(bool SkipPseudoOp = false) const {
+  /// unlikely hurt profile quality, e.g., without block merging. The default
+  /// value of \c SkipPseudoOp is set to true to maximize code quality in
+  /// general, with an explict false value passed in in a few places like branch
+  /// folding and if-conversion to favor profile quality.
+  iterator getFirstNonDebugInstr(bool SkipPseudoOp = true);
+  const_iterator getFirstNonDebugInstr(bool SkipPseudoOp = true) const {
     return const_cast<MachineBasicBlock *>(this)->getFirstNonDebugInstr(
         SkipPseudoOp);
   }
 
   /// Returns an iterator to the last non-debug instruction in the basic block,
   /// or end(). Skip any pseudo operation if \c SkipPseudoOp is true.
   /// Pseudo probes are like debug instructions which do not turn into real
   /// machine code. We try to use the function to skip both debug instructions
   /// and pseudo probe operations to avoid API proliferation. This should work
   /// most of the time when considering optimizing the rest of code in the
   /// block, except for certain cases where pseudo probes are designed to block
   /// the optimizations. For example, code merge like optimizations are supposed
   /// to be blocked by pseudo probes for better AutoFDO profile quality.
   /// Therefore, they should be considered as a valid instruction when this
   /// function is called in a context of such optimizations. On the other hand,
   /// \c SkipPseudoOp should be true when it's used in optimizations that
-  /// unlikely hurt profile quality, e.g., without block merging.
-  iterator getLastNonDebugInstr(bool SkipPseudoOp = false);
-  const_iterator getLastNonDebugInstr(bool SkipPseudoOp = false) const {
+  /// unlikely hurt profile quality, e.g., without block merging. The default
+  /// value of \c SkipPseudoOp is set to true to maximize code quality in
+  /// general, with an explict false value passed in in a few places like branch
+  /// folding and if-conversion to favor profile quality.
+  iterator getLastNonDebugInstr(bool SkipPseudoOp = true);
+  const_iterator getLastNonDebugInstr(bool SkipPseudoOp = true) const {
     return const_cast<MachineBasicBlock *>(this)->getLastNonDebugInstr(
         SkipPseudoOp);
   }
 
   /// Convenience function that returns true if the block ends in a return
   /// instruction.
   bool isReturnBlock() const {
     return !empty() && back().isReturn();

 };
 
 /// 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,
-                                          bool SkipPseudoOp = false) {
+                                          bool SkipPseudoOp = true) {
   while (It != End &&
          (It->isDebugInstr() || (SkipPseudoOp && It->isPseudoProbe())))
     ++It;
   return It;
 }
 
 /// 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,
-                                           bool SkipPseudoOp = false) {
+                                           bool SkipPseudoOp = true) {
   while (It != Begin &&
          (It->isDebugInstr() || (SkipPseudoOp && It->isPseudoProbe())))
     --It;
   return It;
 }
 
 /// Increment \p It, then continue incrementing it while it points to a debug
 /// instruction. A replacement for std::next.
 template <typename IterT>
-inline IterT next_nodbg(IterT It, IterT End, bool SkipPseudoOp = false) {
+inline IterT next_nodbg(IterT It, IterT End, bool SkipPseudoOp = true) {
   return skipDebugInstructionsForward(std::next(It), End, SkipPseudoOp);
 }
 
 /// Decrement \p It, then continue decrementing it while it points to a debug
 /// instruction. A replacement for std::prev.
 template <typename IterT>
-inline IterT prev_nodbg(IterT It, IterT Begin, bool SkipPseudoOp = false) {
+inline IterT prev_nodbg(IterT It, IterT Begin, bool SkipPseudoOp = true) {
   return skipDebugInstructionsBackward(std::prev(It), Begin, SkipPseudoOp);
 }
 
 /// Construct a range iterator which begins at \p It and moves forwards until
 /// \p End is reached, skipping any debug instructions.
 template <typename IterT>
 inline auto instructionsWithoutDebug(IterT It, IterT End,
-                                     bool SkipPseudoOp = false) {
+                                     bool SkipPseudoOp = true) {
   return make_filter_range(make_range(It, End), [=](const MachineInstr &MI) {
     return !MI.isDebugInstr() && !(SkipPseudoOp && MI.isPseudoProbe());
   });
 }
 
 } // end namespace llvm
 
 #endif // LLVM_CODEGEN_MACHINEBASICBLOCK_H

llvm/lib/CodeGen/BranchFolding.cpp

for (unsigned i = 0, e = MI.getNumOperands(); i != e; ++i) {
 
     Hash += ((OperandHash << 3) | Op.getType()) << (i & 31);
   }
   return Hash;
 }
 
 /// HashEndOfMBB - Hash the last instruction in the MBB.
 static unsigned HashEndOfMBB(const MachineBasicBlock &MBB) {
-  MachineBasicBlock::const_iterator I = MBB.getLastNonDebugInstr();
+  MachineBasicBlock::const_iterator I = MBB.getLastNonDebugInstr(false);
   if (I == MBB.end())
     return 0;
 
   return HashMachineInstr(*I);
 }
 
 /// Whether MI should be counted as an instruction when calculating common tail.
 static bool countsAsInstruction(const MachineInstr &MI) {
if (CommonTailLen == 0)
     return false;
   LLVM_DEBUG(dbgs() << "Common tail length of " << printMBBReference(*MBB1)
                     << " and " << printMBBReference(*MBB2) << " is "
                     << CommonTailLen << '\n');
 
   // Move the iterators to the beginning of the MBB if we only got debug
   // instructions before the tail. This is to avoid splitting a block when we
   // only got debug instructions before the tail (to be invariant on -g).
-  if (skipDebugInstructionsForward(MBB1->begin(), MBB1->end()) == I1)
+  if (skipDebugInstructionsForward(MBB1->begin(), MBB1->end(), false) == I1)
     I1 = MBB1->begin();
-  if (skipDebugInstructionsForward(MBB2->begin(), MBB2->end()) == I2)
+  if (skipDebugInstructionsForward(MBB2->begin(), MBB2->end(), false) == I2)
     I2 = MBB2->begin();
 
   bool FullBlockTail1 = I1 == MBB1->begin();
   bool FullBlockTail2 = I2 == MBB2->begin();
 
   // It's almost always profitable to merge any number of non-terminator
   // instructions with the block that falls through into the common successor.
   // This is true only for a single successor. For multiple successors, we are
bool BranchFolder::HoistCommonCodeInSuccs(MachineBasicBlock *MBB) {
   bool HasDups = false;
   SmallSet<Register, 4> ActiveDefsSet, AllDefsSet;
   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);
+    TIB = skipDebugInstructionsForward(TIB, TIE, false);
+    FIB = skipDebugInstructionsForward(FIB, FIE, false);
     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.

llvm/lib/CodeGen/IfConversion.cpp

bool IfConverter::CountDuplicatedInstructions(
     MachineBasicBlock::iterator &FIB,
     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);
-    FIB = skipDebugInstructionsForward(FIB, FIE);
+    TIB = skipDebugInstructionsForward(TIB, TIE, false);
+    FIB = skipDebugInstructionsForward(FIB, FIE, false);
     if (TIB == TIE || 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->ClobbersPredicate(*TIB, PredDefs, 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);
-    RFIE = skipDebugInstructionsForward(RFIE, RFIB);
+    RTIE = skipDebugInstructionsForward(RTIE, RTIB, false);
+    RFIE = skipDebugInstructionsForward(RFIE, RFIB, false);
     if (RTIE == RTIB || 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;

 static void verifySameBranchInstructions(
     MachineBasicBlock *MBB1,
     MachineBasicBlock *MBB2) {
   const MachineBasicBlock::reverse_iterator B1 = MBB1->rend();
   const MachineBasicBlock::reverse_iterator B2 = MBB2->rend();
   MachineBasicBlock::reverse_iterator E1 = MBB1->rbegin();
   MachineBasicBlock::reverse_iterator E2 = MBB2->rbegin();
   while (E1 != B1 && E2 != B2) {
-    skipDebugInstructionsForward(E1, B1);
-    skipDebugInstructionsForward(E2, B2);
+    skipDebugInstructionsForward(E1, B1, false);
+    skipDebugInstructionsForward(E2, B2, false);
     if (E1 == B1 && E2 == B2)
       break;
 
     if (E1 == B1) {
       assert(!E2->isBranch() && "Branch mis-match, one block is empty.");
       break;
     }
     if (E2 == B2) {
bool IfConverter::IfConvertDiamondCommon(
   Redefs.init(*TRI);
   if (MRI->tracksLiveness()) {
     Redefs.addLiveIns(MBB1);
     Redefs.addLiveIns(MBB2);
   }
 
   // Remove the duplicated instructions at the beginnings of both paths.
   // Skip dbg_value instructions.
-  MachineBasicBlock::iterator DI1 = MBB1.getFirstNonDebugInstr();
-  MachineBasicBlock::iterator DI2 = MBB2.getFirstNonDebugInstr();
+  MachineBasicBlock::iterator DI1 = MBB1.getFirstNonDebugInstr(false);
+  MachineBasicBlock::iterator DI2 = MBB2.getFirstNonDebugInstr(false);
   BBI1->NonPredSize -= NumDups1;
   BBI2->NonPredSize -= NumDups1;
 
   // Skip past the dups on each side separately since there may be
   // differing dbg_value entries. NumDups1 can include a "return"
   // instruction, if it's not marked as "branch".
   for (unsigned i = 0; i < NumDups1; ++DI1) {
     if (DI1 == MBB1.end())

llvm/lib/CodeGen/MachineBasicBlock.cpp

MachineBasicBlock::SkipPHIsAndLabels(MachineBasicBlock::iterator I) {
   // FIXME: This needs to change if we wish to bundle labels
   // inside the bundle.
   assert((I == E || !I->isInsideBundle()) &&
          "First non-phi / non-label instruction is inside a bundle!");
   return I;
 }
 
 MachineBasicBlock::iterator
-MachineBasicBlock::SkipPHIsLabelsAndDebug(MachineBasicBlock::iterator I) {
+MachineBasicBlock::SkipPHIsLabelsAndDebug(MachineBasicBlock::iterator I,
+                                          bool SkipPseudoOp) {
   const TargetInstrInfo *TII = getParent()->getSubtarget().getInstrInfo();
 
   iterator E = end();
   while (I != E && (I->isPHI() || I->isPosition() || I->isDebugInstr() ||
+                    (SkipPseudoOp && I->isPseudoProbe()) ||
                     TII->isBasicBlockPrologue(*I)))
     ++I;
   // FIXME: This needs to change if we wish to bundle labels / dbg_values
   // inside the bundle.
   assert((I == E || !I->isInsideBundle()) &&
          "First non-phi / non-label / non-debug "
          "instruction is inside a bundle!");
   return I;