[g-arc-opts] Allow removing of increment, decrement pairs after seeing partial merges, ensuring that we still will not move them.

This enables us to handle significantly crazier control flow without
needing to worry about control dependency issues relating to different
groups of insertion points potentially not being control dependent which
can cause incorrect behavior in the optimizer.

Swift SVN r18861

lib/SILAnalysis/GlobalARCPairingAnalysis.cpp

@@ -15,6 +15,7 @@
 #include "swift/Basic/BlotMapVector.h"
 #include "ReferenceCountState.h"
 #include "GlobalARCSequenceDataflow.h"
+#include "swift/Basic/Optional.h"
 #include "swift/Basic/Fallthrough.h"
 #include "swift/SIL/SILBuilder.h"
 #include "swift/SIL/SILVisitor.h"
@@ -34,7 +35,12 @@ using namespace swift::arc;
 
 namespace {
 
-enum class IncrementDecrementMatchResult { Fail, KnownSafe, NotKnownSafe };
+struct MatchingSetFlags {
+  bool KnownSafe;
+  bool Partial;
+};
+static_assert(std::is_pod<MatchingSetFlags>::value,
+              "MatchingSetFlags should be a pod.");
 
 struct ARCMatchingSetBuilder {
   using TDMapTy = BlotMapVector<SILInstruction *, TopDownRefCountState>;
@@ -74,17 +80,18 @@ public:
   bool matchedPair() const { return MatchedPair; }
 
 private:
-  IncrementDecrementMatchResult matchIncrementsToDecrements();
+  /// Returns .Some(MatchingSetFlags) on success and .None on failure.
-  IncrementDecrementMatchResult matchDecrementsToIncrements();
+  Optional<MatchingSetFlags> matchIncrementsToDecrements();
+  Optional<MatchingSetFlags> matchDecrementsToIncrements();
 };
 
 } // end anonymous namespace
 
 /// Match retains to releases and return whether or not all of the releases
 /// were known safe.
-IncrementDecrementMatchResult
+Optional<MatchingSetFlags>
 ARCMatchingSetBuilder::matchIncrementsToDecrements() {
-  bool KnownSafe = true;
+  MatchingSetFlags Flags = { true, false };
 
   // For each increment in our list of new increments.
   //
@@ -97,7 +104,7 @@ ARCMatchingSetBuilder::matchIncrementsToDecrements() {
     if (BURefCountState == BUMap.end()) {
       DEBUG(llvm::dbgs() << "        FAILURE! Could not find state for "
             "increment!\n");
-      return IncrementDecrementMatchResult::Fail;
+      return Nothing_t::Nothing;
     }
 
     DEBUG(llvm::dbgs() << "        SUCCESS! Found state for increment.\n");
@@ -112,7 +119,11 @@ ARCMatchingSetBuilder::matchIncrementsToDecrements() {
 
     // We need to be known safe over all increments/decrements we are matching up
     // to ignore insertion points.
-    KnownSafe &= BURefCountState->second.isKnownSafe();
+    Flags.KnownSafe &= BURefCountState->second.isKnownSafe();
+
+    // We can only move instructions if we know that we are not partial. We can
+    // still delete instructions in such cases though.
+    Flags.Partial |= BURefCountState->second.isPartial();
 
     // Now that we know we have an inst, grab the decrement.
     for (auto DecIter : BURefCountState->second.getInstructions()) {
@@ -125,7 +136,7 @@ ARCMatchingSetBuilder::matchIncrementsToDecrements() {
       if (TDRefCountState == TDMap.end()) {
         DEBUG(llvm::dbgs() << "        FAILURE! Could not find state for "
               "decrement.\n");
-        return IncrementDecrementMatchResult::Fail;
+        return Nothing_t::Nothing;
       }
       DEBUG(llvm::dbgs() << "        SUCCESS! Found state for decrement.\n");
 
@@ -135,7 +146,7 @@ ARCMatchingSetBuilder::matchIncrementsToDecrements() {
           !TDRefCountState->second.containsInstruction(Increment)) {
         DEBUG(llvm::dbgs() << "        FAILURE! Not tracking instruction or "
               "found increment that did not match.\n");
-        return IncrementDecrementMatchResult::Fail;
+        return Nothing_t::Nothing;
       }
 
       // Add the decrement to the decrement to move set. If we don't insert
@@ -153,15 +164,12 @@ ARCMatchingSetBuilder::matchIncrementsToDecrements() {
     }
   }
 
-  if (!KnownSafe)
+  return Flags;
-    return IncrementDecrementMatchResult::NotKnownSafe;
-
-  return IncrementDecrementMatchResult::KnownSafe;
 }
 
-IncrementDecrementMatchResult
+Optional<MatchingSetFlags>
 ARCMatchingSetBuilder::matchDecrementsToIncrements() {
-  bool KnownSafe = true;
+  MatchingSetFlags Flags = { true, false };
 
   // For each increment in our list of new increments.
   //
@@ -174,7 +182,7 @@ ARCMatchingSetBuilder::matchDecrementsToIncrements() {
     if (TDRefCountState == TDMap.end()) {
       DEBUG(llvm::dbgs() << "        FAILURE! Could not find state for "
             "increment!\n");
-      return IncrementDecrementMatchResult::Fail;
+      return Nothing_t::Nothing;
     }
 
     DEBUG(llvm::dbgs() << "        SUCCESS! Found state for decrement.\n");
@@ -189,7 +197,11 @@ ARCMatchingSetBuilder::matchDecrementsToIncrements() {
 
     // We need to be known safe over all increments/decrements we are matching up
     // to ignore insertion points.
-    KnownSafe &= TDRefCountState->second.isKnownSafe();
+    Flags.KnownSafe &= TDRefCountState->second.isKnownSafe();
+
+    // We can only move instructions if we know that we are not partial. We can
+    // still delete instructions in such cases though.
+    Flags.Partial |= TDRefCountState->second.isPartial();
 
     // Now that we know we have an inst, grab the decrement.
     for (auto IncIter : TDRefCountState->second.getInstructions()) {
@@ -202,7 +214,7 @@ ARCMatchingSetBuilder::matchDecrementsToIncrements() {
       if (BURefCountState == BUMap.end()) {
         DEBUG(llvm::dbgs() << "        FAILURE! Could not find state for "
               "increment.\n");
-        return IncrementDecrementMatchResult::Fail;
+        return Nothing_t::Nothing;
       }
 
       DEBUG(llvm::dbgs() << "        SUCCESS! Found state for increment.\n");
@@ -213,7 +225,7 @@ ARCMatchingSetBuilder::matchDecrementsToIncrements() {
           !BURefCountState->second.containsInstruction(Decrement)) {
         DEBUG(llvm::dbgs() << "        FAILURE! Not tracking instruction or "
               "found increment that did not match.\n");
-        return IncrementDecrementMatchResult::Fail;
+        return Nothing_t::Nothing;
       }
 
       // Add the decrement to the decrement to move set. If we don't insert
@@ -231,9 +243,7 @@ ARCMatchingSetBuilder::matchDecrementsToIncrements() {
     }
   }
 
-  if (!KnownSafe)
+  return Flags;
-    return IncrementDecrementMatchResult::NotKnownSafe;
-  return IncrementDecrementMatchResult::KnownSafe;
 }
 
 /// Visit each retain/release that is matched up to our operand over and over
@@ -243,20 +253,25 @@ ARCMatchingSetBuilder::matchDecrementsToIncrements() {
 bool ARCMatchingSetBuilder::matchUpIncDecSetsForPtr() {
   bool KnownSafeTD = true;
   bool KnownSafeBU = true;
+  bool Partial = false;
 
   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.
     auto Result = matchIncrementsToDecrements();
-    if (Result == IncrementDecrementMatchResult::Fail) {
+    if (!Result) {
       DEBUG(llvm::dbgs() << "    FAILED TO MATCH INCREMENTS -> DECREMENTS!\n");
       return false;
     }
-    if (Result == IncrementDecrementMatchResult::NotKnownSafe) {
+    if (!Result->KnownSafe) {
       DEBUG(llvm::dbgs() << "    NOT KNOWN SAFE!\n");
       KnownSafeTD = false;
     }
+    if (Result->Partial) {
+      DEBUG(llvm::dbgs() << "    IS PARTIAL!\n");
+      Partial = true;
+    }
     NewIncrements.clear();
 
     // If we do not have any decrements to attempt to match up with, bail.
@@ -265,14 +280,18 @@ bool ARCMatchingSetBuilder::matchUpIncDecSetsForPtr() {
 
     DEBUG(llvm::dbgs() << "Attempting to match up decrements -> increments:\n");
     Result = matchDecrementsToIncrements();
-    if (Result == IncrementDecrementMatchResult::Fail) {
+    if (!Result) {
       DEBUG(llvm::dbgs() << "    FAILED TO MATCH DECREMENTS -> INCREMENTS!\n");
       return false;
     }
-    if (Result == IncrementDecrementMatchResult::NotKnownSafe) {
+    if (!Result->KnownSafe) {
       DEBUG(llvm::dbgs() << "    NOT KNOWN SAFE!\n");
       KnownSafeBU = false;
     }
+    if (Result->Partial) {
+      DEBUG(llvm::dbgs() << "    IS PARTIAL!\n");
+      Partial = true;
+    }
     NewDecrements.clear();
 
     // If we do not have any increment to attempt to match up with again, bail.
@@ -287,13 +306,21 @@ bool ARCMatchingSetBuilder::matchUpIncDecSetsForPtr() {
     MatchSet.DecrementInsertPts.clear();
   } else {
     DEBUG(llvm::dbgs() << "NOT UNCONDITIONALLY SAFE!\n");
+
   }
 
+  // If we have insertion points and partial merges, return false to avoid
+  // control dependency issues.
+  if ((!MatchSet.IncrementInsertPts.empty() ||
+       !MatchSet.DecrementInsertPts.empty()) &&
+      Partial)
+    return false;
+
   if (MatchSet.IncrementInsertPts.empty() && !MatchSet.Increments.empty())
     MatchedPair = true;
 
-  DEBUG(llvm::dbgs() << "SUCCESS! We can move, remove things.\n");
   // Success!
+  DEBUG(llvm::dbgs() << "SUCCESS! We can move, remove things.\n");
   return true;
 }
 

lib/SILAnalysis/GlobalARCSequenceDataflow.cpp

@@ -219,10 +219,9 @@ bool TopDownRefCountState::merge(const TopDownRefCountState &Other) {
   // conservatively drop the sequence, to avoid doing partial RR
   // elimination. If the branch predicates for the two merge differ, mixing
   // them is unsafe since they are not control dependent.
-  if (LatState == TopDownRefCountState::LatticeState::None || Partial ||
+  if (LatState == TopDownRefCountState::LatticeState::None) {
-      Other.Partial) {
     RefCountState<TopDownRefCountState>::clear();
-    DEBUG(llvm::dbgs() << "            Found LatticeState::None or Partial. "
+    DEBUG(llvm::dbgs() << "            Found LatticeState::None. "
                           "Clearing State!\n");
     return false;
   }
@@ -238,16 +237,10 @@ bool TopDownRefCountState::merge(const TopDownRefCountState &Other) {
 
   Increments.insert(Other.Increments.begin(), Other.Increments.end());
 
-  Partial = InsertPts.size() != Other.InsertPts.size();
+  Partial |= InsertPts.size() != Other.InsertPts.size();
   for (auto *SI : Other.InsertPts)
     Partial |= InsertPts.insert(SI);
 
-  if (Partial) {
-    DEBUG(llvm::dbgs() << "            Found partial, clearing state!\n");
-    RefCountState<TopDownRefCountState>::clear();
-    return false;
-  }
-
   return true;
 }
 
@@ -264,9 +257,8 @@ bool BottomUpRefCountState::merge(const BottomUpRefCountState &Other) {
   // conservatively drop the sequence, to avoid doing partial RR
   // elimination. If the branch predicates for the two merge differ, mixing
   // them is unsafe since they are not control dependent.
-  if (LatState == BottomUpRefCountState::LatticeState::None || Partial ||
+  if (LatState == BottomUpRefCountState::LatticeState::None) {
-      Other.Partial) {
+    DEBUG(llvm::dbgs() << "            Found LatticeState::None. "
-    DEBUG(llvm::dbgs() << "            Found LatticeState::None or Partial. "
                           "Clearing State!\n");
     RefCountState<BottomUpRefCountState>::clear();
     return false;
@@ -274,16 +266,10 @@ bool BottomUpRefCountState::merge(const BottomUpRefCountState &Other) {
 
   Decrements.insert(Other.Decrements.begin(), Other.Decrements.end());
 
-  Partial = InsertPts.size() != Other.InsertPts.size();
+  Partial |= InsertPts.size() != Other.InsertPts.size();
   for (auto *SI : Other.InsertPts)
     Partial |= InsertPts.insert(SI);
 
-  if (Partial) {
-    DEBUG(llvm::dbgs() << "            Found partial, clearing state!\n");
-    RefCountState<BottomUpRefCountState>::clear();
-    return false;
-  }
-
   return true;
 }
 

lib/SILAnalysis/ReferenceCountState.h

@@ -178,6 +178,11 @@ struct RefCountState {
   /// increments.
   bool isKnownSafe() const { return KnownSafe; }
 
+  /// This reference count state is partial if we found a partial merge of
+  /// insertion points. This stymies our ability to move instructions due to
+  /// potential control dependency issues.
+  bool isPartial() const { return Partial; }
+
   /// Check if PotentialDecrement can decrement the reference count associated
   /// with the value we are tracking. If so advance the state's sequence
   /// appropriately and return true. Otherwise return false.

test/SILPasses/globalarcopts.sil

@@ -1038,3 +1038,49 @@ bb2:
   %3 = apply %2() : $@thin @noreturn @callee_owned () -> ()
   unreachable
 }
+
+// CHECK: sil @dont_move_values_in_face_of_partial_merges : $@thin (Builtin.NativeObject) -> () {
+// CHECK: strong_retain
+// CHECK: strong_release
+sil @dont_move_values_in_face_of_partial_merges : $@thin (Builtin.NativeObject) -> () {
+bb0(%0 : $Builtin.NativeObject):
+  %1 = function_ref @user : $@thin (Builtin.NativeObject) -> ()
+  strong_retain %0 : $Builtin.NativeObject
+  cond_br undef, bb1, bb2
+
+bb1:
+  apply %1(%0) : $@thin (Builtin.NativeObject) -> ()
+  br bb3
+
+bb2:
+  br bb3
+
+bb3:
+  strong_release %0 : $Builtin.NativeObject
+  %2 = tuple()
+  return %2 : $()
+}
+
+// CHECK-LABEL: sil @do_remove_values_in_face_of_partial_merges : $@thin (Builtin.NativeObject) -> () {
+// CHECK: strong_retain
+// CHECK-NOT: strong_release
+// CHECK-NOT: strong_release
+sil @do_remove_values_in_face_of_partial_merges : $@thin (Builtin.NativeObject) -> () {
+bb0(%0 : $Builtin.NativeObject):
+  %1 = function_ref @user : $@thin (Builtin.NativeObject) -> ()
+  strong_retain %0 : $Builtin.NativeObject
+  strong_retain %0 : $Builtin.NativeObject
+  cond_br undef, bb1, bb2
+
+bb1:
+  apply %1(%0) : $@thin (Builtin.NativeObject) -> ()
+  br bb3
+
+bb2:
+  br bb3
+
+bb3:
+  strong_release %0 : $Builtin.NativeObject
+  %2 = tuple()
+  return %2 : $()
+}