Add SIL SinkAddressProjections utility to avoid address phis.

Enable this utility during jump-threading in SimplifyCFG.

Ultimately, the SIL verifier should prevent all address-phis and we'll
need to use this utility in a few more places.

Fixes <rdar://problem/55320867> SIL verification failed: Unknown
formal access pattern: storage

Author: atrick

include/swift/SILOptimizer/Utils/Local.h

@@ -372,6 +372,46 @@ class BasicBlockCloner : public SILCloner<BasicBlockCloner> {
   }
 };
 
+/// Sink address projections to their out-of-block uses. This is
+/// required after cloning a block and before calling
+/// updateSSAAfterCloning to avoid address-type phis.
+///
+/// This clones address projections at their use points, but does not
+/// mutate the block containing the projections.
+class SinkAddressProjections {
+  // Projections ordered from last to first in the chain.
+  SmallVector<SingleValueInstruction *, 4> projections;
+  SmallSetVector<SILValue, 4> inBlockDefs;
+
+public:
+  /// Check for an address projection chain ending at \p inst. Return true if
+  /// the given instruction is successfully analyzed.
+  ///
+  /// If \p inst does not produce an address, then return
+  /// true. getInBlockDefs() will contain \p inst if any of its
+  /// (non-address) values are used outside its block.
+  ///
+  /// If \p inst does produce an address, return true only of the
+  /// chain of address projections within this block is clonable at
+  /// their use sites. getInBlockDefs will return all non-address
+  /// operands in the chain that are also defined in this block. These
+  /// may require phis after cloning the projections.
+  bool analyzeAddressProjections(SILInstruction *inst);
+
+  /// After analyzing projections, returns the list of (non-address) values
+  /// defined in the same block as the projections which will have uses outside
+  /// the block after cloning.
+  ArrayRef<SILValue> getInBlockDefs() const {
+    return inBlockDefs.getArrayRef();
+  }
+  /// Clone the chain of projections at their use sites.
+  ///
+  /// Return true if anything was done.
+  ///
+  /// getInBlockProjectionOperandValues() can be called before or after cloning.
+  bool cloneProjections();
+};
+
 /// Helper function to perform SSA updates in case of jump threading.
 void updateSSAAfterCloning(BasicBlockCloner &Cloner, SILBasicBlock *SrcBB,
                            SILBasicBlock *DestBB);

lib/SILOptimizer/Transforms/SimplifyCFG.cpp

@@ -209,6 +209,79 @@ static bool isUsedOutsideOfBlock(SILValue V, SILBasicBlock *BB) {
   return false;
 }
 
+// Populate 'projections' with the chain of address projections leading
+// to and including 'inst'.
+//
+// Populate 'inBlockDefs' with all the non-address value definitions in
+// the block that will be used outside this block after projection sinking.
+//
+// Return true on success, even if projections is empty.
+bool SinkAddressProjections::analyzeAddressProjections(SILInstruction *inst) {
+  projections.clear();
+  inBlockDefs.clear();
+
+  SILBasicBlock *bb = inst->getParent();
+  auto pushOperandVal = [&](SILValue def) {
+    if (def->getParentBlock() != bb)
+      return true;
+
+    if (!def->getType().isAddress()) {
+      inBlockDefs.insert(def);
+      return true;
+    }
+    if (auto *addressProj = dyn_cast<SingleValueInstruction>(def)) {
+      if (addressProj->isTriviallyDuplicatable()) {
+        projections.push_back(addressProj);
+        return true;
+      }
+    }
+    // Can't handle a multi-value or unclonable address producer.
+    return false;
+  };
+  // Check the given instruction for any address-type results.
+  for (auto result : inst->getResults()) {
+    if (!isUsedOutsideOfBlock(result, bb))
+      continue;
+    if (!pushOperandVal(result))
+      return false;
+  }
+  // Recurse upward through address projections.
+  for (unsigned idx = 0; idx < projections.size(); ++idx) {
+    // Only one address result/operand can be handled per instruction.
+    if (projections.size() != idx + 1)
+      return false;
+
+    for (SILValue operandVal : projections[idx]->getOperandValues())
+      pushOperandVal(operandVal);
+  }
+  return true;
+}
+
+// Clone the projections gathered by 'analyzeAddressProjections' at
+// their use site outside this block.
+bool SinkAddressProjections::cloneProjections() {
+  if (projections.empty())
+    return false;
+
+  SILBasicBlock *bb = projections.front()->getParent();
+  SmallVector<Operand *, 4> usesToReplace;
+  // Clone projections in last-to-first order.
+  for (unsigned idx = 0; idx < projections.size(); ++idx) {
+    auto *oldProj = projections[idx];
+    assert(oldProj->getParent() == bb);
+    usesToReplace.clear();
+    for (Operand *use : oldProj->getUses()) {
+      if (use->getUser()->getParent() != bb)
+        usesToReplace.push_back(use);
+    }
+    for (Operand *use : usesToReplace) {
+      auto *newProj = oldProj->clone(use->getUser());
+      use->set(cast<SingleValueInstruction>(newProj));
+    }
+  }
+  return true;
+}
+
 /// Helper function to perform SSA updates in case of jump threading.
 void swift::updateSSAAfterCloning(BasicBlockCloner &Cloner,
                                   SILBasicBlock *SrcBB, SILBasicBlock *DestBB) {
@@ -1024,20 +1097,31 @@ bool SimplifyCFG::tryJumpThreading(BranchInst *BI) {
   // If it looks potentially interesting, decide whether we *can* do the
   // operation and whether the block is small enough to be worth duplicating.
   int copyCosts = 0;
-  for (auto &Inst : *DestBB) {
-    copyCosts += getThreadingCost(&Inst);
+  SinkAddressProjections sinkProj;
+  for (auto ii = DestBB->begin(), ie = DestBB->end(); ii != ie;) {
+    copyCosts += getThreadingCost(&*ii);
     if (ThreadingBudget <= copyCosts)
       return false;
 
-    // We need to update ssa if a value is used outside the duplicated block.
-    if (!NeedToUpdateSSA) {
-      for (auto result : Inst.getResults()) {
-        if (isUsedOutsideOfBlock(result, DestBB)) {
-          NeedToUpdateSSA = true;
-          break;
-        }
-      }
-    }
+    // If this is an address projection with outside uses, sink it before
+    // checking for SSA update.
+    if (!sinkProj.analyzeAddressProjections(&*ii))
+      return false;
+
+    sinkProj.cloneProjections();
+    // After cloning check if any of the non-address defs in the cloned block
+    // (including the current instruction) now have uses outside the
+    // block. Do this even if nothing was cloned.
+    if (!sinkProj.getInBlockDefs().empty())
+      NeedToUpdateSSA = true;
+
+    auto nextII = std::next(ii);
+    recursivelyDeleteTriviallyDeadInstructions(
+        &*ii, false, [&nextII](SILInstruction *deadInst) {
+          if (deadInst->getIterator() == nextII)
+            ++nextII;
+        });
+    ii = nextII;
   }
 
   // Don't jump thread through a potential header - this can produce irreducible

test/SILOptimizer/simplify_cfg.sil

@@ -1425,24 +1425,24 @@ bb6(%r : $Int32):
 // CHECK:      bb0:
 // CHECK-NEXT:   cond_br undef, bb1, bb2
 // CHECK:      bb1:
-// CHECK-NEXT:   cond_br undef, bb3, bb4
+// CHECK:        enum $Optional<Int32>, #Optional.none!enumelt
+// CHECK:        br bb3
 // CHECK:      bb2:
+// CHECK:        integer_literal $Builtin.Int32, 0
+// CHECK:        enum $Optional<Int32>, #Optional.some!enumelt.1
+// CHECK:        br bb3
+// CHECK:      bb3
 // CHECK:        integer_literal {{.*}}, 27
-// CHECK:        br bb5
-// CHECK:      bb3:
-// CHECK:        br bb4
-// CHECK:      bb4:
 // CHECK:        integer_literal {{.*}}, 28
-// CHECK:        br bb5
-// CHECK:      bb5({{.*}}):
-// CHECK-NEXT:   return
+// CHECK:        return
 sil @jumpthread_switch_enum4 : $@convention(thin) () -> Builtin.Int32 {
 bb0:
+  %c0 = builtin "assert_configuration"() : $Builtin.Int32
   cond_br undef, bb1, bb2
 
 bb1:
   %4 = enum $Optional<Int32>, #Optional.none!enumelt
-  cond_br undef, bb3(%4 : $Optional<Int32>), bb4(%4 : $Optional<Int32>)
+  cond_br undef, bb3(%4 : $Optional<Int32>), bb4(%4 : $Optional<Int32>, %c0 : $Builtin.Int32)
 
 bb2:
   %6 = integer_literal $Builtin.Int32, 0
@@ -1454,23 +1454,22 @@ bb2:
 bb3(%10 : $Optional<Int32>):
   // Some instruction which is not "trivial"
   %c1 = builtin "assert_configuration"() : $Builtin.Int32
-  br bb4(%10 : $Optional<Int32>)
-
+  br bb4(%10 : $Optional<Int32>, %c1 : $Builtin.Int32)
 
-bb4(%13 : $Optional<Int32>):
+bb4(%13 : $Optional<Int32>, %carg1 : $Builtin.Int32):
   switch_enum %13 : $Optional<Int32>, case #Optional.some!enumelt.1: bb5, case #Optional.none!enumelt: bb6
 
 bb5:
   %r1 = integer_literal $Builtin.Int32, 27
   %c2 = builtin "assert_configuration"() : $Builtin.Int32
-  br bb7(%r1 : $Builtin.Int32)
+  br bb7(%r1 : $Builtin.Int32, %c2 : $Builtin.Int32)
 
 bb6:
   %r2 = integer_literal $Builtin.Int32, 28
   %c3 = builtin "assert_configuration"() : $Builtin.Int32
-  br bb7(%r2 : $Builtin.Int32)
+  br bb7(%r2 : $Builtin.Int32, %c3 : $Builtin.Int32)
 
-bb7(%r : $Builtin.Int32):
+bb7(%r : $Builtin.Int32, %carg2 : $Builtin.Int32):
   return %r : $Builtin.Int32
 }
 

test/SILOptimizer/simplify_cfg_address_phi.sil

@@ -0,0 +1,136 @@
+// RUN: %target-sil-opt -enable-objc-interop -enable-sil-verify-all %s -jumpthread-simplify-cfg | %FileCheck %s
+//
+// Test SimplifyCFG's handling of address-type phis. In other words, makes sure it doesn't create them at all!
+
+sil_stage canonical
+
+import Builtin
+import Swift
+import SwiftShims
+
+class C {
+  @_hasStorage @_hasInitialValue var field: Int { get set }
+  @objc deinit
+    init()
+}
+
+sil @getC : $@convention(thin) () -> C
+
+// Test that jump threading sinks a ref_element_addr, generating a
+// non-address phi for its operand.
+//
+// The retain on separate paths followed by a merged release, and
+// target block with a conditional branch are necessary just to get
+// jump threading to kick in.
+//
+// CHECK-LABEL: sil @testJumpThreadRefEltLoop : $@convention(thin) () -> () {
+// CHECK: bb0
+// CHECK:   function_ref @getC : $@convention(thin) () -> C
+// CHECK:   cond_br undef, bb1, bb2
+// CHECK: bb1:
+// CHECK:   [[C1:%.*]] = apply %0() : $@convention(thin) () -> C
+// CHECK:   strong_retain [[C1]] : $C
+// CHECK:   strong_release [[C1]] : $C
+// CHECK:   br bb3([[C1]] : $C)
+// CHECK: bb2:
+// CHECK:   [[C2:%.*]] = apply %0() : $@convention(thin) () -> C
+// CHECK:   strong_retain [[C2]] : $C
+// CHECK:   strong_release [[C2]] : $C
+// CHECK:   br bb3([[C2]] : $C)
+// CHECK: bb3([[PHI:%.*]] : $C):
+// CHECK:   [[ADR:%.*]] = ref_element_addr [[PHI]] : $C, #C.field
+// CHECK:   begin_access [read] [dynamic] [[ADR]] : $*Int
+// CHECK:   load
+// CHECK:   end_access
+// CHECK-LABEL: } // end sil function 'testJumpThreadRefEltLoop'
+sil @testJumpThreadRefEltLoop : $@convention(thin) () -> () {
+bb0:
+  %f = function_ref @getC : $@convention(thin) () -> C
+  cond_br undef, bb1, bb2
+
+bb1:
+  %c1 = apply %f() : $@convention(thin) ()->C
+  strong_retain %c1 : $C
+  br bb3(%c1 : $C)
+
+bb2:
+  %c2 = apply %f() : $@convention(thin) ()->C
+  strong_retain %c2 : $C
+  br bb3(%c2 : $C)
+
+bb3(%arg : $C):
+  strong_release %arg : $C
+  %18 = ref_element_addr %arg : $C, #C.field
+  br bb4
+
+bb4:
+  %19 = begin_access [read] [dynamic] %18 : $*Int
+  %20 = load %19 : $*Int
+  end_access %19 : $*Int
+  cond_br undef, bb4, bb5
+
+bb5:
+  %z = tuple ()
+  return %z : $()
+}
+
+// Test that jump threading sinks a
+// ref_tail_addr->index_addr->struct_element_addr chain and generates
+// a phi for the index_addr's index operand.
+//
+// The retain on separate paths followed by a merged release, and
+// target block with a conditional branch are necessary just to get
+// jump threading to kick in.
+//
+// CHECK-LABEL: sil @testJumpThreadIndex : $@convention(thin) (__ContiguousArrayStorageBase, Builtin.Int64) -> Builtin.Int32 {
+// CHECK: bb0(%0 : $__ContiguousArrayStorageBase, %1 : $Builtin.Int64):
+// CHECK:   cond_br undef, bb1, bb2
+// CHECK: bb1:
+// CHECK:   apply
+// CHECK:   strong_retain
+// CHECK:   strong_release
+// CHECK:   [[IDX1:%.*]] = builtin "truncOrBitCast_Int64_Word"(%1 : $Builtin.Int64) : $Builtin.Word
+// CHECK:   br bb3([[IDX1]] : $Builtin.Word)
+// CHECK: bb2:
+// CHECK:   apply
+// CHECK:   strong_retain
+// CHECK:   strong_release
+// CHECK:   [[IDX2:%.*]] = builtin "truncOrBitCast_Int64_Word"(%1 : $Builtin.Int64) : $Builtin.Word
+// CHECK:   br bb3([[IDX2]] : $Builtin.Word)
+// CHECK: bb3([[PHI:%.*]] : $Builtin.Word):
+// CHECK:   [[TAIL:%.*]] = ref_tail_addr %0 : $__ContiguousArrayStorageBase, $Int32
+// CHECK:   [[ELT:%.*]] = index_addr [[TAIL]] : $*Int32, %14 : $Builtin.Word
+// CHECK:   [[ADR:%.*]] = struct_element_addr [[ELT]] : $*Int32, #Int32._value
+// CHECK:   load [[ADR]] : $*Builtin.Int32
+// CHECK:   cond_br undef, bb4, bb5
+// CHECK-LABEL: } // end sil function 'testJumpThreadIndex'
+sil @testJumpThreadIndex : $@convention(thin) (__ContiguousArrayStorageBase, Builtin.Int64) -> Builtin.Int32 {
+bb0(%0 : $__ContiguousArrayStorageBase, %1 : $Builtin.Int64):
+  %f = function_ref @getC : $@convention(thin) () -> C
+  cond_br undef, bb1, bb2
+
+bb1:
+  %c1 = apply %f() : $@convention(thin) ()->C
+  strong_retain %c1 : $C
+  br bb3(%c1 : $C)
+
+bb2:
+  %c2 = apply %f() : $@convention(thin) ()->C
+  strong_retain %c2 : $C
+  br bb3(%c2 : $C)
+
+bb3(%arg : $C):
+  strong_release %arg : $C
+  %tail = ref_tail_addr %0 : $__ContiguousArrayStorageBase, $Int32
+  %idx = builtin "truncOrBitCast_Int64_Word"(%1 : $Builtin.Int64) : $Builtin.Word
+  %elt = index_addr %tail : $*Int32, %idx : $Builtin.Word
+  %adr = struct_element_addr %elt : $*Int32, #Int32._value
+  br bb4
+
+bb4:
+  %val = load %adr : $*Builtin.Int32
+  cond_br undef, bb4, bb5
+
+bb5:
+  return %val : $Builtin.Int32
+}