Fix ARCSequenceOpts bottom up handling of terminators

lib/SILOptimizer/ARC/ARCMatchingSet.cpp

@@ -68,8 +68,6 @@ ARCMatchingSetBuilder::matchIncrementsToDecrements() {
       continue;
     }
 
-    // We need to be known safe over all increments/decrements we are matching
-    // up to ignore insertion points.
     bool BUIsKnownSafe = (*BURefCountState)->second.isKnownSafe();
     LLVM_DEBUG(llvm::dbgs() << "        BOTTOM UP KNOWNSAFE: "
                             << (BUIsKnownSafe ? "true" : "false") << "\n");
@@ -152,8 +150,6 @@ ARCMatchingSetBuilder::matchDecrementsToIncrements() {
       continue;
     }
 
-    // We need to be known safe over all increments/decrements we are matching
-    // up to ignore insertion points.
     bool TDIsKnownSafe = (*TDRefCountState)->second.isKnownSafe();
     LLVM_DEBUG(llvm::dbgs() << "        TOP DOWN KNOWNSAFE: "
                             << (TDIsKnownSafe ? "true" : "false") << "\n");
@@ -223,7 +219,7 @@ bool ARCMatchingSetBuilder::matchUpIncDecSetsForPtr() {
     LLVM_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.
     auto Result = matchIncrementsToDecrements();
     if (!Result) {
       LLVM_DEBUG(llvm::dbgs() << "    FAILED TO MATCH INCREMENTS -> "
@@ -287,8 +283,6 @@ bool ARCMatchingSetBuilder::matchUpIncDecSetsForPtr() {
   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 (!MatchSet.Increments.empty())
     MatchedPair = true;
 

lib/SILOptimizer/ARC/ARCRegionState.cpp

@@ -12,6 +12,7 @@
 
 #define DEBUG_TYPE "arc-sequence-opts"
 #include "ARCRegionState.h"
+#include "ARCSequenceOptUtils.h"
 #include "RCStateTransitionVisitors.h"
 #include "swift/Basic/Range.h"
 #include "swift/SILOptimizer/Analysis/LoopRegionAnalysis.h"
@@ -155,77 +156,6 @@ void ARCRegionState::mergePredTopDown(ARCRegionState &PredRegionState) {
 // Bottom Up Dataflow
 //
 
-static bool isARCSignificantTerminator(TermInst *TI) {
-  switch (TI->getTermKind()) {
-  case TermKind::UnreachableInst:
-  // br is a forwarding use for its arguments. It cannot in of itself extend
-  // the lifetime of an object (just like a phi-node) cannot.
-  case TermKind::BranchInst:
-  // A cond_br is a forwarding use for its non-operand arguments in a similar
-  // way to br. Its operand must be an i1 that has a different lifetime from any
-  // ref counted object.
-  case TermKind::CondBranchInst:
-    return false;
-  // Be conservative for now. These actually perform some sort of operation
-  // against the operand or can use the value in some way.
-  case TermKind::ThrowInst:
-  case TermKind::ReturnInst:
-  case TermKind::UnwindInst:
-  case TermKind::YieldInst:
-  case TermKind::TryApplyInst:
-  case TermKind::SwitchValueInst:
-  case TermKind::SwitchEnumInst:
-  case TermKind::SwitchEnumAddrInst:
-  case TermKind::DynamicMethodBranchInst:
-  case TermKind::CheckedCastBranchInst:
-  case TermKind::CheckedCastValueBranchInst:
-  case TermKind::CheckedCastAddrBranchInst:
-    return true;
-  }
-
-  llvm_unreachable("Unhandled TermKind in switch.");
-}
-
-// Visit each one of our predecessor regions and see if any are blocks that can
-// use reference counted values. If any of them do, we advance the sequence for
-// the pointer and create an insertion point here. This state will be propagated
-// into all of our predecessors, allowing us to be conservatively correct in all
-// cases.
-//
-// The key thing to notice is that in general this cannot happen due to
-// critical edge splitting. To trigger this, one would need a terminator that
-// uses a reference counted value and only has one successor due to critical
-// edge splitting. This is just to be conservative when faced with the unknown
-// of future changes.
-//
-// We do not need to worry about loops here, since a loop exit block can only
-// have predecessors in the loop itself implying that loop exit blocks at the
-// loop region level always have only one predecessor, the loop itself.
-void ARCRegionState::processBlockBottomUpPredTerminators(
-    const LoopRegion *R, AliasAnalysis *AA, LoopRegionFunctionInfo *LRFI,
-    ImmutablePointerSetFactory<SILInstruction> &SetFactory) {
-  llvm::TinyPtrVector<SILInstruction *> PredTerminators;
-  for (unsigned PredID : R->getPreds()) {
-    auto *PredRegion = LRFI->getRegion(PredID);
-    if (!PredRegion->isBlock())
-      continue;
-
-    auto *TermInst = PredRegion->getBlock()->getTerminator();
-    if (!isARCSignificantTerminator(TermInst))
-      continue;
-    PredTerminators.push_back(TermInst);
-  }
-
-  for (auto &OtherState : getBottomupStates()) {
-    // If the other state's value is blotted, skip it.
-    if (!OtherState.hasValue())
-      continue;
-
-    OtherState->second.updateForPredTerminators(PredTerminators,
-                                                SetFactory, AA);
-  }
-}
-
 static bool processBlockBottomUpInsts(
     ARCRegionState &State, SILBasicBlock &BB,
     BottomUpDataflowRCStateVisitor<ARCRegionState> &DataflowVisitor,
@@ -239,9 +169,9 @@ static bool processBlockBottomUpInsts(
   if (II == IE)
     return false;
 
-  // If II is the terminator, skip it since our terminator was already processed
-  // in our successors.
-  if (*II == BB.getTerminator())
+  // If II is not an arc significant terminator, skip it.
+  if (*II == BB.getTerminator() &&
+      !isARCSignificantTerminator(cast<TermInst>(*II)))
     ++II;
 
   bool NestingDetected = false;
@@ -298,18 +228,10 @@ bool ARCRegionState::processBlockBottomUp(
       RCIA, EAFI, *this, FreezeOwnedArgEpilogueReleases, IncToDecStateMap,
       SetFactory);
 
-  // Visit each non-terminator arc relevant instruction I in BB visited in
-  // reverse...
+  // Visit each arc relevant instruction I in BB visited in reverse...
   bool NestingDetected =
       processBlockBottomUpInsts(*this, BB, DataflowVisitor, AA, SetFactory);
 
-  // Now visit each one of our predecessor regions and see if any are blocks
-  // that can use reference counted values. If any of them do, we advance the
-  // sequence for the pointer and create an insertion point here. This state
-  // will be propagated into all of our predecessors, allowing us to be
-  // conservatively correct in all cases.
-  processBlockBottomUpPredTerminators(R, AA, LRFI, SetFactory);
-
   return NestingDetected;
 }
 

lib/SILOptimizer/ARC/ARCSequenceOptUtils.cpp

@@ -0,0 +1,48 @@
+//===--- ARCSequenceOptUtils.cpp - ARCSequenceOpt utilities ------------===//
+//
+// This source file is part of the Swift.org open source project
+//
+// Copyright (c) 2014 - 2020 Apple Inc. and the Swift project authors
+// Licensed under Apache License v2.0 with Runtime Library Exception
+//
+// See https://swift.org/LICENSE.txt for license information
+// See https://swift.org/CONTRIBUTORS.txt for the list of Swift project authors
+//
+//===----------------------------------------------------------------------===//
+
+#include "ARCSequenceOptUtils.h"
+
+using namespace swift;
+namespace swift {
+bool isARCSignificantTerminator(TermInst *TI) {
+  switch (TI->getTermKind()) {
+  case TermKind::UnreachableInst:
+  // br is a forwarding use for its arguments. It cannot in of itself extend
+  // the lifetime of an object (just like a phi-node) cannot.
+  case TermKind::BranchInst:
+  // A cond_br is a forwarding use for its non-operand arguments in a similar
+  // way to br. Its operand must be an i1 that has a different lifetime from any
+  // ref counted object.
+  case TermKind::CondBranchInst:
+    return false;
+  // Be conservative for now. These actually perform some sort of operation
+  // against the operand or can use the value in some way.
+  case TermKind::ThrowInst:
+  case TermKind::ReturnInst:
+  case TermKind::UnwindInst:
+  case TermKind::YieldInst:
+  case TermKind::TryApplyInst:
+  case TermKind::SwitchValueInst:
+  case TermKind::SwitchEnumInst:
+  case TermKind::SwitchEnumAddrInst:
+  case TermKind::DynamicMethodBranchInst:
+  case TermKind::CheckedCastBranchInst:
+  case TermKind::CheckedCastValueBranchInst:
+  case TermKind::CheckedCastAddrBranchInst:
+    return true;
+  }
+
+  llvm_unreachable("Unhandled TermKind in switch.");
+}
+
+} // end namespace 'swift'

lib/SILOptimizer/ARC/ARCSequenceOptUtils.h

@@ -0,0 +1,27 @@
+//===--- ARCSequenceOptUtils.h - ARCSequenceOpts utilities ----*- C++ -*-===//
+//
+// This source file is part of the Swift.org open source project
+//
+// Copyright (c) 2014 - 2020 Apple Inc. and the Swift project authors
+// Licensed under Apache License v2.0 with Runtime Library Exception
+//
+// See https://swift.org/LICENSE.txt for license information
+// See https://swift.org/CONTRIBUTORS.txt for the list of Swift project authors
+//
+//===----------------------------------------------------------------------===//
+///
+/// Utilities used by the ARCSequenceOpts for analyzing and transforming
+/// SILInstructions.
+///
+//===----------------------------------------------------------------------===//
+
+#ifndef SWIFT_SILOPTIMIZER_ARC_ARCSEQUENCEOPTUTILS_H
+#define SWIFT_SILOPTIMIZER_ARC_ARCSEQUENCEOPTUTILS_H
+
+#include "swift/SIL/SILInstruction.h"
+
+namespace swift {
+  bool isARCSignificantTerminator(TermInst *TI);
+} // end namespace swift
+
+#endif

lib/SILOptimizer/ARC/ARCSequenceOpts.cpp

@@ -45,10 +45,8 @@ llvm::cl::opt<bool> EnableLoopARC("enable-loop-arc", llvm::cl::init(false));
 //                                Code Motion
 //===----------------------------------------------------------------------===//
 
-// This routine takes in the ARCMatchingSet \p MatchSet and inserts new
-// increments, decrements at the insertion points and adds the old increment,
-// decrements to the delete list. Sets changed to true if anything was moved or
-// deleted.
+// 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,
     llvm::SmallVectorImpl<SILInstruction *> &DeadInsts) {
@@ -99,9 +97,6 @@ bool ARCPairingContext::performMatching(
       for (auto *I : Set.Decrements)
         DecToIncStateMap.erase(I);
 
-      // Add the Set to the callback. *NOTE* No instruction destruction can
-      // happen here since we may remove instructions that are insertion points
-      // for other instructions.
       optimizeMatchingSet(Set, NewInsts, DeadInsts);
     }
   }

lib/SILOptimizer/ARC/CMakeLists.txt

@@ -4,6 +4,7 @@ target_sources(swiftSILOptimizer PRIVATE
   ARCMatchingSet.cpp
   ARCRegionState.cpp
   ARCSequenceOpts.cpp
+  ARCSequenceOptUtils.cpp
   GlobalARCSequenceDataflow.cpp
   GlobalLoopARCSequenceDataflow.cpp
   RCStateTransition.cpp

lib/SILOptimizer/ARC/GlobalARCSequenceDataflow.cpp

@@ -13,6 +13,7 @@
 #define DEBUG_TYPE "arc-sequence-opts"
 #include "GlobalARCSequenceDataflow.h"
 #include "ARCBBState.h"
+#include "ARCSequenceOptUtils.h"
 #include "RCStateTransitionVisitors.h"
 #include "swift/SILOptimizer/Analysis/ARCAnalysis.h"
 #include "swift/SILOptimizer/Analysis/PostOrderAnalysis.h"
@@ -195,39 +196,6 @@ bool ARCSequenceDataflowEvaluator::processTopDown() {
 //                             Bottom Up Dataflow
 //===----------------------------------------------------------------------===//
 
-// This is temporary code duplication. This will be removed when Loop ARC is
-// finished and Block ARC is removed.
-static bool isARCSignificantTerminator(TermInst *TI) {
-  switch (TI->getTermKind()) {
-  case TermKind::UnreachableInst:
-  // br is a forwarding use for its arguments. It cannot in of itself extend
-  // the lifetime of an object (just like a phi-node) cannot.
-  case TermKind::BranchInst:
-  // A cond_br is a forwarding use for its non-operand arguments in a similar
-  // way to br. Its operand must be an i1 that has a different lifetime from any
-  // ref counted object.
-  case TermKind::CondBranchInst:
-    return false;
-  // Be conservative for now. These actually perform some sort of operation
-  // against the operand or can use the value in some way.
-  case TermKind::ThrowInst:
-  case TermKind::ReturnInst:
-  case TermKind::UnwindInst:
-  case TermKind::YieldInst:
-  case TermKind::TryApplyInst:
-  case TermKind::SwitchValueInst:
-  case TermKind::SwitchEnumInst:
-  case TermKind::SwitchEnumAddrInst:
-  case TermKind::DynamicMethodBranchInst:
-  case TermKind::CheckedCastBranchInst:
-  case TermKind::CheckedCastValueBranchInst:
-  case TermKind::CheckedCastAddrBranchInst:
-    return true;
-  }
-
-  llvm_unreachable("Unhandled TermKind in switch.");
-}
-
 /// Analyze a single BB for refcount inc/dec instructions.
 ///
 /// If anything was found it will be added to DecToIncStateMap.
@@ -254,8 +222,15 @@ bool ARCSequenceDataflowEvaluator::processBBBottomUp(
       RCIA, EAFI, BBState, FreezeOwnedArgEpilogueReleases, IncToDecStateMap,
       SetFactory);
 
-  // For each terminator instruction I in BB visited in reverse...
-  for (auto II = std::next(BB.rbegin()), IE = BB.rend(); II != IE;) {
+  auto II = BB.rbegin();
+  if (isa<TermInst>(*II)) {
+    if (!isARCSignificantTerminator(&cast<TermInst>(*II))) {
+      II++;
+    }
+  }
+
+  // For each instruction I in BB visited in reverse...
+  for (auto IE = BB.rend(); II != IE;) {
     SILInstruction &I = *II;
     ++II;
 
@@ -292,31 +267,6 @@ bool ARCSequenceDataflowEvaluator::processBBBottomUp(
     }
   }
 
-  // This is ignoring the possibility that we may have a loop with an
-  // interesting terminator but for which, we are going to clear all state
-  // (since it is a loop boundary). We may in such a case, be too conservative
-  // with our other predecessors. Luckily this cannot happen since cond_br is
-  // the only terminator that allows for critical edges and all other
-  // "interesting terminators" always having multiple successors. This means
-  // that this block could not have multiple predecessors since otherwise, the
-  // edge would be broken.
-  llvm::TinyPtrVector<SILInstruction *> PredTerminators;
-  for (SILBasicBlock *PredBB : BB.getPredecessorBlocks()) {
-    auto *TermInst = PredBB->getTerminator();
-    if (!isARCSignificantTerminator(TermInst))
-      continue;
-    PredTerminators.push_back(TermInst);
-  }
-
-  for (auto &OtherState : BBState.getBottomupStates()) {
-    // If the other state's value is blotted, skip it.
-    if (!OtherState.hasValue())
-      continue;
-
-    OtherState->second.updateForPredTerminators(PredTerminators,
-                                                SetFactory, AA);
-  }
-
   return NestingDetected;
 }