Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[NFC][EarlyIfConverter] Replace boolean Predicate for a class #108519

Merged
merged 1 commit into from
Sep 20, 2024

Conversation

jmmartinez
Copy link
Contributor

Currently SSAIfConv is used in 2 scenarios. Generalize them to support more scenarios.

@llvmbot
Copy link
Collaborator

llvmbot commented Sep 13, 2024

@llvm/pr-subscribers-backend-amdgpu
@llvm/pr-subscribers-backend-systemz

@llvm/pr-subscribers-backend-hexagon

Author: Juan Manuel Martinez Caamaño (jmmartinez)

Changes

Currently SSAIfConv is used in 2 scenarios. Generalize them to support more scenarios.


Full diff: https://github.com/llvm/llvm-project/pull/108519.diff

1 Files Affected:

  • (modified) llvm/lib/CodeGen/EarlyIfConversion.cpp (+117-137)
diff --git a/llvm/lib/CodeGen/EarlyIfConversion.cpp b/llvm/lib/CodeGen/EarlyIfConversion.cpp
index 99ffc01ca4ebb9..ae3b367b09308a 100644
--- a/llvm/lib/CodeGen/EarlyIfConversion.cpp
+++ b/llvm/lib/CodeGen/EarlyIfConversion.cpp
@@ -122,6 +122,17 @@ class SSAIfConv {
   /// The branch condition determined by analyzeBranch.
   SmallVector<MachineOperand, 4> Cond;
 
+  struct PredicationStrategyBase {
+    virtual bool canConvertIf(MachineBasicBlock *Tail) { return true; }
+    virtual bool canPredicateInstr(const MachineInstr &I) = 0;
+    virtual void predicateBlock(MachineBasicBlock *MBB,
+                                ArrayRef<MachineOperand> Cond,
+                                bool Reverse) = 0;
+    virtual ~PredicationStrategyBase() = default;
+  };
+
+  PredicationStrategyBase &Predicate;
+
 private:
   /// Instructions in Head that define values used by the conditional blocks.
   /// The hoisted instructions must be inserted after these instructions.
@@ -137,10 +148,6 @@ class SSAIfConv {
   /// and FBB.
   MachineBasicBlock::iterator InsertionPoint;
 
-  /// Return true if all non-terminator instructions in MBB can be safely
-  /// speculated.
-  bool canSpeculateInstrs(MachineBasicBlock *MBB);
-
   /// Return true if all non-terminator instructions in MBB can be safely
   /// predicated.
   bool canPredicateInstrs(MachineBasicBlock *MBB);
@@ -149,10 +156,6 @@ class SSAIfConv {
   /// Return false if any dependency is incompatible with if conversion.
   bool InstrDependenciesAllowIfConv(MachineInstr *I);
 
-  /// Predicate all instructions of the basic block with current condition
-  /// except for terminators. Reverse the condition if ReversePredicate is set.
-  void PredicateBlock(MachineBasicBlock *MBB, bool ReversePredicate);
-
   /// Find a valid insertion point in Head.
   bool findInsertionPoint();
 
@@ -163,7 +166,8 @@ class SSAIfConv {
   void rewritePHIOperands();
 
 public:
-  SSAIfConv(MachineFunction &MF) {
+  SSAIfConv(PredicationStrategyBase &Predicate, MachineFunction &MF)
+      : Predicate(Predicate) {
     TII = MF.getSubtarget().getInstrInfo();
     TRI = MF.getSubtarget().getRegisterInfo();
     MRI = &MF.getRegInfo();
@@ -175,77 +179,14 @@ class SSAIfConv {
 
   /// canConvertIf - If the sub-CFG headed by MBB can be if-converted,
   /// initialize the internal state, and return true.
-  /// If predicate is set try to predicate the block otherwise try to
-  /// speculatively execute it.
-  bool canConvertIf(MachineBasicBlock *MBB, bool Predicate = false);
+  bool canConvertIf(MachineBasicBlock *MBB);
 
   /// convertIf - If-convert the last block passed to canConvertIf(), assuming
   /// it is possible. Add any blocks that are to be erased to RemoveBlocks.
-  void convertIf(SmallVectorImpl<MachineBasicBlock *> &RemoveBlocks,
-                 bool Predicate = false);
+  void convertIf(SmallVectorImpl<MachineBasicBlock *> &RemoveBlocks);
 };
 } // end anonymous namespace
 
-
-/// canSpeculateInstrs - Returns true if all the instructions in MBB can safely
-/// be speculated. The terminators are not considered.
-///
-/// If instructions use any values that are defined in the head basic block,
-/// the defining instructions are added to InsertAfter.
-///
-/// Any clobbered regunits are added to ClobberedRegUnits.
-///
-bool SSAIfConv::canSpeculateInstrs(MachineBasicBlock *MBB) {
-  // Reject any live-in physregs. It's probably CPSR/EFLAGS, and very hard to
-  // get right.
-  if (!MBB->livein_empty()) {
-    LLVM_DEBUG(dbgs() << printMBBReference(*MBB) << " has live-ins.\n");
-    return false;
-  }
-
-  unsigned InstrCount = 0;
-
-  // Check all instructions, except the terminators. It is assumed that
-  // terminators never have side effects or define any used register values.
-  for (MachineInstr &MI :
-       llvm::make_range(MBB->begin(), MBB->getFirstTerminator())) {
-    if (MI.isDebugInstr())
-      continue;
-
-    if (++InstrCount > BlockInstrLimit && !Stress) {
-      LLVM_DEBUG(dbgs() << printMBBReference(*MBB) << " has more than "
-                        << BlockInstrLimit << " instructions.\n");
-      return false;
-    }
-
-    // There shouldn't normally be any phis in a single-predecessor block.
-    if (MI.isPHI()) {
-      LLVM_DEBUG(dbgs() << "Can't hoist: " << MI);
-      return false;
-    }
-
-    // Don't speculate loads. Note that it may be possible and desirable to
-    // speculate GOT or constant pool loads that are guaranteed not to trap,
-    // but we don't support that for now.
-    if (MI.mayLoad()) {
-      LLVM_DEBUG(dbgs() << "Won't speculate load: " << MI);
-      return false;
-    }
-
-    // We never speculate stores, so an AA pointer isn't necessary.
-    bool DontMoveAcrossStore = true;
-    if (!MI.isSafeToMove(DontMoveAcrossStore)) {
-      LLVM_DEBUG(dbgs() << "Can't speculate: " << MI);
-      return false;
-    }
-
-    // Check for any dependencies on Head instructions.
-    if (!InstrDependenciesAllowIfConv(&MI))
-      return false;
-  }
-  return true;
-}
-
 /// Check that there is no dependencies preventing if conversion.
 ///
 /// If instruction uses any values that are defined in the head basic block,
@@ -319,17 +260,8 @@ bool SSAIfConv::canPredicateInstrs(MachineBasicBlock *MBB) {
       return false;
     }
 
-    // Check that instruction is predicable
-    if (!TII->isPredicable(*I)) {
-      LLVM_DEBUG(dbgs() << "Isn't predicable: " << *I);
-      return false;
-    }
-
-    // Check that instruction is not already predicated.
-    if (TII->isPredicated(*I) && !TII->canPredicatePredicatedInstr(*I)) {
-      LLVM_DEBUG(dbgs() << "Is already predicated: " << *I);
+    if (!Predicate.canPredicateInstr(*I))
       return false;
-    }
 
     // Check for any dependencies on Head instructions.
     if (!InstrDependenciesAllowIfConv(&(*I)))
@@ -338,24 +270,6 @@ bool SSAIfConv::canPredicateInstrs(MachineBasicBlock *MBB) {
   return true;
 }
 
-// Apply predicate to all instructions in the machine block.
-void SSAIfConv::PredicateBlock(MachineBasicBlock *MBB, bool ReversePredicate) {
-  auto Condition = Cond;
-  if (ReversePredicate) {
-    bool CanRevCond = !TII->reverseBranchCondition(Condition);
-    assert(CanRevCond && "Reversed predicate is not supported");
-    (void)CanRevCond;
-  }
-  // Terminators don't need to be predicated as they will be removed.
-  for (MachineBasicBlock::iterator I = MBB->begin(),
-                                   E = MBB->getFirstTerminator();
-       I != E; ++I) {
-    if (I->isDebugInstr())
-      continue;
-    TII->PredicateInstruction(*I, Condition);
-  }
-}
-
 /// Find an insertion point in Head for the speculated instructions. The
 /// insertion point must be:
 ///
@@ -434,7 +348,7 @@ bool SSAIfConv::findInsertionPoint() {
 /// canConvertIf - analyze the sub-cfg rooted in MBB, and return true if it is
 /// a potential candidate for if-conversion. Fill out the internal state.
 ///
-bool SSAIfConv::canConvertIf(MachineBasicBlock *MBB, bool Predicate) {
+bool SSAIfConv::canConvertIf(MachineBasicBlock *MBB) {
   Head = MBB;
   TBB = FBB = Tail = nullptr;
 
@@ -474,14 +388,6 @@ bool SSAIfConv::canConvertIf(MachineBasicBlock *MBB, bool Predicate) {
                       << printMBBReference(*Tail) << '\n');
   }
 
-  // This is a triangle or a diamond.
-  // Skip if we cannot predicate and there are no phis skip as there must be
-  // side effects that can only be handled with predication.
-  if (!Predicate && (Tail->empty() || !Tail->front().isPHI())) {
-    LLVM_DEBUG(dbgs() << "No phis in tail.\n");
-    return false;
-  }
-
   // The branch we're looking to eliminate must be analyzable.
   Cond.clear();
   if (TII->analyzeBranch(*Head, TBB, FBB, Cond)) {
@@ -489,6 +395,10 @@ bool SSAIfConv::canConvertIf(MachineBasicBlock *MBB, bool Predicate) {
     return false;
   }
 
+  if (!Predicate.canConvertIf(Tail)) {
+    return false;
+  }
+
   // This is weird, probably some sort of degenerate CFG.
   if (!TBB) {
     LLVM_DEBUG(dbgs() << "analyzeBranch didn't find conditional branch.\n");
@@ -536,17 +446,9 @@ bool SSAIfConv::canConvertIf(MachineBasicBlock *MBB, bool Predicate) {
   // Check that the conditional instructions can be speculated.
   InsertAfter.clear();
   ClobberedRegUnits.reset();
-  if (Predicate) {
-    if (TBB != Tail && !canPredicateInstrs(TBB))
-      return false;
-    if (FBB != Tail && !canPredicateInstrs(FBB))
-      return false;
-  } else {
-    if (TBB != Tail && !canSpeculateInstrs(TBB))
-      return false;
-    if (FBB != Tail && !canSpeculateInstrs(FBB))
+  for (MachineBasicBlock *MBB : {TBB, FBB})
+    if (MBB != Tail && !canPredicateInstrs(MBB))
       return false;
-  }
 
   // Try to find a valid insertion point for the speculated instructions in the
   // head basic block.
@@ -679,8 +581,7 @@ void SSAIfConv::rewritePHIOperands() {
 ///
 /// Any basic blocks that need to be erased will be added to RemoveBlocks.
 ///
-void SSAIfConv::convertIf(SmallVectorImpl<MachineBasicBlock *> &RemoveBlocks,
-                          bool Predicate) {
+void SSAIfConv::convertIf(SmallVectorImpl<MachineBasicBlock *> &RemoveBlocks) {
   assert(Head && Tail && TBB && FBB && "Call canConvertIf first.");
 
   // Update statistics.
@@ -690,16 +591,15 @@ void SSAIfConv::convertIf(SmallVectorImpl<MachineBasicBlock *> &RemoveBlocks,
     ++NumDiamondsConv;
 
   // Move all instructions into Head, except for the terminators.
-  if (TBB != Tail) {
-    if (Predicate)
-      PredicateBlock(TBB, /*ReversePredicate=*/false);
-    Head->splice(InsertionPoint, TBB, TBB->begin(), TBB->getFirstTerminator());
-  }
-  if (FBB != Tail) {
-    if (Predicate)
-      PredicateBlock(FBB, /*ReversePredicate=*/true);
-    Head->splice(InsertionPoint, FBB, FBB->begin(), FBB->getFirstTerminator());
+  for (MachineBasicBlock *MBB : {TBB, FBB}) {
+    if (MBB != Tail) {
+      // reverse the condition for the false bb
+      Predicate.predicateBlock(MBB, Cond, MBB == FBB);
+      Head->splice(InsertionPoint, MBB, MBB->begin(),
+                   MBB->getFirstTerminator());
+    }
   }
+
   // Are there extra Tail predecessors?
   bool ExtraPreds = Tail->pred_size() != 2;
   if (ExtraPreds)
@@ -863,6 +763,45 @@ template <typename Remark> Remark &operator<<(Remark &R, Cycles C) {
 }
 } // anonymous namespace
 
+struct SpeculateStrategy : SSAIfConv::PredicationStrategyBase {
+  bool canConvertIf(MachineBasicBlock *Tail) override {
+    // This is a triangle or a diamond.
+    // Skip if we cannot predicate and there are no phis skip as there must
+    // be side effects that can only be handled with predication.
+    if (Tail->empty() || !Tail->front().isPHI()) {
+      LLVM_DEBUG(dbgs() << "No phis in tail.\n");
+      return false;
+    }
+    return true;
+  }
+
+  bool canPredicateInstr(const MachineInstr &I) override {
+    // Don't speculate loads. Note that it may be possible and desirable to
+    // speculate GOT or constant pool loads that are guaranteed not to trap,
+    // but we don't support that for now.
+    if (I.mayLoad()) {
+      LLVM_DEBUG(dbgs() << "Won't speculate load: " << I);
+      return false;
+    }
+
+    // We never speculate stores, so an AA pointer isn't necessary.
+    bool DontMoveAcrossStore = true;
+    if (!I.isSafeToMove(DontMoveAcrossStore)) {
+      LLVM_DEBUG(dbgs() << "Can't speculate: " << I);
+      return false;
+    }
+    return true;
+  }
+
+  void predicateBlock(MachineBasicBlock *MBB, ArrayRef<MachineOperand> Cond,
+                      bool Reverse)
+      override { /* do nothing, everything is speculatable and it's valid to
+                    move the instructions into the head */
+  }
+
+  ~SpeculateStrategy() override = default;
+};
+
 /// Apply cost model and heuristics to the if-conversion in IfConv.
 /// Return true if the conversion is a good idea.
 ///
@@ -1096,7 +1035,8 @@ bool EarlyIfConverter::runOnMachineFunction(MachineFunction &MF) {
   MinInstr = nullptr;
 
   bool Changed = false;
-  SSAIfConv IfConv(MF);
+  SpeculateStrategy Speculate;
+  SSAIfConv IfConv(Speculate, MF);
 
   // Visit blocks in dominator tree post-order. The post-order enables nested
   // if-conversion in a single pass. The tryConvertIf() function may erase
@@ -1158,6 +1098,46 @@ void EarlyIfPredicator::getAnalysisUsage(AnalysisUsage &AU) const {
   MachineFunctionPass::getAnalysisUsage(AU);
 }
 
+struct PredicatorStrategy : SSAIfConv::PredicationStrategyBase {
+  const TargetInstrInfo *TII = nullptr;
+  PredicatorStrategy(const TargetInstrInfo *TII) : TII(TII) {}
+
+  bool canPredicateInstr(const MachineInstr &I) override {
+    // Check that instruction is predicable
+    if (!TII->isPredicable(I)) {
+      LLVM_DEBUG(dbgs() << "Isn't predicable: " << I);
+      return false;
+    }
+
+    // Check that instruction is not already predicated.
+    if (TII->isPredicated(I) && !TII->canPredicatePredicatedInstr(I)) {
+      LLVM_DEBUG(dbgs() << "Is already predicated: " << I);
+      return false;
+    }
+    return true;
+  }
+
+  void predicateBlock(MachineBasicBlock *MBB, ArrayRef<MachineOperand> Cond,
+                      bool Reverse) override {
+    SmallVector<MachineOperand> Condition(Cond.begin(), Cond.end());
+    if (Reverse) {
+      bool CanRevCond = !TII->reverseBranchCondition(Condition);
+      assert(CanRevCond && "Reversed predicate is not supported");
+      (void)CanRevCond;
+    }
+    // Terminators don't need to be predicated as they will be removed.
+    for (MachineBasicBlock::iterator I = MBB->begin(),
+                                     E = MBB->getFirstTerminator();
+         I != E; ++I) {
+      if (I->isDebugInstr())
+        continue;
+      TII->PredicateInstruction(*I, Condition);
+    }
+  }
+
+  ~PredicatorStrategy() override = default;
+};
+
 /// Apply the target heuristic to decide if the transformation is profitable.
 bool EarlyIfPredicator::shouldConvertIf(SSAIfConv &IfConv) {
   auto TrueProbability = MBPI->getEdgeProbability(IfConv.Head, IfConv.TBB);
@@ -1202,11 +1182,10 @@ bool EarlyIfPredicator::shouldConvertIf(SSAIfConv &IfConv) {
 bool EarlyIfPredicator::tryConvertIf(SSAIfConv &IfConv,
                                      MachineBasicBlock *MBB) {
   bool Changed = false;
-  while (IfConv.canConvertIf(MBB, /*Predicate=*/true) &&
-         shouldConvertIf(IfConv)) {
+  while (IfConv.canConvertIf(MBB) && shouldConvertIf(IfConv)) {
     // If-convert MBB and update analyses.
     SmallVector<MachineBasicBlock *, 4> RemoveBlocks;
-    IfConv.convertIf(RemoveBlocks, /*Predicate=*/true);
+    IfConv.convertIf(RemoveBlocks);
     Changed = true;
     updateDomTree(DomTree, IfConv, RemoveBlocks);
     for (MachineBasicBlock *MBB : RemoveBlocks)
@@ -1232,7 +1211,8 @@ bool EarlyIfPredicator::runOnMachineFunction(MachineFunction &MF) {
   MBPI = &getAnalysis<MachineBranchProbabilityInfoWrapperPass>().getMBPI();
 
   bool Changed = false;
-  SSAIfConv IfConv(MF);
+  PredicatorStrategy Predicate(TII);
+  SSAIfConv IfConv(Predicate, MF);
 
   // Visit blocks in dominator tree post-order. The post-order enables nested
   // if-conversion in a single pass. The tryConvertIf() function may erase

@llvmbot
Copy link
Collaborator

llvmbot commented Sep 13, 2024

@llvm/pr-subscribers-backend-aarch64

Author: Juan Manuel Martinez Caamaño (jmmartinez)

Changes

Currently SSAIfConv is used in 2 scenarios. Generalize them to support more scenarios.


Full diff: https://github.com/llvm/llvm-project/pull/108519.diff

1 Files Affected:

  • (modified) llvm/lib/CodeGen/EarlyIfConversion.cpp (+117-137)
diff --git a/llvm/lib/CodeGen/EarlyIfConversion.cpp b/llvm/lib/CodeGen/EarlyIfConversion.cpp
index 99ffc01ca4ebb9..ae3b367b09308a 100644
--- a/llvm/lib/CodeGen/EarlyIfConversion.cpp
+++ b/llvm/lib/CodeGen/EarlyIfConversion.cpp
@@ -122,6 +122,17 @@ class SSAIfConv {
   /// The branch condition determined by analyzeBranch.
   SmallVector<MachineOperand, 4> Cond;
 
+  struct PredicationStrategyBase {
+    virtual bool canConvertIf(MachineBasicBlock *Tail) { return true; }
+    virtual bool canPredicateInstr(const MachineInstr &I) = 0;
+    virtual void predicateBlock(MachineBasicBlock *MBB,
+                                ArrayRef<MachineOperand> Cond,
+                                bool Reverse) = 0;
+    virtual ~PredicationStrategyBase() = default;
+  };
+
+  PredicationStrategyBase &Predicate;
+
 private:
   /// Instructions in Head that define values used by the conditional blocks.
   /// The hoisted instructions must be inserted after these instructions.
@@ -137,10 +148,6 @@ class SSAIfConv {
   /// and FBB.
   MachineBasicBlock::iterator InsertionPoint;
 
-  /// Return true if all non-terminator instructions in MBB can be safely
-  /// speculated.
-  bool canSpeculateInstrs(MachineBasicBlock *MBB);
-
   /// Return true if all non-terminator instructions in MBB can be safely
   /// predicated.
   bool canPredicateInstrs(MachineBasicBlock *MBB);
@@ -149,10 +156,6 @@ class SSAIfConv {
   /// Return false if any dependency is incompatible with if conversion.
   bool InstrDependenciesAllowIfConv(MachineInstr *I);
 
-  /// Predicate all instructions of the basic block with current condition
-  /// except for terminators. Reverse the condition if ReversePredicate is set.
-  void PredicateBlock(MachineBasicBlock *MBB, bool ReversePredicate);
-
   /// Find a valid insertion point in Head.
   bool findInsertionPoint();
 
@@ -163,7 +166,8 @@ class SSAIfConv {
   void rewritePHIOperands();
 
 public:
-  SSAIfConv(MachineFunction &MF) {
+  SSAIfConv(PredicationStrategyBase &Predicate, MachineFunction &MF)
+      : Predicate(Predicate) {
     TII = MF.getSubtarget().getInstrInfo();
     TRI = MF.getSubtarget().getRegisterInfo();
     MRI = &MF.getRegInfo();
@@ -175,77 +179,14 @@ class SSAIfConv {
 
   /// canConvertIf - If the sub-CFG headed by MBB can be if-converted,
   /// initialize the internal state, and return true.
-  /// If predicate is set try to predicate the block otherwise try to
-  /// speculatively execute it.
-  bool canConvertIf(MachineBasicBlock *MBB, bool Predicate = false);
+  bool canConvertIf(MachineBasicBlock *MBB);
 
   /// convertIf - If-convert the last block passed to canConvertIf(), assuming
   /// it is possible. Add any blocks that are to be erased to RemoveBlocks.
-  void convertIf(SmallVectorImpl<MachineBasicBlock *> &RemoveBlocks,
-                 bool Predicate = false);
+  void convertIf(SmallVectorImpl<MachineBasicBlock *> &RemoveBlocks);
 };
 } // end anonymous namespace
 
-
-/// canSpeculateInstrs - Returns true if all the instructions in MBB can safely
-/// be speculated. The terminators are not considered.
-///
-/// If instructions use any values that are defined in the head basic block,
-/// the defining instructions are added to InsertAfter.
-///
-/// Any clobbered regunits are added to ClobberedRegUnits.
-///
-bool SSAIfConv::canSpeculateInstrs(MachineBasicBlock *MBB) {
-  // Reject any live-in physregs. It's probably CPSR/EFLAGS, and very hard to
-  // get right.
-  if (!MBB->livein_empty()) {
-    LLVM_DEBUG(dbgs() << printMBBReference(*MBB) << " has live-ins.\n");
-    return false;
-  }
-
-  unsigned InstrCount = 0;
-
-  // Check all instructions, except the terminators. It is assumed that
-  // terminators never have side effects or define any used register values.
-  for (MachineInstr &MI :
-       llvm::make_range(MBB->begin(), MBB->getFirstTerminator())) {
-    if (MI.isDebugInstr())
-      continue;
-
-    if (++InstrCount > BlockInstrLimit && !Stress) {
-      LLVM_DEBUG(dbgs() << printMBBReference(*MBB) << " has more than "
-                        << BlockInstrLimit << " instructions.\n");
-      return false;
-    }
-
-    // There shouldn't normally be any phis in a single-predecessor block.
-    if (MI.isPHI()) {
-      LLVM_DEBUG(dbgs() << "Can't hoist: " << MI);
-      return false;
-    }
-
-    // Don't speculate loads. Note that it may be possible and desirable to
-    // speculate GOT or constant pool loads that are guaranteed not to trap,
-    // but we don't support that for now.
-    if (MI.mayLoad()) {
-      LLVM_DEBUG(dbgs() << "Won't speculate load: " << MI);
-      return false;
-    }
-
-    // We never speculate stores, so an AA pointer isn't necessary.
-    bool DontMoveAcrossStore = true;
-    if (!MI.isSafeToMove(DontMoveAcrossStore)) {
-      LLVM_DEBUG(dbgs() << "Can't speculate: " << MI);
-      return false;
-    }
-
-    // Check for any dependencies on Head instructions.
-    if (!InstrDependenciesAllowIfConv(&MI))
-      return false;
-  }
-  return true;
-}
-
 /// Check that there is no dependencies preventing if conversion.
 ///
 /// If instruction uses any values that are defined in the head basic block,
@@ -319,17 +260,8 @@ bool SSAIfConv::canPredicateInstrs(MachineBasicBlock *MBB) {
       return false;
     }
 
-    // Check that instruction is predicable
-    if (!TII->isPredicable(*I)) {
-      LLVM_DEBUG(dbgs() << "Isn't predicable: " << *I);
-      return false;
-    }
-
-    // Check that instruction is not already predicated.
-    if (TII->isPredicated(*I) && !TII->canPredicatePredicatedInstr(*I)) {
-      LLVM_DEBUG(dbgs() << "Is already predicated: " << *I);
+    if (!Predicate.canPredicateInstr(*I))
       return false;
-    }
 
     // Check for any dependencies on Head instructions.
     if (!InstrDependenciesAllowIfConv(&(*I)))
@@ -338,24 +270,6 @@ bool SSAIfConv::canPredicateInstrs(MachineBasicBlock *MBB) {
   return true;
 }
 
-// Apply predicate to all instructions in the machine block.
-void SSAIfConv::PredicateBlock(MachineBasicBlock *MBB, bool ReversePredicate) {
-  auto Condition = Cond;
-  if (ReversePredicate) {
-    bool CanRevCond = !TII->reverseBranchCondition(Condition);
-    assert(CanRevCond && "Reversed predicate is not supported");
-    (void)CanRevCond;
-  }
-  // Terminators don't need to be predicated as they will be removed.
-  for (MachineBasicBlock::iterator I = MBB->begin(),
-                                   E = MBB->getFirstTerminator();
-       I != E; ++I) {
-    if (I->isDebugInstr())
-      continue;
-    TII->PredicateInstruction(*I, Condition);
-  }
-}
-
 /// Find an insertion point in Head for the speculated instructions. The
 /// insertion point must be:
 ///
@@ -434,7 +348,7 @@ bool SSAIfConv::findInsertionPoint() {
 /// canConvertIf - analyze the sub-cfg rooted in MBB, and return true if it is
 /// a potential candidate for if-conversion. Fill out the internal state.
 ///
-bool SSAIfConv::canConvertIf(MachineBasicBlock *MBB, bool Predicate) {
+bool SSAIfConv::canConvertIf(MachineBasicBlock *MBB) {
   Head = MBB;
   TBB = FBB = Tail = nullptr;
 
@@ -474,14 +388,6 @@ bool SSAIfConv::canConvertIf(MachineBasicBlock *MBB, bool Predicate) {
                       << printMBBReference(*Tail) << '\n');
   }
 
-  // This is a triangle or a diamond.
-  // Skip if we cannot predicate and there are no phis skip as there must be
-  // side effects that can only be handled with predication.
-  if (!Predicate && (Tail->empty() || !Tail->front().isPHI())) {
-    LLVM_DEBUG(dbgs() << "No phis in tail.\n");
-    return false;
-  }
-
   // The branch we're looking to eliminate must be analyzable.
   Cond.clear();
   if (TII->analyzeBranch(*Head, TBB, FBB, Cond)) {
@@ -489,6 +395,10 @@ bool SSAIfConv::canConvertIf(MachineBasicBlock *MBB, bool Predicate) {
     return false;
   }
 
+  if (!Predicate.canConvertIf(Tail)) {
+    return false;
+  }
+
   // This is weird, probably some sort of degenerate CFG.
   if (!TBB) {
     LLVM_DEBUG(dbgs() << "analyzeBranch didn't find conditional branch.\n");
@@ -536,17 +446,9 @@ bool SSAIfConv::canConvertIf(MachineBasicBlock *MBB, bool Predicate) {
   // Check that the conditional instructions can be speculated.
   InsertAfter.clear();
   ClobberedRegUnits.reset();
-  if (Predicate) {
-    if (TBB != Tail && !canPredicateInstrs(TBB))
-      return false;
-    if (FBB != Tail && !canPredicateInstrs(FBB))
-      return false;
-  } else {
-    if (TBB != Tail && !canSpeculateInstrs(TBB))
-      return false;
-    if (FBB != Tail && !canSpeculateInstrs(FBB))
+  for (MachineBasicBlock *MBB : {TBB, FBB})
+    if (MBB != Tail && !canPredicateInstrs(MBB))
       return false;
-  }
 
   // Try to find a valid insertion point for the speculated instructions in the
   // head basic block.
@@ -679,8 +581,7 @@ void SSAIfConv::rewritePHIOperands() {
 ///
 /// Any basic blocks that need to be erased will be added to RemoveBlocks.
 ///
-void SSAIfConv::convertIf(SmallVectorImpl<MachineBasicBlock *> &RemoveBlocks,
-                          bool Predicate) {
+void SSAIfConv::convertIf(SmallVectorImpl<MachineBasicBlock *> &RemoveBlocks) {
   assert(Head && Tail && TBB && FBB && "Call canConvertIf first.");
 
   // Update statistics.
@@ -690,16 +591,15 @@ void SSAIfConv::convertIf(SmallVectorImpl<MachineBasicBlock *> &RemoveBlocks,
     ++NumDiamondsConv;
 
   // Move all instructions into Head, except for the terminators.
-  if (TBB != Tail) {
-    if (Predicate)
-      PredicateBlock(TBB, /*ReversePredicate=*/false);
-    Head->splice(InsertionPoint, TBB, TBB->begin(), TBB->getFirstTerminator());
-  }
-  if (FBB != Tail) {
-    if (Predicate)
-      PredicateBlock(FBB, /*ReversePredicate=*/true);
-    Head->splice(InsertionPoint, FBB, FBB->begin(), FBB->getFirstTerminator());
+  for (MachineBasicBlock *MBB : {TBB, FBB}) {
+    if (MBB != Tail) {
+      // reverse the condition for the false bb
+      Predicate.predicateBlock(MBB, Cond, MBB == FBB);
+      Head->splice(InsertionPoint, MBB, MBB->begin(),
+                   MBB->getFirstTerminator());
+    }
   }
+
   // Are there extra Tail predecessors?
   bool ExtraPreds = Tail->pred_size() != 2;
   if (ExtraPreds)
@@ -863,6 +763,45 @@ template <typename Remark> Remark &operator<<(Remark &R, Cycles C) {
 }
 } // anonymous namespace
 
+struct SpeculateStrategy : SSAIfConv::PredicationStrategyBase {
+  bool canConvertIf(MachineBasicBlock *Tail) override {
+    // This is a triangle or a diamond.
+    // Skip if we cannot predicate and there are no phis skip as there must
+    // be side effects that can only be handled with predication.
+    if (Tail->empty() || !Tail->front().isPHI()) {
+      LLVM_DEBUG(dbgs() << "No phis in tail.\n");
+      return false;
+    }
+    return true;
+  }
+
+  bool canPredicateInstr(const MachineInstr &I) override {
+    // Don't speculate loads. Note that it may be possible and desirable to
+    // speculate GOT or constant pool loads that are guaranteed not to trap,
+    // but we don't support that for now.
+    if (I.mayLoad()) {
+      LLVM_DEBUG(dbgs() << "Won't speculate load: " << I);
+      return false;
+    }
+
+    // We never speculate stores, so an AA pointer isn't necessary.
+    bool DontMoveAcrossStore = true;
+    if (!I.isSafeToMove(DontMoveAcrossStore)) {
+      LLVM_DEBUG(dbgs() << "Can't speculate: " << I);
+      return false;
+    }
+    return true;
+  }
+
+  void predicateBlock(MachineBasicBlock *MBB, ArrayRef<MachineOperand> Cond,
+                      bool Reverse)
+      override { /* do nothing, everything is speculatable and it's valid to
+                    move the instructions into the head */
+  }
+
+  ~SpeculateStrategy() override = default;
+};
+
 /// Apply cost model and heuristics to the if-conversion in IfConv.
 /// Return true if the conversion is a good idea.
 ///
@@ -1096,7 +1035,8 @@ bool EarlyIfConverter::runOnMachineFunction(MachineFunction &MF) {
   MinInstr = nullptr;
 
   bool Changed = false;
-  SSAIfConv IfConv(MF);
+  SpeculateStrategy Speculate;
+  SSAIfConv IfConv(Speculate, MF);
 
   // Visit blocks in dominator tree post-order. The post-order enables nested
   // if-conversion in a single pass. The tryConvertIf() function may erase
@@ -1158,6 +1098,46 @@ void EarlyIfPredicator::getAnalysisUsage(AnalysisUsage &AU) const {
   MachineFunctionPass::getAnalysisUsage(AU);
 }
 
+struct PredicatorStrategy : SSAIfConv::PredicationStrategyBase {
+  const TargetInstrInfo *TII = nullptr;
+  PredicatorStrategy(const TargetInstrInfo *TII) : TII(TII) {}
+
+  bool canPredicateInstr(const MachineInstr &I) override {
+    // Check that instruction is predicable
+    if (!TII->isPredicable(I)) {
+      LLVM_DEBUG(dbgs() << "Isn't predicable: " << I);
+      return false;
+    }
+
+    // Check that instruction is not already predicated.
+    if (TII->isPredicated(I) && !TII->canPredicatePredicatedInstr(I)) {
+      LLVM_DEBUG(dbgs() << "Is already predicated: " << I);
+      return false;
+    }
+    return true;
+  }
+
+  void predicateBlock(MachineBasicBlock *MBB, ArrayRef<MachineOperand> Cond,
+                      bool Reverse) override {
+    SmallVector<MachineOperand> Condition(Cond.begin(), Cond.end());
+    if (Reverse) {
+      bool CanRevCond = !TII->reverseBranchCondition(Condition);
+      assert(CanRevCond && "Reversed predicate is not supported");
+      (void)CanRevCond;
+    }
+    // Terminators don't need to be predicated as they will be removed.
+    for (MachineBasicBlock::iterator I = MBB->begin(),
+                                     E = MBB->getFirstTerminator();
+         I != E; ++I) {
+      if (I->isDebugInstr())
+        continue;
+      TII->PredicateInstruction(*I, Condition);
+    }
+  }
+
+  ~PredicatorStrategy() override = default;
+};
+
 /// Apply the target heuristic to decide if the transformation is profitable.
 bool EarlyIfPredicator::shouldConvertIf(SSAIfConv &IfConv) {
   auto TrueProbability = MBPI->getEdgeProbability(IfConv.Head, IfConv.TBB);
@@ -1202,11 +1182,10 @@ bool EarlyIfPredicator::shouldConvertIf(SSAIfConv &IfConv) {
 bool EarlyIfPredicator::tryConvertIf(SSAIfConv &IfConv,
                                      MachineBasicBlock *MBB) {
   bool Changed = false;
-  while (IfConv.canConvertIf(MBB, /*Predicate=*/true) &&
-         shouldConvertIf(IfConv)) {
+  while (IfConv.canConvertIf(MBB) && shouldConvertIf(IfConv)) {
     // If-convert MBB and update analyses.
     SmallVector<MachineBasicBlock *, 4> RemoveBlocks;
-    IfConv.convertIf(RemoveBlocks, /*Predicate=*/true);
+    IfConv.convertIf(RemoveBlocks);
     Changed = true;
     updateDomTree(DomTree, IfConv, RemoveBlocks);
     for (MachineBasicBlock *MBB : RemoveBlocks)
@@ -1232,7 +1211,8 @@ bool EarlyIfPredicator::runOnMachineFunction(MachineFunction &MF) {
   MBPI = &getAnalysis<MachineBranchProbabilityInfoWrapperPass>().getMBPI();
 
   bool Changed = false;
-  SSAIfConv IfConv(MF);
+  PredicatorStrategy Predicate(TII);
+  SSAIfConv IfConv(Predicate, MF);
 
   // Visit blocks in dominator tree post-order. The post-order enables nested
   // if-conversion in a single pass. The tryConvertIf() function may erase

@llvmbot
Copy link
Collaborator

llvmbot commented Sep 13, 2024

@llvm/pr-subscribers-backend-x86

Author: Juan Manuel Martinez Caamaño (jmmartinez)

Changes

Currently SSAIfConv is used in 2 scenarios. Generalize them to support more scenarios.


Full diff: https://github.com/llvm/llvm-project/pull/108519.diff

1 Files Affected:

  • (modified) llvm/lib/CodeGen/EarlyIfConversion.cpp (+117-137)
diff --git a/llvm/lib/CodeGen/EarlyIfConversion.cpp b/llvm/lib/CodeGen/EarlyIfConversion.cpp
index 99ffc01ca4ebb9..ae3b367b09308a 100644
--- a/llvm/lib/CodeGen/EarlyIfConversion.cpp
+++ b/llvm/lib/CodeGen/EarlyIfConversion.cpp
@@ -122,6 +122,17 @@ class SSAIfConv {
   /// The branch condition determined by analyzeBranch.
   SmallVector<MachineOperand, 4> Cond;
 
+  struct PredicationStrategyBase {
+    virtual bool canConvertIf(MachineBasicBlock *Tail) { return true; }
+    virtual bool canPredicateInstr(const MachineInstr &I) = 0;
+    virtual void predicateBlock(MachineBasicBlock *MBB,
+                                ArrayRef<MachineOperand> Cond,
+                                bool Reverse) = 0;
+    virtual ~PredicationStrategyBase() = default;
+  };
+
+  PredicationStrategyBase &Predicate;
+
 private:
   /// Instructions in Head that define values used by the conditional blocks.
   /// The hoisted instructions must be inserted after these instructions.
@@ -137,10 +148,6 @@ class SSAIfConv {
   /// and FBB.
   MachineBasicBlock::iterator InsertionPoint;
 
-  /// Return true if all non-terminator instructions in MBB can be safely
-  /// speculated.
-  bool canSpeculateInstrs(MachineBasicBlock *MBB);
-
   /// Return true if all non-terminator instructions in MBB can be safely
   /// predicated.
   bool canPredicateInstrs(MachineBasicBlock *MBB);
@@ -149,10 +156,6 @@ class SSAIfConv {
   /// Return false if any dependency is incompatible with if conversion.
   bool InstrDependenciesAllowIfConv(MachineInstr *I);
 
-  /// Predicate all instructions of the basic block with current condition
-  /// except for terminators. Reverse the condition if ReversePredicate is set.
-  void PredicateBlock(MachineBasicBlock *MBB, bool ReversePredicate);
-
   /// Find a valid insertion point in Head.
   bool findInsertionPoint();
 
@@ -163,7 +166,8 @@ class SSAIfConv {
   void rewritePHIOperands();
 
 public:
-  SSAIfConv(MachineFunction &MF) {
+  SSAIfConv(PredicationStrategyBase &Predicate, MachineFunction &MF)
+      : Predicate(Predicate) {
     TII = MF.getSubtarget().getInstrInfo();
     TRI = MF.getSubtarget().getRegisterInfo();
     MRI = &MF.getRegInfo();
@@ -175,77 +179,14 @@ class SSAIfConv {
 
   /// canConvertIf - If the sub-CFG headed by MBB can be if-converted,
   /// initialize the internal state, and return true.
-  /// If predicate is set try to predicate the block otherwise try to
-  /// speculatively execute it.
-  bool canConvertIf(MachineBasicBlock *MBB, bool Predicate = false);
+  bool canConvertIf(MachineBasicBlock *MBB);
 
   /// convertIf - If-convert the last block passed to canConvertIf(), assuming
   /// it is possible. Add any blocks that are to be erased to RemoveBlocks.
-  void convertIf(SmallVectorImpl<MachineBasicBlock *> &RemoveBlocks,
-                 bool Predicate = false);
+  void convertIf(SmallVectorImpl<MachineBasicBlock *> &RemoveBlocks);
 };
 } // end anonymous namespace
 
-
-/// canSpeculateInstrs - Returns true if all the instructions in MBB can safely
-/// be speculated. The terminators are not considered.
-///
-/// If instructions use any values that are defined in the head basic block,
-/// the defining instructions are added to InsertAfter.
-///
-/// Any clobbered regunits are added to ClobberedRegUnits.
-///
-bool SSAIfConv::canSpeculateInstrs(MachineBasicBlock *MBB) {
-  // Reject any live-in physregs. It's probably CPSR/EFLAGS, and very hard to
-  // get right.
-  if (!MBB->livein_empty()) {
-    LLVM_DEBUG(dbgs() << printMBBReference(*MBB) << " has live-ins.\n");
-    return false;
-  }
-
-  unsigned InstrCount = 0;
-
-  // Check all instructions, except the terminators. It is assumed that
-  // terminators never have side effects or define any used register values.
-  for (MachineInstr &MI :
-       llvm::make_range(MBB->begin(), MBB->getFirstTerminator())) {
-    if (MI.isDebugInstr())
-      continue;
-
-    if (++InstrCount > BlockInstrLimit && !Stress) {
-      LLVM_DEBUG(dbgs() << printMBBReference(*MBB) << " has more than "
-                        << BlockInstrLimit << " instructions.\n");
-      return false;
-    }
-
-    // There shouldn't normally be any phis in a single-predecessor block.
-    if (MI.isPHI()) {
-      LLVM_DEBUG(dbgs() << "Can't hoist: " << MI);
-      return false;
-    }
-
-    // Don't speculate loads. Note that it may be possible and desirable to
-    // speculate GOT or constant pool loads that are guaranteed not to trap,
-    // but we don't support that for now.
-    if (MI.mayLoad()) {
-      LLVM_DEBUG(dbgs() << "Won't speculate load: " << MI);
-      return false;
-    }
-
-    // We never speculate stores, so an AA pointer isn't necessary.
-    bool DontMoveAcrossStore = true;
-    if (!MI.isSafeToMove(DontMoveAcrossStore)) {
-      LLVM_DEBUG(dbgs() << "Can't speculate: " << MI);
-      return false;
-    }
-
-    // Check for any dependencies on Head instructions.
-    if (!InstrDependenciesAllowIfConv(&MI))
-      return false;
-  }
-  return true;
-}
-
 /// Check that there is no dependencies preventing if conversion.
 ///
 /// If instruction uses any values that are defined in the head basic block,
@@ -319,17 +260,8 @@ bool SSAIfConv::canPredicateInstrs(MachineBasicBlock *MBB) {
       return false;
     }
 
-    // Check that instruction is predicable
-    if (!TII->isPredicable(*I)) {
-      LLVM_DEBUG(dbgs() << "Isn't predicable: " << *I);
-      return false;
-    }
-
-    // Check that instruction is not already predicated.
-    if (TII->isPredicated(*I) && !TII->canPredicatePredicatedInstr(*I)) {
-      LLVM_DEBUG(dbgs() << "Is already predicated: " << *I);
+    if (!Predicate.canPredicateInstr(*I))
       return false;
-    }
 
     // Check for any dependencies on Head instructions.
     if (!InstrDependenciesAllowIfConv(&(*I)))
@@ -338,24 +270,6 @@ bool SSAIfConv::canPredicateInstrs(MachineBasicBlock *MBB) {
   return true;
 }
 
-// Apply predicate to all instructions in the machine block.
-void SSAIfConv::PredicateBlock(MachineBasicBlock *MBB, bool ReversePredicate) {
-  auto Condition = Cond;
-  if (ReversePredicate) {
-    bool CanRevCond = !TII->reverseBranchCondition(Condition);
-    assert(CanRevCond && "Reversed predicate is not supported");
-    (void)CanRevCond;
-  }
-  // Terminators don't need to be predicated as they will be removed.
-  for (MachineBasicBlock::iterator I = MBB->begin(),
-                                   E = MBB->getFirstTerminator();
-       I != E; ++I) {
-    if (I->isDebugInstr())
-      continue;
-    TII->PredicateInstruction(*I, Condition);
-  }
-}
-
 /// Find an insertion point in Head for the speculated instructions. The
 /// insertion point must be:
 ///
@@ -434,7 +348,7 @@ bool SSAIfConv::findInsertionPoint() {
 /// canConvertIf - analyze the sub-cfg rooted in MBB, and return true if it is
 /// a potential candidate for if-conversion. Fill out the internal state.
 ///
-bool SSAIfConv::canConvertIf(MachineBasicBlock *MBB, bool Predicate) {
+bool SSAIfConv::canConvertIf(MachineBasicBlock *MBB) {
   Head = MBB;
   TBB = FBB = Tail = nullptr;
 
@@ -474,14 +388,6 @@ bool SSAIfConv::canConvertIf(MachineBasicBlock *MBB, bool Predicate) {
                       << printMBBReference(*Tail) << '\n');
   }
 
-  // This is a triangle or a diamond.
-  // Skip if we cannot predicate and there are no phis skip as there must be
-  // side effects that can only be handled with predication.
-  if (!Predicate && (Tail->empty() || !Tail->front().isPHI())) {
-    LLVM_DEBUG(dbgs() << "No phis in tail.\n");
-    return false;
-  }
-
   // The branch we're looking to eliminate must be analyzable.
   Cond.clear();
   if (TII->analyzeBranch(*Head, TBB, FBB, Cond)) {
@@ -489,6 +395,10 @@ bool SSAIfConv::canConvertIf(MachineBasicBlock *MBB, bool Predicate) {
     return false;
   }
 
+  if (!Predicate.canConvertIf(Tail)) {
+    return false;
+  }
+
   // This is weird, probably some sort of degenerate CFG.
   if (!TBB) {
     LLVM_DEBUG(dbgs() << "analyzeBranch didn't find conditional branch.\n");
@@ -536,17 +446,9 @@ bool SSAIfConv::canConvertIf(MachineBasicBlock *MBB, bool Predicate) {
   // Check that the conditional instructions can be speculated.
   InsertAfter.clear();
   ClobberedRegUnits.reset();
-  if (Predicate) {
-    if (TBB != Tail && !canPredicateInstrs(TBB))
-      return false;
-    if (FBB != Tail && !canPredicateInstrs(FBB))
-      return false;
-  } else {
-    if (TBB != Tail && !canSpeculateInstrs(TBB))
-      return false;
-    if (FBB != Tail && !canSpeculateInstrs(FBB))
+  for (MachineBasicBlock *MBB : {TBB, FBB})
+    if (MBB != Tail && !canPredicateInstrs(MBB))
       return false;
-  }
 
   // Try to find a valid insertion point for the speculated instructions in the
   // head basic block.
@@ -679,8 +581,7 @@ void SSAIfConv::rewritePHIOperands() {
 ///
 /// Any basic blocks that need to be erased will be added to RemoveBlocks.
 ///
-void SSAIfConv::convertIf(SmallVectorImpl<MachineBasicBlock *> &RemoveBlocks,
-                          bool Predicate) {
+void SSAIfConv::convertIf(SmallVectorImpl<MachineBasicBlock *> &RemoveBlocks) {
   assert(Head && Tail && TBB && FBB && "Call canConvertIf first.");
 
   // Update statistics.
@@ -690,16 +591,15 @@ void SSAIfConv::convertIf(SmallVectorImpl<MachineBasicBlock *> &RemoveBlocks,
     ++NumDiamondsConv;
 
   // Move all instructions into Head, except for the terminators.
-  if (TBB != Tail) {
-    if (Predicate)
-      PredicateBlock(TBB, /*ReversePredicate=*/false);
-    Head->splice(InsertionPoint, TBB, TBB->begin(), TBB->getFirstTerminator());
-  }
-  if (FBB != Tail) {
-    if (Predicate)
-      PredicateBlock(FBB, /*ReversePredicate=*/true);
-    Head->splice(InsertionPoint, FBB, FBB->begin(), FBB->getFirstTerminator());
+  for (MachineBasicBlock *MBB : {TBB, FBB}) {
+    if (MBB != Tail) {
+      // reverse the condition for the false bb
+      Predicate.predicateBlock(MBB, Cond, MBB == FBB);
+      Head->splice(InsertionPoint, MBB, MBB->begin(),
+                   MBB->getFirstTerminator());
+    }
   }
+
   // Are there extra Tail predecessors?
   bool ExtraPreds = Tail->pred_size() != 2;
   if (ExtraPreds)
@@ -863,6 +763,45 @@ template <typename Remark> Remark &operator<<(Remark &R, Cycles C) {
 }
 } // anonymous namespace
 
+struct SpeculateStrategy : SSAIfConv::PredicationStrategyBase {
+  bool canConvertIf(MachineBasicBlock *Tail) override {
+    // This is a triangle or a diamond.
+    // Skip if we cannot predicate and there are no phis skip as there must
+    // be side effects that can only be handled with predication.
+    if (Tail->empty() || !Tail->front().isPHI()) {
+      LLVM_DEBUG(dbgs() << "No phis in tail.\n");
+      return false;
+    }
+    return true;
+  }
+
+  bool canPredicateInstr(const MachineInstr &I) override {
+    // Don't speculate loads. Note that it may be possible and desirable to
+    // speculate GOT or constant pool loads that are guaranteed not to trap,
+    // but we don't support that for now.
+    if (I.mayLoad()) {
+      LLVM_DEBUG(dbgs() << "Won't speculate load: " << I);
+      return false;
+    }
+
+    // We never speculate stores, so an AA pointer isn't necessary.
+    bool DontMoveAcrossStore = true;
+    if (!I.isSafeToMove(DontMoveAcrossStore)) {
+      LLVM_DEBUG(dbgs() << "Can't speculate: " << I);
+      return false;
+    }
+    return true;
+  }
+
+  void predicateBlock(MachineBasicBlock *MBB, ArrayRef<MachineOperand> Cond,
+                      bool Reverse)
+      override { /* do nothing, everything is speculatable and it's valid to
+                    move the instructions into the head */
+  }
+
+  ~SpeculateStrategy() override = default;
+};
+
 /// Apply cost model and heuristics to the if-conversion in IfConv.
 /// Return true if the conversion is a good idea.
 ///
@@ -1096,7 +1035,8 @@ bool EarlyIfConverter::runOnMachineFunction(MachineFunction &MF) {
   MinInstr = nullptr;
 
   bool Changed = false;
-  SSAIfConv IfConv(MF);
+  SpeculateStrategy Speculate;
+  SSAIfConv IfConv(Speculate, MF);
 
   // Visit blocks in dominator tree post-order. The post-order enables nested
   // if-conversion in a single pass. The tryConvertIf() function may erase
@@ -1158,6 +1098,46 @@ void EarlyIfPredicator::getAnalysisUsage(AnalysisUsage &AU) const {
   MachineFunctionPass::getAnalysisUsage(AU);
 }
 
+struct PredicatorStrategy : SSAIfConv::PredicationStrategyBase {
+  const TargetInstrInfo *TII = nullptr;
+  PredicatorStrategy(const TargetInstrInfo *TII) : TII(TII) {}
+
+  bool canPredicateInstr(const MachineInstr &I) override {
+    // Check that instruction is predicable
+    if (!TII->isPredicable(I)) {
+      LLVM_DEBUG(dbgs() << "Isn't predicable: " << I);
+      return false;
+    }
+
+    // Check that instruction is not already predicated.
+    if (TII->isPredicated(I) && !TII->canPredicatePredicatedInstr(I)) {
+      LLVM_DEBUG(dbgs() << "Is already predicated: " << I);
+      return false;
+    }
+    return true;
+  }
+
+  void predicateBlock(MachineBasicBlock *MBB, ArrayRef<MachineOperand> Cond,
+                      bool Reverse) override {
+    SmallVector<MachineOperand> Condition(Cond.begin(), Cond.end());
+    if (Reverse) {
+      bool CanRevCond = !TII->reverseBranchCondition(Condition);
+      assert(CanRevCond && "Reversed predicate is not supported");
+      (void)CanRevCond;
+    }
+    // Terminators don't need to be predicated as they will be removed.
+    for (MachineBasicBlock::iterator I = MBB->begin(),
+                                     E = MBB->getFirstTerminator();
+         I != E; ++I) {
+      if (I->isDebugInstr())
+        continue;
+      TII->PredicateInstruction(*I, Condition);
+    }
+  }
+
+  ~PredicatorStrategy() override = default;
+};
+
 /// Apply the target heuristic to decide if the transformation is profitable.
 bool EarlyIfPredicator::shouldConvertIf(SSAIfConv &IfConv) {
   auto TrueProbability = MBPI->getEdgeProbability(IfConv.Head, IfConv.TBB);
@@ -1202,11 +1182,10 @@ bool EarlyIfPredicator::shouldConvertIf(SSAIfConv &IfConv) {
 bool EarlyIfPredicator::tryConvertIf(SSAIfConv &IfConv,
                                      MachineBasicBlock *MBB) {
   bool Changed = false;
-  while (IfConv.canConvertIf(MBB, /*Predicate=*/true) &&
-         shouldConvertIf(IfConv)) {
+  while (IfConv.canConvertIf(MBB) && shouldConvertIf(IfConv)) {
     // If-convert MBB and update analyses.
     SmallVector<MachineBasicBlock *, 4> RemoveBlocks;
-    IfConv.convertIf(RemoveBlocks, /*Predicate=*/true);
+    IfConv.convertIf(RemoveBlocks);
     Changed = true;
     updateDomTree(DomTree, IfConv, RemoveBlocks);
     for (MachineBasicBlock *MBB : RemoveBlocks)
@@ -1232,7 +1211,8 @@ bool EarlyIfPredicator::runOnMachineFunction(MachineFunction &MF) {
   MBPI = &getAnalysis<MachineBranchProbabilityInfoWrapperPass>().getMBPI();
 
   bool Changed = false;
-  SSAIfConv IfConv(MF);
+  PredicatorStrategy Predicate(TII);
+  SSAIfConv IfConv(Predicate, MF);
 
   // Visit blocks in dominator tree post-order. The post-order enables nested
   // if-conversion in a single pass. The tryConvertIf() function may erase

@arsenm arsenm changed the title [NFC][EarlyIfConverter] Replace boolean Preadicate for a class [NFC][EarlyIfConverter] Replace boolean Predicate for a class Sep 13, 2024
This will help when adding new EarlyIfConverter transformations (other
than the current 2)
@jmmartinez
Copy link
Contributor Author

If this PR is ok for you I'd like to merge it.

@jmmartinez jmmartinez merged commit 9e73159 into llvm:main Sep 20, 2024
8 checks passed
@arsenm
Copy link
Contributor

arsenm commented Sep 20, 2024

Wasn't reviewed?

@jmmartinez
Copy link
Contributor Author

Wasn't reviewed?

Sorry, I've assumed it was. I've addressed the remarks and after some time without new feedback I merged it.

@arsenm
Copy link
Contributor

arsenm commented Oct 4, 2024

Are you still planning on using this? I thought you abandoned the speculative execution changes. If so should this be reverted?

@jmmartinez
Copy link
Contributor Author

I'll revert them. I don't think we need them anymore.

jmmartinez added a commit to jmmartinez/llvm-project that referenced this pull request Oct 7, 2024
jmmartinez added a commit to jmmartinez/llvm-project that referenced this pull request Oct 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants