Remove updateForPredTerminators in ARCSequenceOpts

Historically TermInsts were handled specially while visiting blocks in
bottom up order. TermInsts were not visited while traversing a block.
They were visited while traversing successors, and the most conservative
terminator of all predecessors would affect the refcount state in the
dataflow.

This was needed because ARCSequenceOpts also computed 'insertion points'
for the purposes of ARC code motion. ARC code motion was then removed
from ARCSequenceOpts and this code remained unchanged.

With this change, arc significant terminators are handled like all other
instructions while processing basic blocks bottom up.

Also updateForSameLoopInst, updateForDifferentLoopInst,
updateForPredTerminators all serve similar purpose with subtle differences.
This change removes some code duplication due to this.

Remove code duplication of isARCSignificantTerminator. Create
ARCSequenceOptUtils for common utils used in ARCSequenceOpts

Author: meg-gupta

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. 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,17 +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. 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/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;
 }
 

lib/SILOptimizer/ARC/RefCountState.cpp

@@ -500,40 +500,6 @@ void BottomUpRefCountState::updateForDifferentLoopInst(
                           << getRCRoot());
 }
 
-void BottomUpRefCountState::updateForPredTerminators(
-    ArrayRef<SILInstruction *> Terms,
-    ImmutablePointerSetFactory<SILInstruction> &SetFactory, AliasAnalysis *AA) {
-  // If this state is not tracking anything, there is nothing to update.
-  if (!isTrackingRefCount())
-    return;
-
-  if (valueCanBeGuaranteedUsedGivenLatticeState() &&
-      std::any_of(Terms.begin(), Terms.end(),
-                  [this, &AA](SILInstruction *I) -> bool {
-                    return mayGuaranteedUseValue(I, getRCRoot(), AA);
-                  })) {
-    handleGuaranteedUser(getRCRoot(), SetFactory, AA);
-    return;
-  }
-
-  if (valueCanBeDecrementedGivenLatticeState() &&
-      std::any_of(Terms.begin(), Terms.end(),
-                  [this, &AA](SILInstruction *I) -> bool {
-                    return mayDecrementRefCount(I, getRCRoot(), AA);
-                  })) {
-    handleDecrement();
-    return;
-  }
-
-  if (!valueCanBeUsedGivenLatticeState() ||
-      std::none_of(Terms.begin(), Terms.end(),
-                   [this, &AA](SILInstruction *I)
-       -> bool { return mayHaveSymmetricInterference(I, getRCRoot(), AA); }))
-    return;
-
-  handleUser(getRCRoot(), SetFactory, AA);
-}
-
 //===----------------------------------------------------------------------===//
 //                          Top Down Ref Count State
 //===----------------------------------------------------------------------===//

lib/SILOptimizer/ARC/RefCountState.h

@@ -205,13 +205,6 @@ class BottomUpRefCountState : public RefCountState {
       ImmutablePointerSetFactory<SILInstruction> &SetFactory,
       AliasAnalysis *AA);
 
-  // Determine the conservative effect of the given list of predecessor
-  // terminators upon this reference count.
-  void updateForPredTerminators(
-      ArrayRef<SILInstruction *> PredTerms,
-      ImmutablePointerSetFactory<SILInstruction> &SetFactory,
-      AliasAnalysis *AA);
-
   /// Attempt to merge \p Other into this ref count state. Return true if we
   /// succeed and false otherwise.
   bool merge(const BottomUpRefCountState &Other);