Minor cleanup in ARCSequenceOpts (swiftlang#33578)

* Remove NewInsts from ARCSequenceOpts

* Remove more instances of InsertPts

* Address comments from swiftlang#33504

* Make bottom up loop traversal simpler. Use better apis

* Update LoopRegion printer with more info

Author: meg-gupta

include/swift/SILOptimizer/Analysis/LoopRegionAnalysis.h

@@ -726,8 +726,9 @@ class LoopRegion {
     return getSubregionData().RPONumOfHeaderBlock;
   }
 
-  void dump() const;
-  void print(llvm::raw_ostream &os, bool insertSpaces = false) const;
+  void dump(bool isVerbose = false) const;
+  void print(llvm::raw_ostream &os, bool isShort = false,
+             bool isVerbose = false) const;
   void dumpName() const;
   void printName(llvm::raw_ostream &os) const;
 

lib/SILOptimizer/ARC/ARCRegionState.cpp

@@ -235,14 +235,12 @@ bool ARCRegionState::processBlockBottomUp(
   return NestingDetected;
 }
 
-// Find the relevant insertion points for the loop region R in its
-// successors. Returns true if we succeeded. Returns false if any of the
-// non-local successors of the region are not leaking blocks. We currently do
-// not handle early exits, but do handle trapping blocks.
-static bool getInsertionPtsForLoopRegionExits(
+// Returns true if any of the non-local successors of the region are leaking
+// blocks. We currently do not handle early exits, but do handle trapping
+// blocks. Returns false if otherwise
+static bool hasEarlyExits(
     const LoopRegion *R, LoopRegionFunctionInfo *LRFI,
-    llvm::DenseMap<const LoopRegion *, ARCRegionState *> &RegionStateInfo,
-    llvm::SmallVectorImpl<SILInstruction *> &InsertPts) {
+    llvm::DenseMap<const LoopRegion *, ARCRegionState *> &RegionStateInfo) {
   assert(R->isLoop() && "Expected a loop region that is representing a loop");
 
   // Go through all of our non local successors. If any of them cannot be
@@ -251,23 +249,10 @@ static bool getInsertionPtsForLoopRegionExits(
   if (any_of(R->getNonLocalSuccs(), [&](unsigned SuccID) -> bool {
         return !RegionStateInfo[LRFI->getRegion(SuccID)]->allowsLeaks();
       })) {
-    return false;
-  }
-
-  // We assume that all of our loops have been canonicalized so that /all/ loop
-  // exit blocks only have exiting blocks as predecessors. This means that all
-  // successor regions of any region /cannot/ be a region representing a loop.
-  for (unsigned SuccID : R->getLocalSuccs()) {
-    auto *SuccRegion = LRFI->getRegion(SuccID);
-    assert(SuccRegion->isBlock() && "Loop canonicalization failed?!");
-    InsertPts.push_back(&*SuccRegion->getBlock()->begin());
+    return true;
   }
 
-  // Sort and unique the insert points so we can put them into
-  // ImmutablePointerSets.
-  sortUnique(InsertPts);
-
-  return true;
+  return false;
 }
 
 bool ARCRegionState::processLoopBottomUp(
@@ -276,11 +261,9 @@ bool ARCRegionState::processLoopBottomUp(
     ImmutablePointerSetFactory<SILInstruction> &SetFactory) {
   ARCRegionState *State = RegionStateInfo[R];
 
-  llvm::SmallVector<SILInstruction *, 2> InsertPts;
-  // Try to lookup insertion points for this region. If when checking for
-  // insertion points, we find that we have non-leaking early exits, clear state
+  // If we find that we have non-leaking early exits, clear state
   // and bail. We do not handle these for now.
-  if (!getInsertionPtsForLoopRegionExits(R, LRFI, RegionStateInfo, InsertPts)) {
+  if (hasEarlyExits(R, LRFI, RegionStateInfo)) {
     clearBottomUpState();
     return false;
   }

lib/SILOptimizer/ARC/ARCSequenceOptUtils.h

@@ -21,7 +21,7 @@
 #include "swift/SIL/SILInstruction.h"
 
 namespace swift {
-  bool isARCSignificantTerminator(TermInst *TI);
+bool isARCSignificantTerminator(TermInst *TI);
 } // end namespace swift
 
 #endif

lib/SILOptimizer/ARC/ARCSequenceOpts.cpp

@@ -48,7 +48,7 @@ llvm::cl::opt<bool> EnableLoopARC("enable-loop-arc", llvm::cl::init(false));
 // This routine takes in the ARCMatchingSet \p MatchSet and adds the increments
 // and decrements to the delete list.
 void ARCPairingContext::optimizeMatchingSet(
-    ARCMatchingSet &MatchSet, llvm::SmallVectorImpl<SILInstruction *> &NewInsts,
+    ARCMatchingSet &MatchSet,
     llvm::SmallVectorImpl<SILInstruction *> &DeadInsts) {
   LLVM_DEBUG(llvm::dbgs() << "**** Optimizing Matching Set ****\n");
   // Add the old increments to the delete list.
@@ -69,7 +69,6 @@ void ARCPairingContext::optimizeMatchingSet(
 }
 
 bool ARCPairingContext::performMatching(
-    llvm::SmallVectorImpl<SILInstruction *> &NewInsts,
     llvm::SmallVectorImpl<SILInstruction *> &DeadInsts) {
   bool MatchedPair = false;
 
@@ -97,7 +96,7 @@ bool ARCPairingContext::performMatching(
       for (auto *I : Set.Decrements)
         DecToIncStateMap.erase(I);
 
-      optimizeMatchingSet(Set, NewInsts, DeadInsts);
+      optimizeMatchingSet(Set, DeadInsts);
     }
   }
 
@@ -131,7 +130,6 @@ void LoopARCPairingContext::runOnFunction(SILFunction *F) {
 bool LoopARCPairingContext::processRegion(const LoopRegion *Region,
                                           bool FreezePostDomReleases,
                                           bool RecomputePostDomReleases) {
-  llvm::SmallVector<SILInstruction *, 8> NewInsts;
   llvm::SmallVector<SILInstruction *, 8> DeadInsts;
 
   // We have already summarized all subloops of this loop. Now summarize our
@@ -145,16 +143,7 @@ bool LoopARCPairingContext::processRegion(const LoopRegion *Region,
   do {
     NestingDetected = Evaluator.runOnLoop(Region, FreezePostDomReleases,
                                           RecomputePostDomReleases);
-    MatchedPair = Context.performMatching(NewInsts, DeadInsts);
-
-    if (!NewInsts.empty()) {
-      LLVM_DEBUG(llvm::dbgs() << "Adding new interesting insts!\n");
-      do {
-        auto *I = NewInsts.pop_back_val();
-        LLVM_DEBUG(llvm::dbgs() << "    " << *I);
-        Evaluator.addInterestingInst(I);
-      } while (!NewInsts.empty());
-    }
+    MatchedPair = Context.performMatching(DeadInsts);
 
     if (!DeadInsts.empty()) {
       LLVM_DEBUG(llvm::dbgs() << "Removing dead interesting insts!\n");

lib/SILOptimizer/ARC/ARCSequenceOpts.h

@@ -39,11 +39,9 @@ struct ARCPairingContext {
 
   ARCPairingContext(SILFunction &F, RCIdentityFunctionInfo *RCIA)
       : F(F), DecToIncStateMap(), IncToDecStateMap(), RCIA(RCIA) {}
-  bool performMatching(llvm::SmallVectorImpl<SILInstruction *> &NewInsts,
-                       llvm::SmallVectorImpl<SILInstruction *> &DeadInsts);
+  bool performMatching(llvm::SmallVectorImpl<SILInstruction *> &DeadInsts);
 
   void optimizeMatchingSet(ARCMatchingSet &MatchSet,
-                           llvm::SmallVectorImpl<SILInstruction *> &NewInsts,
                            llvm::SmallVectorImpl<SILInstruction *> &DeadInsts);
 };
 
@@ -68,10 +66,8 @@ struct BlockARCPairingContext {
     bool NestingDetected = Evaluator.run(FreezePostDomReleases);
     Evaluator.clear();
 
-    llvm::SmallVector<SILInstruction *, 8> NewInsts;
     llvm::SmallVector<SILInstruction *, 8> DeadInsts;
-    bool MatchedPair = Context.performMatching(NewInsts, DeadInsts);
-    NewInsts.clear();
+    bool MatchedPair = Context.performMatching(DeadInsts);
     while (!DeadInsts.empty())
       DeadInsts.pop_back_val()->eraseFromParent();
     return NestingDetected && MatchedPair;

lib/SILOptimizer/ARC/GlobalARCSequenceDataflow.cpp

@@ -118,7 +118,6 @@ bool ARCSequenceDataflowEvaluator::processBBTopDown(ARCBBState &BBState) {
 void ARCSequenceDataflowEvaluator::mergePredecessors(
     ARCBBStateInfoHandle &DataHandle) {
   bool HasAtLeastOnePred = false;
-  llvm::SmallVector<SILBasicBlock *, 4> BBThatNeedInsertPts;
 
   SILBasicBlock *BB = DataHandle.getBB();
   ARCBBState &BBState = DataHandle.getState();
@@ -223,10 +222,8 @@ bool ARCSequenceDataflowEvaluator::processBBBottomUp(
       SetFactory);
 
   auto II = BB.rbegin();
-  if (isa<TermInst>(*II)) {
-    if (!isARCSignificantTerminator(&cast<TermInst>(*II))) {
-      II++;
-    }
+  if (!isARCSignificantTerminator(&cast<TermInst>(*II))) {
+    II++;
   }
 
   // For each instruction I in BB visited in reverse...

lib/SILOptimizer/ARC/GlobalLoopARCSequenceDataflow.cpp

@@ -200,40 +200,14 @@ bool LoopARCSequenceDataflowEvaluator::processLoopBottomUp(
   bool NestingDetected = false;
 
   // For each BB in our post order...
-  auto Start = R->subregion_begin(), End = R->subregion_end();
-  if (Start == End)
-    return false;
-
-  --End;
-  while (Start != End) {
-    unsigned SubregionIndex = *End;
+  for (unsigned SubregionIndex : R->getReverseSubregions()) {
     auto *Subregion = LRFI->getRegion(SubregionIndex);
     auto &SubregionData = getARCState(Subregion);
 
     // This will always succeed since we have an entry for each BB in our post
     // order.
-    LLVM_DEBUG(llvm::dbgs() << "Processing Subregion#: " << SubregionIndex
-                            << "\n");
-
-    LLVM_DEBUG(llvm::dbgs() << "Merging Successors!\n");
-    mergeSuccessors(Subregion, SubregionData);
-
-    // Then perform the region optimization.
-    NestingDetected |= SubregionData.processBottomUp(
-        AA, RCFI, EAFI, LRFI, FreezeOwnedArgEpilogueReleases, IncToDecStateMap,
-        RegionStateInfo, SetFactory);
-    --End;
-  }
-
-  {
-    unsigned SubregionIndex = *End;
-    auto *Subregion = LRFI->getRegion(SubregionIndex);
-    auto &SubregionData = getARCState(Subregion);
-
-    // This will always succeed since we have an entry for each BB in our post
-    // order.
-    LLVM_DEBUG(llvm::dbgs() << "Processing Subregion#: " << SubregionIndex
-                            << "\n");
+    LLVM_DEBUG(llvm::dbgs()
+               << "Processing Subregion#: " << SubregionIndex << "\n");
 
     LLVM_DEBUG(llvm::dbgs() << "Merging Successors!\n");
     mergeSuccessors(Subregion, SubregionData);

lib/SILOptimizer/ARC/RefCountState.cpp

@@ -285,9 +285,6 @@ handleRefCountInstMatch(SILInstruction *RefCountInst) {
     return false;
   case LatticeState::Decremented:
   case LatticeState::MightBeUsed:
-    // Unset InsertPt so we remove retain release pairs instead of
-    // performing code motion.
-    LLVM_FALLTHROUGH;
   case LatticeState::MightBeDecremented:
     return true;
   }

lib/SILOptimizer/ARC/RefCountState.h

@@ -185,17 +185,13 @@ class BottomUpRefCountState : public RefCountState {
   bool initWithMutatorInst(ImmutablePointerSet<SILInstruction> *I,
                            RCIdentityFunctionInfo *RCFI);
 
-  /// Update this reference count's state given the instruction \p I. \p
-  /// InsertPt is the point furthest up the CFG where we can move the currently
-  /// tracked reference count.
+  /// Update this reference count's state given the instruction \p I.
   void
   updateForSameLoopInst(SILInstruction *I,
                         ImmutablePointerSetFactory<SILInstruction> &SetFactory,
                         AliasAnalysis *AA);
 
-  /// Update this reference count's state given the instruction \p I. \p
-  /// InsertPts are the points furthest up the CFG where we can move the
-  /// currently tracked reference count.
+  /// Update this reference count's state given the instruction \p I.
   //
   /// The main difference in between this routine and update for same loop inst
   /// is that if we see any decrements on a value, we treat it as being
@@ -244,9 +240,7 @@ class BottomUpRefCountState : public RefCountState {
   bool valueCanBeUsedGivenLatticeState() const;
 
   /// Given the current lattice state, if we have seen a use, advance the
-  /// lattice state. Return true if we do so and false otherwise. \p InsertPt is
-  /// the location where if \p PotentialUser is a user of this ref count, we
-  /// would insert a release.
+  /// lattice state. Return true if we do so and false otherwise.
   bool handleUser(SILValue RCIdentity,
                   ImmutablePointerSetFactory<SILInstruction> &SetFactory,
                   AliasAnalysis *AA);
@@ -264,9 +258,7 @@ class BottomUpRefCountState : public RefCountState {
   bool valueCanBeGuaranteedUsedGivenLatticeState() const;
 
   /// Given the current lattice state, if we have seen a use, advance the
-  /// lattice state. Return true if we do so and false otherwise. \p InsertPt is
-  /// the location where if \p PotentialUser is a user of this ref count, we
-  /// would insert a release.
+  /// lattice state. Return true if we do so and false otherwise.
   bool
   handleGuaranteedUser(SILValue RCIdentity,
                        ImmutablePointerSetFactory<SILInstruction> &SetFactory,
@@ -338,17 +330,13 @@ class TopDownRefCountState : public RefCountState {
   /// Uninitialize the current state.
   void clear();
 
-  /// Update this reference count's state given the instruction \p I. \p
-  /// InsertPt is the point furthest up the CFG where we can move the currently
-  /// tracked reference count.
+  /// Update this reference count's state given the instruction \p I.
   void
   updateForSameLoopInst(SILInstruction *I,
                         ImmutablePointerSetFactory<SILInstruction> &SetFactory,
                         AliasAnalysis *AA);
 
-  /// Update this reference count's state given the instruction \p I. \p
-  /// InsertPts are the points furthest up the CFG where we can move the
-  /// currently tracked reference count.
+  /// Update this reference count's state given the instruction \p I.
   ///
   /// The main difference in between this routine and update for same loop inst
   /// is that if we see any decrements on a value, we treat it as being

lib/SILOptimizer/Analysis/LoopRegionAnalysis.cpp

@@ -45,8 +45,8 @@ LoopRegion::FunctionTy *LoopRegion::getFunction() const {
   return Ptr.get<FunctionTy *>();
 }
 
-void LoopRegion::dump() const {
-  print(llvm::outs());
+void LoopRegion::dump(bool isVerbose) const {
+  print(llvm::outs(), false, isVerbose);
   llvm::outs() << "\n";
 }
 
@@ -69,7 +69,8 @@ void LoopRegion::printName(llvm::raw_ostream &os) const {
   return;
 }
 
-void LoopRegion::print(llvm::raw_ostream &os, bool isShort) const {
+void LoopRegion::print(llvm::raw_ostream &os, bool isShort,
+                       bool isVerbose) const {
   os << "(region id:" << ID;
   if (isShort) {
     os << ")";
@@ -88,6 +89,20 @@ void LoopRegion::print(llvm::raw_ostream &os, bool isShort) const {
 
   os << " ucfh:" << (IsUnknownControlFlowEdgeHead? "true " : "false")
      << " ucft:" << (IsUnknownControlFlowEdgeTail? "true " : "false");
+
+  if (!isVerbose) {
+    return;
+  }
+  os << "\n";
+  if (isBlock()) {
+    getBlock()->dump();
+  } else if (isLoop()) {
+    getLoop()->dump();
+  } else if (isFunction()) {
+    getFunction()->dump();
+  } else {
+    llvm_unreachable("Unknown region type");
+  }
 }
 
 llvm::raw_ostream &llvm::operator<<(llvm::raw_ostream &os, LoopRegion &LR) {