Merge pull request #2391 from trentxintong/InsertPoint

Remove code motion in ASO.

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;