[simplify-cfg] Add a visitedBlocks set to hasSameUltimateSuccessor to prevent infinite loops.

Previously, we were not handling properly blocks that we could visit multiple
times. In this commit, I added a SmallPtrSet to ensure that we handle all of the
same cases that we handled previously.

The key reason that we want to follow this approach rather than something else
is that the previous algorithm on purpose allowed for side-entrances from other
checks since often times when we have multiple checks, all of the .none branches
funnel together into a single ultimate block.

This can be seen by the need of this code to support the test two_chained_calls
in simplify_switch_enum_objc.sil.

rdar://55861081

Author: gottesmm

lib/SILOptimizer/Transforms/SimplifyCFG.cpp

@@ -1964,43 +1964,88 @@ static bool onlyForwardsNone(SILBasicBlock *noneBB, SILBasicBlock *someBB,
   return true;
 }
 
-/// Check whether \p noneBB has the same ultimate successor as the successor to someBB.
-///  someBB       noneBB
-///   \             |
-///    \            ... (more bbs?)
+/// Check whether the \p someBB has only one single successor and that successor
+/// post-dominates \p noneBB.
+///
+///                          (maybe otherNoneBB)
+///  someBB       noneBB     /
+///   \             |       v
+///    \            ... more bbs? (A)
 ///     \           /
 ///       ulimateBB
+///
+/// This routine does not support diverging control flow in (A). This means that
+/// there must not be any loops or diamonds beginning in that region. We do
+/// support side-entrances from blocks not reachable from noneBB in order to
+/// ensure that we properly handle other failure cases where the failure case
+/// merges into .noneBB before ultimate BB.
+///
+/// DISCUSSION: We allow this side-entrance pattern to handle iterative
+/// conditional checks which all feed the failing case through the .none
+/// path. This is a common pattern in swift code. As an example consider a
+/// switch statement with multiple pattern binding matching that use the same
+/// cleanup code upon failure.
 static bool hasSameUltimateSuccessor(SILBasicBlock *noneBB, SILBasicBlock *someBB) {
   // Make sure that both our some, none blocks both have single successors that
   // are not themselves (which can happen due to single block loops).
   auto *someSuccessorBB = someBB->getSingleSuccessorBlock();
   if (!someSuccessorBB || someSuccessorBB == someBB)
     return false;
+
   auto *noneSuccessorBB = noneBB->getSingleSuccessorBlock();
   if (!noneSuccessorBB || noneSuccessorBB == noneBB)
     return false;
 
-  // If we immediately find a diamond, return true. We are done.
+  // If we immediately find a simple diamond, return true. We are done.
   if (noneSuccessorBB == someSuccessorBB)
     return true;
 
-  // Otherwise, lets keep looking down the none case.
-  auto *next = noneSuccessorBB;
-  while (next != someSuccessorBB) {
-    noneSuccessorBB = next;
-    next = noneSuccessorBB->getSingleSuccessorBlock();
+  // Otherwise, lets begin a traversal along the successors of noneSuccessorBB,
+  // searching for someSuccessorBB, being careful to only allow for blocks to be
+  // visited once. This enables us to guarantee that there are not any loops or
+  // any sub-diamonds in the part of the CFG we are traversing. This /does/
+  // allow for side-entrances to the region from blocks not reachable from
+  // noneSuccessorBB. See function level comment above.
+  SILBasicBlock *iter = noneSuccessorBB;
+  SmallPtrSet<SILBasicBlock *, 8> visitedBlocks;
+  visitedBlocks.insert(iter);
 
-    // If we find another single successor and it is not our own block (due to a
-    // self-loop), continue.
-    if (next && next != noneSuccessorBB)
-      continue;
+  do {
+    // First try to grab our single successor if we have only one. If we have no
+    // successor or more than one successor, bail and do not optimize.
+    //
+    // DISCUSSION: Trivially, if we do not have a successor, then we have
+    // reached either a return/unreachable and this path will never merge with
+    // the ultimate block. If we have more than one successor, then for our
+    // condition to pass, we must have that both successors eventually join into
+    // someSuccessorBB. But this would imply that either someSuccessorBB has
+    // more than two predecessors and or that we merge the two paths before we
+    // visit someSuccessorBB.
+    auto *succBlock = iter->getSingleSuccessorBlock();
+    if (!succBlock)
+      return false;
 
-    // Otherwise, we either have multiple successors or a self-loop. We do not
-    // support this, return false.
-    return false;
-  }
+    // Then check if our single successor block has been visited already. If so,
+    // we have some sort of loop or have some sort of merge point that is not
+    // the final merge point.
+    //
+    // NOTE: We do not need to worry about someSuccessorBB being in
+    // visitedBlocks since before we begin the loop, we check that
+    // someSuccessorBB != iter and also check that in the do-while condition. So
+    // we can never have visited someSuccessorBB on any previous iteration
+    // meaning that the only time we can have succBlock equal to someSuccessorBB
+    // is on the last iteration before we exit the loop.
+    if (!visitedBlocks.insert(succBlock).second)
+      return false;
+
+    // Otherwise, set iter to succBlock.
+    iter = succBlock;
+
+    // And then check if this new successor block is someSuccessorBB. If so, we
+    // break and then return true since we have found our target. Otherwise, we
+    // need to visit further successors, so go back around the loop.
+  } while (iter != someSuccessorBB);
 
-  // At this point, we know that next must be someSuccessorBB.
   return true;
 }
 

test/SILOptimizer/simplify_switch_enum_objc.sil

@@ -1,5 +1,9 @@
 // RUN: %target-sil-opt -enable-objc-interop -enforce-exclusivity=none -enable-sil-verify-all %s -simplify-cfg | %FileCheck %s
 
+// Just make sure that we do not infinite loop when compiling without block merging.
+//
+// RUN: %target-sil-opt -enable-objc-interop -enforce-exclusivity=none -enable-sil-verify-all %s -simplify-cfg -simplify-cfg-simplify-unconditional-branches=0
+
 // REQUIRES: objc_interop
 
 import Swift
@@ -421,3 +425,33 @@ bb5:
   release_value %16 : $Optional<NSObject>
   switch_enum %16 : $Optional<NSObject>, case #Optional.none!enumelt: bb4, default bb3
 }
+
+// Just make sure that we do not infinite loop here.
+//
+// CHECK-LABEL: sil @infinite_loop_2 : $@convention(thin) () -> @owned Optional<NSObject> {
+// CHECK: } // end sil function 'infinite_loop_2'
+sil @infinite_loop_2 : $@convention(thin) () -> @owned Optional<NSObject> {
+bb0:
+  %0 = function_ref @infinite_loop_get_optional_nsobject : $@convention(thin) () -> @autoreleased Optional<NSObject>
+  %1 = apply %0() : $@convention(thin) () -> @autoreleased Optional<NSObject>
+  br bb1(%1 : $Optional<NSObject>)
+
+bb1(%3 : $Optional<NSObject>):
+  switch_enum %3 : $Optional<NSObject>, case #Optional.some!enumelt.1: bb3, case #Optional.none!enumelt: bb2
+
+bb2:
+  br bb5(%3 : $Optional<NSObject>)
+
+bb3(%7 : $NSObject):
+  br bb4(%3 : $Optional<NSObject>)
+
+bb4(%9 : $Optional<NSObject>):
+  %11 = enum $Optional<NSObject>, #Optional.none!enumelt
+  return %11 : $Optional<NSObject>
+
+bb5(%14 : $Optional<NSObject>):
+  br bb6
+
+bb6:
+  br bb5(%14 : $Optional<NSObject>)
+}