Remove code motion in ASO. This code has been disabled for sometime.

As promised, we separate the duty of moving retain release pairs with the
task of removing them. Now the task of moving retains and releases are in
Retain Release Code Motion committed in 51b1c0b.

Author: trentxintong

lib/SILOptimizer/ARC/ARCBBState.cpp

@@ -127,9 +127,6 @@ void ARCBBState::mergePredTopDown(ARCBBState &PredBBState) {
       PtrToTopDownState.blot(RefCountedValue);
       continue;
     }
-
-    DEBUG(llvm::dbgs() << "            Partial: "
-                       << (RefCountState.isPartial() ? "yes" : "no") << "\n");
   }
 }
 

lib/SILOptimizer/ARC/ARCMatchingSet.cpp

@@ -33,8 +33,6 @@
 
 using namespace swift;
 
-llvm::cl::opt<bool> DisableARCRRMotion("disable-code-motion-arc", llvm::cl::init(true));
-
 //===----------------------------------------------------------------------===//
 //                          ARC Matching Set Builder
 //===----------------------------------------------------------------------===//
@@ -43,7 +41,7 @@ llvm::cl::opt<bool> DisableARCRRMotion("disable-code-motion-arc", llvm::cl::init
 /// were known safe.
 Optional<MatchingSetFlags>
 ARCMatchingSetBuilder::matchIncrementsToDecrements() {
-  MatchingSetFlags Flags = {true, false};
+  MatchingSetFlags Flags = {true, true};
 
   // For each increment in our list of new increments.
   //
@@ -70,19 +68,16 @@ ARCMatchingSetBuilder::matchIncrementsToDecrements() {
     }
 
     // We need to be known safe over all increments/decrements we are matching
-    // up
-    // to ignore insertion points.
+    // up to ignore insertion points.
     bool BUIsKnownSafe = (*BURefCountState)->second.isKnownSafe();
     DEBUG(llvm::dbgs() << "        KNOWNSAFE: "
                        << (BUIsKnownSafe ? "true" : "false") << "\n");
     Flags.KnownSafe &= BUIsKnownSafe;
 
-    // We can only move instructions if we know that we are not partial. We can
-    // still delete instructions in such cases though.
-    bool BUIsPartial = (*BURefCountState)->second.isPartial();
-    DEBUG(llvm::dbgs() << "        PARTIAL: "
-                       << (BUIsPartial ? "true" : "false") << "\n");
-    Flags.Partial |= BUIsPartial;
+    bool BUCodeMotionSafe = (*BURefCountState)->second.isCodeMotionSafe();
+    DEBUG(llvm::dbgs() << "        KNOWNSAFE: "
+                       << (BUIsKnownSafe ? "true" : "false") << "\n");
+    Flags.CodeMotionSafe &= BUCodeMotionSafe;
 
     // Now that we know we have an inst, grab the decrement.
     for (auto DecIter : (*BURefCountState)->second.getInstructions()) {
@@ -103,8 +98,7 @@ ARCMatchingSetBuilder::matchIncrementsToDecrements() {
                             "decrement.\n");
 
       // Make sure the increment we are looking at is also matched to our
-      // decrement.
-      // Otherwise bail.
+      // decrement. Otherwise bail.
       if (!(*TDRefCountState)->second.isTrackingRefCountInst() ||
           !(*TDRefCountState)->second.containsInstruction(Increment)) {
         DEBUG(
@@ -121,10 +115,6 @@ ARCMatchingSetBuilder::matchIncrementsToDecrements() {
         continue;
       }
 
-      // Collect the increment insertion point if it has one.
-      for (auto InsertPt : (*TDRefCountState)->second.getInsertPts()) {
-        MatchSet.IncrementInsertPts.insert(InsertPt);
-      }
       NewDecrements.push_back(Decrement);
     }
   }
@@ -134,7 +124,7 @@ ARCMatchingSetBuilder::matchIncrementsToDecrements() {
 
 Optional<MatchingSetFlags>
 ARCMatchingSetBuilder::matchDecrementsToIncrements() {
-  MatchingSetFlags Flags = {true, false};
+  MatchingSetFlags Flags = {true, true};
 
   // For each increment in our list of new increments.
   //
@@ -161,19 +151,16 @@ ARCMatchingSetBuilder::matchDecrementsToIncrements() {
     }
 
     // We need to be known safe over all increments/decrements we are matching
-    // up
-    // to ignore insertion points.
+    // up to ignore insertion points.
     bool TDIsKnownSafe = (*TDRefCountState)->second.isKnownSafe();
     DEBUG(llvm::dbgs() << "        KNOWNSAFE: "
                        << (TDIsKnownSafe ? "true" : "false") << "\n");
     Flags.KnownSafe &= TDIsKnownSafe;
 
-    // We can only move instructions if we know that we are not partial. We can
-    // still delete instructions in such cases though.
-    bool TDIsPartial = (*TDRefCountState)->second.isPartial();
-    DEBUG(llvm::dbgs() << "        PARTIAL: "
-                       << (TDIsPartial ? "true" : "false") << "\n");
-    Flags.Partial |= TDIsPartial;
+    bool TDCodeMotionSafe = (*TDRefCountState)->second.isCodeMotionSafe();
+    DEBUG(llvm::dbgs() << "        KNOWNSAFE: "
+                       << (TDIsKnownSafe ? "true" : "false") << "\n");
+    Flags.CodeMotionSafe &= TDCodeMotionSafe;
 
     // Now that we know we have an inst, grab the decrement.
     for (auto IncIter : (*TDRefCountState)->second.getInstructions()) {
@@ -213,10 +200,6 @@ ARCMatchingSetBuilder::matchDecrementsToIncrements() {
         continue;
       }
 
-      // Collect the decrement insertion point if we have one.
-      for (auto InsertPtIter : (*BURefCountState)->second.getInsertPts()) {
-        MatchSet.DecrementInsertPts.insert(InsertPtIter);
-      }
       NewIncrements.push_back(Increment);
     }
   }
@@ -231,13 +214,13 @@ ARCMatchingSetBuilder::matchDecrementsToIncrements() {
 bool ARCMatchingSetBuilder::matchUpIncDecSetsForPtr() {
   bool KnownSafeTD = true;
   bool KnownSafeBU = true;
-  bool Partial = false;
+  bool CodeMotionSafeTD = true;
+  bool CodeMotionSafeBU = true;
 
   while (true) {
     DEBUG(llvm::dbgs() << "Attempting to match up increments -> decrements:\n");
     // For each increment in our list of new increments, attempt to match them
-    // up
-    // with decrements and gather the insertion points of the decrements.
+    // up with decrements and gather the insertion points of the decrements.
     auto Result = matchIncrementsToDecrements();
     if (!Result) {
       DEBUG(llvm::dbgs() << "    FAILED TO MATCH INCREMENTS -> DECREMENTS!\n");
@@ -247,10 +230,11 @@ bool ARCMatchingSetBuilder::matchUpIncDecSetsForPtr() {
       DEBUG(llvm::dbgs() << "    NOT KNOWN SAFE!\n");
       KnownSafeTD = false;
     }
-    if (Result->Partial) {
-      DEBUG(llvm::dbgs() << "    IS PARTIAL!\n");
-      Partial = true;
+    if (!Result->CodeMotionSafe) {
+      DEBUG(llvm::dbgs() << "    NOT CODE MOTION SAFE!\n");
+      CodeMotionSafeTD = false;
     }
+      
     NewIncrements.clear();
 
     // If we do not have any decrements to attempt to match up with, bail.
@@ -267,9 +251,9 @@ bool ARCMatchingSetBuilder::matchUpIncDecSetsForPtr() {
       DEBUG(llvm::dbgs() << "    NOT KNOWN SAFE!\n");
       KnownSafeBU = false;
     }
-    if (Result->Partial) {
-      DEBUG(llvm::dbgs() << "    IS PARTIAL!\n");
-      Partial = true;
+    if (!Result->CodeMotionSafe) {
+      DEBUG(llvm::dbgs() << "    NOT CODE MOTION SAFE!\n");
+      CodeMotionSafeBU = false;
     }
     NewDecrements.clear();
 
@@ -278,42 +262,29 @@ bool ARCMatchingSetBuilder::matchUpIncDecSetsForPtr() {
       break;
   }
 
+  // There is no way we can get a top-down code motion but not a bottom-up, or vice
+  // versa.
+  assert(CodeMotionSafeTD == CodeMotionSafeBU && "Asymmetric code motion safety");
+
   bool UnconditionallySafe = (KnownSafeTD && KnownSafeBU);
-  if (UnconditionallySafe) {
-    DEBUG(llvm::dbgs() << "UNCONDITIONALLY SAFE! DELETING INSTS.\n");
-    MatchSet.IncrementInsertPts.clear();
-    MatchSet.DecrementInsertPts.clear();
+  bool CodeMotionSafe = (CodeMotionSafeTD && CodeMotionSafeBU);
+  if (UnconditionallySafe || CodeMotionSafe) {
+    DEBUG(llvm::dbgs() << "UNCONDITIONALLY OR CODE MOTION SAFE! DELETING INSTS.\n");
   } else {
-    DEBUG(llvm::dbgs() << "NOT UNCONDITIONALLY SAFE!\n");
-  }
-
-  bool HaveIncInsertPts = !MatchSet.IncrementInsertPts.empty();
-  bool HaveDecInsertPts = !MatchSet.DecrementInsertPts.empty();
-
-  // We should not have the case which retains have to be anchored topdown and
-  // releases do not have to be bottomup, or vice-versa.
-  assert(HaveIncInsertPts == HaveDecInsertPts &&
-         "Asymmetric insertion points for retains and releases");
-
-  bool CodeMotionBlocked = HaveIncInsertPts || HaveDecInsertPts;
-  if (DisableARCRRMotion && CodeMotionBlocked && !UnconditionallySafe) {
-    DEBUG(llvm::dbgs() << "Code motion blocked. Bailing!\n");
+    DEBUG(llvm::dbgs() << "NOT UNCONDITIONALLY SAFE AND CODE MOTION BLOCKED!\n");
     return false;
   }
 
-  // If we have insertion points and partial merges, return false to avoid
-  // control dependency issues.
-  if ((HaveIncInsertPts || HaveDecInsertPts) && Partial) {
-    DEBUG(llvm::dbgs() << "Found partial merge and insert pts. Bailing!\n");
-    return false;
-  }
+  // Make sure we always have increments and decrements in the match set.
+  assert(MatchSet.Increments.empty() == MatchSet.Decrements.empty() &&
+         "Match set without increments or decrements");
 
   // If we do not have any insertion points but we do have increments, we must
   // be eliminating pairs.
-  if (!HaveIncInsertPts && !MatchSet.Increments.empty())
+  if (!MatchSet.Increments.empty())
     MatchedPair = true;
 
   // Success!
-  DEBUG(llvm::dbgs() << "SUCCESS! We can move, remove things.\n");
+  DEBUG(llvm::dbgs() << "SUCCESS! We can remove things.\n");
   return true;
 }

lib/SILOptimizer/ARC/ARCMatchingSet.h

@@ -43,21 +43,9 @@ struct ARCMatchingSet {
   /// The set of reference count increments that were paired.
   llvm::SetVector<SILInstruction *> Increments;
 
-  /// An insertion point for an increment means the earliest point in the
-  /// program after the increment has occurred that the increment can be moved
-  /// to
-  /// without moving the increment over an instruction that may decrement a
-  /// reference count.
-  llvm::SetVector<SILInstruction *> IncrementInsertPts;
-
   /// The set of reference count decrements that were paired.
   llvm::SetVector<SILInstruction *> Decrements;
 
-  /// An insertion point for a decrement means the latest point in the program
-  /// before the decrement that the optimizer conservatively assumes that a
-  /// reference counted value could be used.
-  llvm::SetVector<SILInstruction *> DecrementInsertPts;
-
   // This is a data structure that cannot be moved or copied.
   ARCMatchingSet() = default;
   ARCMatchingSet(const ARCMatchingSet &) = delete;
@@ -68,15 +56,13 @@ struct ARCMatchingSet {
   void clear() {
     Ptr = SILValue();
     Increments.clear();
-    IncrementInsertPts.clear();
     Decrements.clear();
-    DecrementInsertPts.clear();
   }
 };
 
 struct MatchingSetFlags {
   bool KnownSafe;
-  bool Partial;
+  bool CodeMotionSafe;
 };
 static_assert(std::is_pod<MatchingSetFlags>::value,
               "MatchingSetFlags should be a pod.");

lib/SILOptimizer/ARC/ARCRegionState.cpp

@@ -148,9 +148,6 @@ void ARCRegionState::mergePredTopDown(ARCRegionState &PredRegionState) {
       PtrToTopDownState.blot(RefCountedValue);
       continue;
     }
-
-    DEBUG(llvm::dbgs() << "            Partial: "
-                       << (RefCountState.isPartial() ? "yes" : "no") << "\n");
   }
 }
 
@@ -202,7 +199,6 @@ static bool isARCSignificantTerminator(TermInst *TI) {
 void ARCRegionState::processBlockBottomUpPredTerminators(
     const LoopRegion *R, AliasAnalysis *AA, LoopRegionFunctionInfo *LRFI,
     ImmutablePointerSetFactory<SILInstruction> &SetFactory) {
-  auto &BB = *R->getBlock();
   llvm::TinyPtrVector<SILInstruction *> PredTerminators;
   for (unsigned PredID : R->getPreds()) {
     auto *PredRegion = LRFI->getRegion(PredID);
@@ -215,13 +211,12 @@ void ARCRegionState::processBlockBottomUpPredTerminators(
     PredTerminators.push_back(TermInst);
   }
 
-  auto *InsertPt = &*BB.begin();
   for (auto &OtherState : getBottomupStates()) {
     // If the other state's value is blotted, skip it.
     if (!OtherState.hasValue())
       continue;
 
-    OtherState->second.updateForPredTerminators(PredTerminators, InsertPt,
+    OtherState->second.updateForPredTerminators(PredTerminators,
                                                 SetFactory, AA);
   }
 }
@@ -266,8 +261,6 @@ static bool processBlockBottomUpInsts(
     // that the instruction "visits".
     SILValue Op = Result.RCIdentity;
 
-    auto *InsertPt = &*std::next(I->getIterator());
-
     // For all other (reference counted value, ref count state) we are
     // tracking...
     for (auto &OtherState : State.getBottomupStates()) {
@@ -280,7 +273,7 @@ static bool processBlockBottomUpInsts(
       if (Op && OtherState->first == Op)
         continue;
 
-      OtherState->second.updateForSameLoopInst(I, InsertPt, SetFactory, AA);
+      OtherState->second.updateForSameLoopInst(I, SetFactory, AA);
     }
   }
 
@@ -372,8 +365,7 @@ bool ARCRegionState::processLoopBottomUp(
       continue;
 
     for (auto *I : State->getSummarizedInterestingInsts())
-      OtherState->second.updateForDifferentLoopInst(I, InsertPts, SetFactory,
-                                                    AA);
+      OtherState->second.updateForDifferentLoopInst(I, SetFactory, AA);
   }
 
   return false;
@@ -460,7 +452,7 @@ bool ARCRegionState::processBlockTopDown(
       if (Op && OtherState->first == Op)
         continue;
 
-      OtherState->second.updateForSameLoopInst(I, I, SetFactory, AA);
+      OtherState->second.updateForSameLoopInst(I, SetFactory, AA);
     }
   }
 
@@ -485,18 +477,14 @@ bool ARCRegionState::processLoopTopDown(
   assert(PredRegion->isBlock() && "Expected the predecessor region to be a "
                                   "block");
 
-  // Our insert point is going to be the terminator inst.
-  SILInstruction *InsertPt = PredRegion->getBlock()->getTerminator();
-
   // For each state that we are currently tracking, apply our summarized
   // instructions to it.
   for (auto &OtherState : getTopDownStates()) {
     if (!OtherState.hasValue())
       continue;
 
     for (auto *I : State->getSummarizedInterestingInsts())
-      OtherState->second.updateForDifferentLoopInst(I, InsertPt, SetFactory,
-                                                    AA);
+      OtherState->second.updateForDifferentLoopInst(I, SetFactory, AA);
   }
 
   return false;