fixup! revert to trySimplifyCondBr

Author: michaelmaitland

llvm/lib/CodeGen/PeepholeOptimizer.cpp

@@ -433,8 +433,6 @@ class PeepholeOptimizer : private MachineFunction::Delegate {
   MachineDominatorTree *DT = nullptr; // Machine dominator tree
   MachineLoopInfo *MLI = nullptr;
 
-  bool NeedToInvalidateMLI = false;
-
 public:
   PeepholeOptimizer(MachineDominatorTree *DT, MachineLoopInfo *MLI)
       : DT(DT), MLI(MLI) {}
@@ -446,7 +444,6 @@ class PeepholeOptimizer : private MachineFunction::Delegate {
   /// Sequence of instructions that formulate recurrence cycle.
   using RecurrenceCycle = SmallVector<RecurrenceInstr, 4>;
 
-  bool needToInvalidateMLI() const { return NeedToInvalidateMLI; }
 private:
   bool optimizeCmpInstr(MachineInstr &MI);
   bool optimizeExtInstr(MachineInstr &MI, MachineBasicBlock &MBB,
@@ -569,8 +566,10 @@ class PeepholeOptimizerLegacy : public MachineFunctionPass {
   bool runOnMachineFunction(MachineFunction &MF) override;
 
   void getAnalysisUsage(AnalysisUsage &AU) const override {
+    AU.setPreservesCFG();
     MachineFunctionPass::getAnalysisUsage(AU);
     AU.addRequired<MachineLoopInfoWrapperPass>();
+    AU.addPreserved<MachineLoopInfoWrapperPass>();
     if (Aggressive) {
       AU.addRequired<MachineDominatorTreeWrapperPass>();
       AU.addPreserved<MachineDominatorTreeWrapperPass>();
@@ -1661,10 +1660,8 @@ PeepholeOptimizerPass::run(MachineFunction &MF,
 
   auto PA = getMachineFunctionPassPreservedAnalyses();
   PA.preserve<MachineDominatorTreeAnalysis>();
-  if (!Impl.needToInvalidateMLI()) {
-    PA.preserve<MachineLoopAnalysis>();
-    PA.preserveSet<CFGAnalyses>();
-  }
+  PA.preserve<MachineLoopAnalysis>();
+  PA.preserveSet<CFGAnalyses>();
   return PA;
 }
 
@@ -1794,24 +1791,6 @@ bool PeepholeOptimizer::run(MachineFunction &MF) {
       }
 
       if (MI->isConditionalBranch() && optimizeCondBranch(*MI)) {
-        NeedToInvalidateMLI = true;
-        // optimizeCondBranch might have converted a conditional branch to
-        // an unconditional branch. If there is a branch instruction after it,
-        // delete it.
-        MachineInstr *NewBr = &*std::prev(MII);
-        if (NewBr->isUnconditionalBranch()) {
-          if (MII != MBB.end()) {
-            MachineInstr *Dead = &*MII;
-            MachineBasicBlock *DeadDest = TII->getBranchDestBlock(*Dead);
-            if (DT && TII->getBranchDestBlock(*NewBr) != DeadDest) {
-              DT->deleteEdge(&MBB, DeadDest);
-              MLI->calculate(*DT);
-              NeedToInvalidateMLI = false;
-            }
-            ++MII;
-            Dead->eraseFromParent();
-          }
-        }
         Changed = true;
         continue;
       }

llvm/lib/Target/RISCV/RISCVInstrInfo.cpp

@@ -1014,6 +1014,109 @@ RISCVCC::CondCode RISCVCC::getOppositeBranchCondition(RISCVCC::CondCode CC) {
   }
 }
 
+// Return true if MO definitely contains the value one.
+static bool isOne(MachineOperand &MO) {
+  if (MO.isImm() && MO.getImm() == 1)
+    return true;
+
+  if (!MO.isReg() || !MO.getReg().isVirtual())
+    return false;
+
+  MachineRegisterInfo &MRI =
+      MO.getParent()->getParent()->getParent()->getRegInfo();
+  MachineInstr *DefMI = MRI.getUniqueVRegDef(MO.getReg());
+  if (!DefMI)
+    return false;
+
+  // For now, just check the canonical one value.
+  if (DefMI->getOpcode() == RISCV::ADDI &&
+      DefMI->getOperand(1).getReg() == RISCV::X0 &&
+      DefMI->getOperand(2).getImm() == 1)
+    return true;
+
+  return false;
+}
+
+// Return true if MO definitely contains the value zero.
+static bool isZero(MachineOperand &MO) {
+  if (MO.isImm() && MO.getImm() == 0)
+    return true;
+  if (MO.isReg() && MO.getReg() == RISCV::X0)
+    return true;
+  return false;
+}
+
+bool RISCVInstrInfo::trySimplifyCondBr(
+    MachineBasicBlock &MBB, MachineBasicBlock *TBB, MachineBasicBlock *FBB,
+    SmallVectorImpl<MachineOperand> &Cond) const {
+
+  if (!TBB || Cond.size() != 3)
+    return false;
+
+  RISCVCC::CondCode CC = static_cast<RISCVCC::CondCode>(Cond[0].getImm());
+  auto LHS = Cond[1];
+  auto RHS = Cond[2];
+
+  MachineBasicBlock *Folded = nullptr;
+  switch (CC) {
+  default:
+    // TODO: Implement for more CCs
+    return false;
+  case RISCVCC::COND_EQ: {
+    // We can statically evaluate that we take the first branch
+    if ((isZero(LHS) && isZero(RHS)) || (isOne(LHS) && isOne(RHS))) {
+      Folded = TBB;
+      break;
+    }
+    // We can statically evaluate that we take the second branch
+    if ((isZero(LHS) && isOne(RHS)) || (isOne(LHS) && isZero(RHS))) {
+      Folded = FBB;
+      break;
+    }
+    return false;
+  }
+  case RISCVCC::COND_NE: {
+    // We can statically evaluate that we take the first branch
+    if ((isOne(LHS) && isZero(RHS)) || (isZero(LHS) && isOne(RHS))) {
+      Folded = TBB;
+      break;
+    }
+    // We can statically evaluate that we take the second branch
+    if ((isZero(LHS) && isZero(RHS)) || (isOne(LHS) && isOne(RHS))) {
+      Folded = FBB;
+      break;
+    }
+    return false;
+  }
+  }
+
+  // At this point, its legal to optimize.
+  removeBranch(MBB);
+  Cond.clear();
+
+  // Only need to insert a branch if we're not falling through.
+  if (Folded) {
+    DebugLoc DL = MBB.findBranchDebugLoc();
+    insertBranch(MBB, Folded, nullptr, {}, DL);
+  }
+
+  // Update the successors. Remove them all and add back the correct one.
+  while (!MBB.succ_empty())
+    MBB.removeSuccessor(MBB.succ_end() - 1);
+
+  // If it's a fallthrough, we need to figure out where MBB is going.
+  if (!Folded) {
+    MachineFunction::iterator Fallthrough = ++MBB.getIterator();
+    if (Fallthrough != MBB.getParent()->end())
+      MBB.addSuccessor(&*Fallthrough);
+  } else
+    MBB.addSuccessor(Folded);
+
+  TBB = Folded;
+  FBB = nullptr;
+  return true;
+}
+
 bool RISCVInstrInfo::analyzeBranch(MachineBasicBlock &MBB,
                                    MachineBasicBlock *&TBB,
                                    MachineBasicBlock *&FBB,
@@ -1071,12 +1174,9 @@ bool RISCVInstrInfo::analyzeBranch(MachineBasicBlock &MBB,
   // Handle a single conditional branch.
   if (NumTerminators == 1 && I->getDesc().isConditionalBranch()) {
     parseCondBranch(*I, TBB, Cond);
-    // Try and optimize the conditional branch.
-    if (AllowModify) {
-      optimizeCondBranch(*I);
-      // The branch might have changed, reanalyze it.
-      return analyzeBranch(MBB, TBB, FBB, Cond, false);
-    }
+    // Try to fold the branch of the conditional branch into a the fallthru.
+    if (AllowModify)
+      trySimplifyCondBr(MBB, TBB, FBB, Cond);
     return false;
   }
 
@@ -1085,14 +1185,10 @@ bool RISCVInstrInfo::analyzeBranch(MachineBasicBlock &MBB,
       I->getDesc().isUnconditionalBranch()) {
     parseCondBranch(*std::prev(I), TBB, Cond);
     FBB = getBranchDestBlock(*I);
-    // Try and optimize the pair.
-    if (AllowModify) {
-      if (optimizeCondBranch(*std::prev(I)))
-        I->eraseFromParent();
-
-      // The branch might have changed, reanalyze it.
-      return analyzeBranch(MBB, TBB, FBB, Cond, false);
-    }
+    // Try to fold the branch of the conditional branch into an unconditional
+    // branch.
+    if (AllowModify)
+      trySimplifyCondBr(MBB, TBB, FBB, Cond);
     return false;
   }
 
@@ -1248,8 +1344,7 @@ bool RISCVInstrInfo::reverseBranchCondition(
 
 bool RISCVInstrInfo::optimizeCondBranch(MachineInstr &MI) const {
   MachineBasicBlock *MBB = MI.getParent();
-  if (!MBB)
-    return false;
+  MachineRegisterInfo &MRI = MBB->getParent()->getRegInfo();
 
   MachineBasicBlock *TBB, *FBB;
   SmallVector<MachineOperand, 3> Cond;
@@ -1259,97 +1354,8 @@ bool RISCVInstrInfo::optimizeCondBranch(MachineInstr &MI) const {
   RISCVCC::CondCode CC = static_cast<RISCVCC::CondCode>(Cond[0].getImm());
   assert(CC != RISCVCC::COND_INVALID);
 
-  // Right now we only care about LI (i.e. ADDI x0, imm)
-  auto isLoadImm = [](const MachineInstr *MI, int64_t &Imm) -> bool {
-    if (MI->getOpcode() == RISCV::ADDI && MI->getOperand(1).isReg() &&
-        MI->getOperand(1).getReg() == RISCV::X0) {
-      Imm = MI->getOperand(2).getImm();
-      return true;
-    }
+  if (CC == RISCVCC::COND_EQ || CC == RISCVCC::COND_NE)
     return false;
-  };
-
-  MachineRegisterInfo &MRI = MBB->getParent()->getRegInfo();
-  // Either a load from immediate instruction or X0.
-  auto isFromLoadImm = [&](const MachineOperand &Op, int64_t &Imm) -> bool {
-    if (!Op.isReg())
-      return false;
-    Register Reg = Op.getReg();
-    if (Reg == RISCV::X0) {
-      Imm = 0;
-      return true;
-    }
-    return Reg.isVirtual() && isLoadImm(MRI.getVRegDef(Reg), Imm);
-  };
-
-  // Try and convert a conditional branch that can be evaluated statically
-  // into an unconditional branch.
-  MachineBasicBlock *Folded = nullptr;
-  int64_t C0, C1;
-  if (isFromLoadImm(Cond[1], C0) && isFromLoadImm(Cond[2], C1)) {
-    switch (CC) {
-    case RISCVCC::COND_INVALID:
-      llvm_unreachable("Unexpected CC");
-    case RISCVCC::COND_EQ: {
-      Folded = (C0 == C1) ? TBB : FBB;
-      break;
-    }
-    case RISCVCC::COND_NE: {
-      Folded = (C0 != C1) ? TBB : FBB;
-      break;
-    }
-    case RISCVCC::COND_LT: {
-      Folded = (C0 < C1) ? TBB : FBB;
-      break;
-    }
-    case RISCVCC::COND_GE: {
-      Folded = (C0 >= C1) ? TBB : FBB;
-      break;
-    }
-    case RISCVCC::COND_LTU: {
-      Folded = ((uint64_t)C0 < (uint64_t)C1) ? TBB : FBB;
-      break;
-    }
-    case RISCVCC::COND_GEU: {
-      Folded = ((uint64_t)C0 >= (uint64_t)C1) ? TBB : FBB;
-      break;
-    }
-    }
-
-    // Do the conversion
-    // Build the new unconditional branch
-    DebugLoc DL = MBB->findBranchDebugLoc();
-    if (Folded) {
-      BuildMI(*MBB, MI, DL, get(RISCV::PseudoBR)).addMBB(Folded);
-    } else {
-      MachineFunction::iterator Fallthrough = ++MBB->getIterator();
-      if (Fallthrough == MBB->getParent()->end())
-        return false;
-      BuildMI(*MBB, MI, DL, get(RISCV::PseudoBR)).addMBB(&*Fallthrough);
-    }
-
-    // Update successors of MBB.
-    if (Folded == TBB) {
-      // If we're taking TBB, then the succ to delete is the fallthrough (if
-      // it was a succ in the first place), or its the MBB from the
-      // unconditional branch.
-      if (!FBB) {
-        MachineFunction::iterator Fallthrough = ++MBB->getIterator();
-        if (Fallthrough != MBB->getParent()->end() &&
-            MBB->isSuccessor(&*Fallthrough))
-          MBB->removeSuccessor(&*Fallthrough, true);
-      } else {
-        MBB->removeSuccessor(FBB, true);
-      }
-    } else if (Folded == FBB) {
-      // If we're taking the fallthrough or unconditional branch, then the
-      // succ to remove is the one from the conditional branch.
-      MBB->removeSuccessor(TBB, true);
-    }
-
-    MI.eraseFromParent();
-    return true;
-  }
 
   // For two constants C0 and C1 from
   // ```
@@ -1368,6 +1374,24 @@ bool RISCVInstrInfo::optimizeCondBranch(MachineInstr &MI) const {
   //
   // To make sure this optimization is really beneficial, we only
   // optimize for cases where Y had only one use (i.e. only used by the branch).
+
+  // Right now we only care about LI (i.e. ADDI x0, imm)
+  auto isLoadImm = [](const MachineInstr *MI, int64_t &Imm) -> bool {
+    if (MI->getOpcode() == RISCV::ADDI && MI->getOperand(1).isReg() &&
+        MI->getOperand(1).getReg() == RISCV::X0) {
+      Imm = MI->getOperand(2).getImm();
+      return true;
+    }
+    return false;
+  };
+  // Either a load from immediate instruction or X0.
+  auto isFromLoadImm = [&](const MachineOperand &Op, int64_t &Imm) -> bool {
+    if (!Op.isReg())
+      return false;
+    Register Reg = Op.getReg();
+    return Reg.isVirtual() && isLoadImm(MRI.getVRegDef(Reg), Imm);
+  };
+
   MachineOperand &LHS = MI.getOperand(0);
   MachineOperand &RHS = MI.getOperand(1);
   // Try to find the register for constant Z; return
@@ -1386,6 +1410,7 @@ bool RISCVInstrInfo::optimizeCondBranch(MachineInstr &MI) const {
   };
 
   bool Modify = false;
+  int64_t C0;
   if (isFromLoadImm(LHS, C0) && MRI.hasOneUse(LHS.getReg())) {
     // Might be case 1.
     // Signed integer overflow is UB. (UINT64_MAX is bigger so we don't need

llvm/lib/Target/RISCV/RISCVInstrInfo.h

@@ -319,6 +319,26 @@ class RISCVInstrInfo : public RISCVGenInstrInfo {
                                const MachineInstr &MI2) const;
   bool hasReassociableVectorSibling(const MachineInstr &Inst,
                                     bool &Commuted) const;
+  /// Return true if the branch represented by the conditional branch with
+  /// components TBB, FBB, and CurCond was folded into an unconditional branch.
+  ///
+  /// If FBB is nullptr, then the the input represents a conditional branch with
+  /// a fallthrough.
+  ///
+  /// For example:
+  /// BRCOND EQ 0, 0, BB1
+  /// BR BB2
+  ///
+  /// can be simplified to BR BB1 since 0 == 0 statically. On the other hand,
+  ///
+  ///
+  /// BRCOND EQ 0, 1, BB1
+  /// BR BB2
+  ///
+  /// can be simplified to BR BB2 because 0 != 1 statically.
+  bool trySimplifyCondBr(MachineBasicBlock &MBB, MachineBasicBlock *TBB,
+                         MachineBasicBlock *FBB,
+                         SmallVectorImpl<MachineOperand> &Cond) const;
 };
 
 namespace RISCV {

llvm/test/CodeGen/AArch64/O3-pipeline.ll

@@ -160,8 +160,6 @@
 ; CHECK-NEXT:       Machine code sinking
 ; CHECK-NEXT:       Peephole Optimizations
 ; CHECK-NEXT:       Remove dead machine instructions
-; CHECK-NEXT:       MachineDominator Tree Construction
-; CHECK-NEXT:       Machine Natural Loop Construction
 ; CHECK-NEXT:       AArch64 MI Peephole Optimization pass
 ; CHECK-NEXT:       AArch64 Dead register definitions
 ; CHECK-NEXT:       Detect Dead Lanes
@@ -171,6 +169,7 @@
 ; CHECK-NEXT:       Live Variable Analysis
 ; CHECK-NEXT:       Eliminate PHI nodes for register allocation
 ; CHECK-NEXT:       Two-Address instruction pass
+; CHECK-NEXT:       MachineDominator Tree Construction
 ; CHECK-NEXT:       Slot index numbering
 ; CHECK-NEXT:       Live Interval Analysis
 ; CHECK-NEXT:       Register Coalescer