Generalize and fix SinkAddressProjections.

Fixes a potential real bug in the case that SinkAddressProjections moves
projections without notifying SimplifyCFG of the change. This could
fail to update Analyses (probably won't break anything in practice).

Introduce SILInstruction::isPure. Among other things, this can tell
you if it's safe to duplicate instructions at their
uses. SinkAddressProjections should check this before sinking uses. I
couldn't find a way to expose this as a real bug, but it is a
theoretical bug.

Add the SinkAddressProjections functionality to the BasicBlockCloner
utility. Enable address projection sinking for all BasicBlockCloner
clients (the four different kinds of jump-threading that use it). This
brings the compiler much closer to banning all address phis.

The "bugs" were originally introduced a week ago here:

commit f22371bf0b (fork/fix-address-phi, fix-address-phi)
Author: Andrew Trick <atrick@apple.com>
Date:   Tue Sep 17 16:45:51 2019

    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

include/swift/SIL/SILInstruction.h

@@ -45,6 +45,7 @@
 
 namespace swift {
 
+class AllocationInst;
 class DeclRefExpr;
 class FloatLiteralExpr;
 class FuncDecl;
@@ -618,6 +619,15 @@ public:
     return getMemoryBehavior() != MemoryBehavior::None;
   }
 
+  /// Return true if the instruction is "pure" in the sense that it may execute
+  /// multiple times without affecting behavior. This implies that it can be
+  /// trivially cloned at multiple use sites without preserving path
+  /// equivalence.
+  bool isPure() const {
+    return !mayReadOrWriteMemory() && !mayTrap() && !isa<AllocationInst>(this)
+           && !isa<TermInst>(this);
+  }
+
   /// Returns true if the result of this instruction is a pointer to stack
   /// allocated memory. In this case there must be an adjacent deallocating
   /// instruction.

include/swift/SILOptimizer/Utils/BasicBlockOptUtils.h

@@ -22,9 +22,10 @@
 #ifndef SWIFT_SILOPTIMIZER_UTILS_BASICBLOCKOPTUTILS_H
 #define SWIFT_SILOPTIMIZER_UTILS_BASICBLOCKOPTUTILS_H
 
-#include "swift/SIL/SILInstruction.h"
 #include "swift/SIL/SILBasicBlock.h"
 #include "swift/SIL/SILCloner.h"
+#include "swift/SIL/SILInstruction.h"
+#include "swift/SILOptimizer/Utils/InstOptUtils.h"
 
 namespace swift {
 
@@ -61,12 +62,65 @@ bool rotateLoop(SILLoop *loop, DominanceInfo *domInfo, SILLoopInfo *loopInfo,
                 bool rotateSingleBlockLoops, SILBasicBlock *upToBB,
                 bool shouldVerify);
 
-/// Helper function to perform SSA updates in case of jump threading.
-void updateSSAAfterCloning(BasicBlockCloner &cloner, SILBasicBlock *srcBB,
-                           SILBasicBlock *destBB);
+/// 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.
+///
+/// BasicBlockCloner handles this internally.
+class SinkAddressProjections {
+  // Projections ordered from last to first in the chain.
+  SmallVector<SingleValueInstruction *, 4> projections;
+  SmallSetVector<SILValue, 4> inBlockDefs;
+
+  // Transient per-projection data for use during cloning.
+  SmallVector<Operand *, 4> usesToReplace;
+  llvm::SmallDenseMap<SILBasicBlock *, Operand *, 4> firstBlockUse;
+
+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();
+};
 
 /// Clone a single basic block and any required successor edges within the same
 /// function.
+///
+/// Before cloning, call either canCloneBlock or call canCloneInstruction for
+/// every instruction in the original block.
+///
+/// To clone just the block, call cloneBlock. To also update the original
+/// block's branch to jump to the newly cloned block, call cloneBranchTarget
+/// instead.
+///
+/// After cloning, call splitCriticalEdges, then updateSSAAfterCloning. This is
+/// decoupled from cloning becaused some clients perform CFG edges updates after
+/// cloning but before splitting CFG edges.
 class BasicBlockCloner : public SILCloner<BasicBlockCloner> {
   using SuperTy = SILCloner<BasicBlockCloner>;
   friend class SILCloner<BasicBlockCloner>;
@@ -75,18 +129,56 @@ protected:
   /// The original block to be cloned.
   SILBasicBlock *origBB;
 
+  /// Will cloning require an SSA update?
+  bool needsSSAUpdate = false;
+
+  /// Transient object for analyzing a single address projction chain. It's
+  /// state is reset each time analyzeAddressProjections is called.
+  SinkAddressProjections sinkProj;
+
 public:
   /// An ordered list of old to new available value pairs.
   ///
   /// updateSSAAfterCloning() expects this public field to hold values that may
   /// be remapped in the cloned block and live out.
-  SmallVector<std::pair<SILValue, SILValue>, 16> AvailVals;
+  SmallVector<std::pair<SILValue, SILValue>, 16> availVals;
 
   // Clone blocks starting at `origBB`, within the same function.
   BasicBlockCloner(SILBasicBlock *origBB)
       : SILCloner(*origBB->getParent()), origBB(origBB) {}
 
+  bool canCloneBlock() {
+    for (auto &inst : *origBB) {
+      if (!canCloneInstruction(&inst))
+        return false;
+    }
+    return true;
+  }
+
+  /// Returns true if \p inst can be cloned.
+  ///
+  /// If canCloneBlock is not called, then this must be called for every
+  /// instruction in origBB, both to ensure clonability and to handle internal
+  /// book-keeping (needsSSAUpdate).
+  bool canCloneInstruction(SILInstruction *inst) {
+    assert(inst->getParent() == origBB);
+
+    if (!inst->isTriviallyDuplicatable())
+      return false;
+
+    if (!sinkProj.analyzeAddressProjections(inst))
+      return false;
+
+    // Check if any of the non-address defs in the cloned block (including the
+    // current instruction) will still have uses outside the block after sinking
+    // address projections.
+    needsSSAUpdate |= !sinkProj.getInBlockDefs().empty();
+    return true;
+  }
+
   void cloneBlock(SILBasicBlock *insertAfterBB = nullptr) {
+    sinkAddressProjections();
+
     SmallVector<SILBasicBlock *, 4> successorBBs;
     successorBBs.reserve(origBB->getSuccessors().size());
     llvm::copy(origBB->getSuccessors(), std::back_inserter(successorBBs));
@@ -95,6 +187,9 @@ public:
 
   /// Clone the given branch instruction's destination block, splitting
   /// its successors, and rewrite the branch instruction.
+  ///
+  /// Return false if the branch's destination block cannot be cloned. When
+  /// false is returned, no changes have been made.
   void cloneBranchTarget(BranchInst *bi) {
     assert(origBB == bi->getDestBB());
 
@@ -110,10 +205,16 @@ public:
     return remapBasicBlock(origBB);
   }
 
+  bool wasCloned() { return isBlockCloned(origBB); }
+
   /// Call this after processing all instructions to fix the control flow
   /// graph. The branch cloner may have left critical edges.
   bool splitCriticalEdges(DominanceInfo *domInfo, SILLoopInfo *loopInfo);
 
+  /// Helper function to perform SSA updates after calling both
+  /// cloneBranchTarget and splitCriticalEdges.
+  void updateSSAAfterCloning();
+
 protected:
   // MARK: CRTP overrides.
 
@@ -137,8 +238,10 @@ protected:
 
   void mapValue(SILValue origValue, SILValue mappedValue) {
     SuperTy::mapValue(origValue, mappedValue);
-    AvailVals.emplace_back(origValue, mappedValue);
+    availVals.emplace_back(origValue, mappedValue);
   }
+
+  void sinkAddressProjections();
 };
 
 // Helper class that provides a callback that can be used in
@@ -173,46 +276,6 @@ public:
   }
 };
 
-/// 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();
-};
-
 /// Utility class for cloning init values into the static initializer of a
 /// SILGlobalVariable.
 class StaticInitCloner : public SILCloner<StaticInitCloner> {

include/swift/SILOptimizer/Utils/SILSSAUpdater.h

@@ -133,8 +133,10 @@ public:
   /// reconstruct the use.
   UseWrapper(Operand *Use);
 
+  Operand *getOperand();
+
   /// Return the operand we wrap. Reconstructing branch operands.
-  operator Operand*();
+  operator Operand*() { return getOperand(); }
 };
 
 } // end namespace swift

lib/SILOptimizer/Transforms/SimplifyCFG.cpp

@@ -274,12 +274,15 @@ public:
 
   ThreadInfo() = default;
 
-  void threadEdge() {
+  bool threadEdge() {
     LLVM_DEBUG(llvm::dbgs() << "thread edge from bb" << Src->getDebugID()
                             << " to bb" << Dest->getDebugID() << '\n');
     auto *SrcTerm = cast<BranchInst>(Src->getTerminator());
 
     BasicBlockCloner Cloner(SrcTerm->getDestBB());
+    if (!Cloner.canCloneBlock())
+      return false;
+
     Cloner.cloneBranchTarget(SrcTerm);
 
     // We have copied the threaded block into the edge.
@@ -329,7 +332,8 @@ public:
     // After rewriting the cloned branch, split the critical edge.
     // This does not currently update DominanceInfo.
     Cloner.splitCriticalEdges(nullptr, nullptr);
-    updateSSAAfterCloning(Cloner, Src, Dest);
+    Cloner.updateSSAAfterCloning();
+    return true;
   }
 };
 
@@ -551,7 +555,7 @@ bool SimplifyCFG::dominatorBasedSimplifications(SILFunction &Fn,
     return Changed;
 
   for (auto &ThreadInfo : JumpThreadableEdges) {
-    ThreadInfo.threadEdge();
+    if (ThreadInfo.threadEdge())
       Changed = true;
   }
 
@@ -922,16 +926,6 @@ bool SimplifyCFG::tryJumpThreading(BranchInst *BI) {
   if (DestBB->getTerminator()->isFunctionExiting())
     return false;
 
-  // We need to update SSA if a value duplicated is used outside of the
-  // duplicated block.
-  bool NeedToUpdateSSA = false;
-
-  // Are the arguments to this block used outside of the block.
-  for (auto Arg : DestBB->getArguments())
-    if ((NeedToUpdateSSA |= isUsedOutsideOfBlock(Arg))) {
-      break;
-    }
-
   // We don't have a great cost model at the SIL level, so we don't want to
   // blissly duplicate tons of code with a goal of improved performance (we'll
   // leave that to LLVM).  However, doing limited code duplication can lead to
@@ -978,36 +972,6 @@ bool SimplifyCFG::tryJumpThreading(BranchInst *BI) {
   if (ThreadingBudget <= 0)
     return false;
 
-  // 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;
-  SinkAddressProjections sinkProj;
-  for (auto ii = DestBB->begin(), ie = DestBB->end(); ii != ie;) {
-    copyCosts += getThreadingCost(&*ii);
-    if (ThreadingBudget <= copyCosts)
-      return false;
-
-    // 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
   // control flow. Still, we make an exception for switch_enum.
   bool DestIsLoopHeader = (LoopHeaders.count(DestBB) != 0);
@@ -1016,19 +980,31 @@ bool SimplifyCFG::tryJumpThreading(BranchInst *BI) {
       return false;
   }
 
+  // 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;
+  BasicBlockCloner Cloner(DestBB);
+  for (auto &inst : *DestBB) {
+    copyCosts += getThreadingCost(&inst);
+    if (ThreadingBudget <= copyCosts)
+      return false;
+
+    // If this is an address projection with outside uses, sink it before
+    // checking for SSA update.
+    if (!Cloner.canCloneInstruction(&inst))
+      return false;
+  }
   LLVM_DEBUG(llvm::dbgs() << "jump thread from bb" << SrcBB->getDebugID()
                           << " to bb" << DestBB->getDebugID() << '\n');
 
   JumpThreadingCost[DestBB] += copyCosts;
 
-  // Okay, it looks like we want to do this and we can.  Duplicate the
-  // destination block into this one, rewriting uses of the BBArgs to use the
-  // branch arguments as we go.
-  BasicBlockCloner Cloner(DestBB);
+  // Duplicate the destination block into this one, rewriting uses of the BBArgs
+  // to use the branch arguments as we go.
   Cloner.cloneBranchTarget(BI);
-
   // Does not currently update DominanceInfo.
   Cloner.splitCriticalEdges(nullptr, nullptr);
+  Cloner.updateSSAAfterCloning();
 
   // Once all the instructions are copied, we can nuke BI itself.  We also add
   // the threaded and edge block to the worklist now that they (likely) can be
@@ -1036,9 +1012,6 @@ bool SimplifyCFG::tryJumpThreading(BranchInst *BI) {
   addToWorklist(SrcBB);
   addToWorklist(Cloner.getNewBB());
 
-  if (NeedToUpdateSSA)
-    updateSSAAfterCloning(Cloner, Cloner.getNewBB(), DestBB);
-
   // We may be able to simplify DestBB now that it has one fewer predecessor.
   simplifyAfterDroppingPredecessor(DestBB);
 
@@ -2742,21 +2715,23 @@ bool SimplifyCFG::tailDuplicateObjCMethodCallSuccessorBlocks() {
   for (auto *BB : ObjCBlocks) {
     auto *Branch = cast<BranchInst>(BB->getTerminator());
     auto *DestBB = Branch->getDestBB();
-    Changed = true;
 
     // Okay, it looks like we want to do this and we can.  Duplicate the
     // destination block into this one, rewriting uses of the BBArgs to use the
     // branch arguments as we go.
     BasicBlockCloner Cloner(DestBB);
+    if (!Cloner.canCloneBlock())
+      continue;
+
     Cloner.cloneBranchTarget(Branch);
 
     // Does not currently update DominanceInfo.
     Cloner.splitCriticalEdges(nullptr, nullptr);
+    Cloner.updateSSAAfterCloning();
 
-    updateSSAAfterCloning(Cloner, Cloner.getNewBB(), DestBB);
+    Changed = true;
     addToWorklist(Cloner.getNewBB());
   }
-
   return Changed;
 }
 

lib/SILOptimizer/Utils/BasicBlockOptUtils.cpp

@@ -68,11 +68,19 @@ bool swift::removeUnreachableBlocks(SILFunction &f) {
   return changed;
 }
 
-/// Helper function to perform SSA updates in case of jump threading.
-void swift::updateSSAAfterCloning(BasicBlockCloner &cloner,
-                                  SILBasicBlock *srcBB, SILBasicBlock *destBB) {
+void BasicBlockCloner::updateSSAAfterCloning() {
+  // All instructions should have been checked by canCloneInstruction. But we
+  // still need to check the arguments.
+  for (auto arg : origBB->getArguments()) {
+    if ((needsSSAUpdate |= isUsedOutsideOfBlock(arg))) {
+      break;
+    }
+  }
+  if (!needsSSAUpdate)
+    return;
+
   SILSSAUpdater ssaUpdater;
-  for (auto availValPair : cloner.AvailVals) {
+  for (auto availValPair : availVals) {
     ValueBase *inst = availValPair.first;
     if (inst->use_empty())
       continue;
@@ -85,20 +93,20 @@ void swift::updateSSAAfterCloning(BasicBlockCloner &cloner,
       useList.push_back(UseWrapper(use));
 
     ssaUpdater.Initialize(inst->getType());
-    ssaUpdater.AddAvailableValue(destBB, inst);
-    ssaUpdater.AddAvailableValue(srcBB, newResult);
+    ssaUpdater.AddAvailableValue(origBB, inst);
+    ssaUpdater.AddAvailableValue(getNewBB(), newResult);
 
     if (useList.empty())
       continue;
 
     // Update all the uses.
     for (auto useWrapper : useList) {
-      Operand *use = useWrapper;
+      Operand *use = useWrapper; // unwrap
       SILInstruction *user = use->getUser();
       assert(user && "Missing user");
 
       // Ignore uses in the same basic block.
-      if (user->getParent() == destBB)
+      if (user->getParent() == origBB)
         continue;
 
       ssaUpdater.RewriteUse(*use);
@@ -128,6 +136,29 @@ bool BasicBlockCloner::splitCriticalEdges(DominanceInfo *domInfo,
   return changed;
 }
 
+void BasicBlockCloner::sinkAddressProjections() {
+  // Because the address projections chains will be disjoint (an instruction
+  // in one chain cannot use the result of an instruction in another chain),
+  // the order they are sunk does not matter.
+  for (auto ii = origBB->begin(), ie = origBB->end(); ii != ie;) {
+    bool canSink = sinkProj.analyzeAddressProjections(&*ii);
+    (void)canSink;
+    assert(canSink && "canCloneInstruction should catch this.");
+
+    sinkProj.cloneProjections();
+    assert((sinkProj.getInBlockDefs().empty() || needsSSAUpdate)
+           && "canCloneInstruction should catch this.");
+
+    auto nextII = std::next(ii);
+    recursivelyDeleteTriviallyDeadInstructions(
+        &*ii, false, [&nextII](SILInstruction *deadInst) {
+          if (deadInst->getIterator() == nextII)
+            ++nextII;
+        });
+    ii = nextII;
+  }
+}
+
 // Populate 'projections' with the chain of address projections leading
 // to and including 'inst'.
 //
@@ -149,7 +180,7 @@ bool SinkAddressProjections::analyzeAddressProjections(SILInstruction *inst) {
       return true;
     }
     if (auto *addressProj = dyn_cast<SingleValueInstruction>(def)) {
-      if (addressProj->isTriviallyDuplicatable()) {
+      if (addressProj->isPure()) {
         projections.push_back(addressProj);
         return true;
       }
@@ -183,19 +214,40 @@ bool SinkAddressProjections::cloneProjections() {
     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);
+    // Reset transient per-projection sets.
     usesToReplace.clear();
+    firstBlockUse.clear();
+    // Gather uses.
     for (Operand *use : oldProj->getUses()) {
-      if (use->getUser()->getParent() != bb)
+      auto *useBB = use->getUser()->getParent();
+      if (useBB != bb) {
+        firstBlockUse.try_emplace(useBB, use);
         usesToReplace.push_back(use);
       }
+    }
+    // Replace uses. Uses must be handled in the same order they were discovered
+    // above.
+    //
+    // Avoid cloning a projection multiple times per block. This avoids extra
+    // projections, but also prevents the removal of DebugValue. If a
+    // projection's only remaining is DebugValue, then it is deleted along with
+    // the DebugValue.
     for (Operand *use : usesToReplace) {
-      auto *newProj = oldProj->clone(use->getUser());
-      use->set(cast<SingleValueInstruction>(newProj));
+      auto *useBB = use->getUser()->getParent();
+      auto *firstUse = firstBlockUse.lookup(useBB);
+      SingleValueInstruction *newProj;
+      if (use == firstUse)
+        newProj = cast<SingleValueInstruction>(oldProj->clone(use->getUser()));
+      else {
+        newProj = cast<SingleValueInstruction>(firstUse->get());
+        assert(newProj->getParent() == useBB);
+        newProj->moveFront(useBB);
+      }
+      use->set(newProj);
     }
   }
   return true;

lib/SILOptimizer/Utils/CheckedCastBrJumpThreading.cpp

@@ -87,8 +87,8 @@ class CheckedCastBrJumpThreading {
       hasUnknownPreds(hasUnknownPreds) { }
 
     void modifyCFGForUnknownPreds();
-    void modifyCFGForFailurePreds(Optional<BasicBlockCloner> &Cloner);
-    void modifyCFGForSuccessPreds(Optional<BasicBlockCloner> &Cloner);
+    void modifyCFGForFailurePreds(BasicBlockCloner &Cloner);
+    void modifyCFGForSuccessPreds(BasicBlockCloner &Cloner);
   };
 
   // Contains an entry for each checked_cast_br to be optimized.
@@ -243,15 +243,14 @@ void CheckedCastBrJumpThreading::Edit::modifyCFGForUnknownPreds() {
 
 /// Create a copy of the BB as a landing BB
 /// for all FailurePreds.
-void CheckedCastBrJumpThreading::Edit::
-modifyCFGForFailurePreds(Optional<BasicBlockCloner> &Cloner) {
+void CheckedCastBrJumpThreading::Edit::modifyCFGForFailurePreds(
+    BasicBlockCloner &Cloner) {
   if (FailurePreds.empty())
     return;
 
-  assert(!Cloner.hasValue());
-  Cloner.emplace(CCBBlock);
-  Cloner->cloneBlock();
-  SILBasicBlock *TargetFailureBB = Cloner->getNewBB();
+  assert(!Cloner.wasCloned());
+  Cloner.cloneBlock();
+  SILBasicBlock *TargetFailureBB = Cloner.getNewBB();
   auto *TI = TargetFailureBB->getTerminator();
   SILBuilderWithScope Builder(TI);
   // This BB copy branches to a FailureBB.
@@ -271,8 +270,8 @@ modifyCFGForFailurePreds(Optional<BasicBlockCloner> &Cloner) {
 
 /// Create a copy of the BB or reuse BB as
 /// a landing basic block for all FailurePreds.
-void CheckedCastBrJumpThreading::Edit::
-modifyCFGForSuccessPreds(Optional<BasicBlockCloner> &Cloner) {
+void CheckedCastBrJumpThreading::Edit::modifyCFGForSuccessPreds(
+    BasicBlockCloner &Cloner) {
   auto *CCBI = cast<CheckedCastBranchInst>(CCBBlock->getTerminator());
 
   if (InvertSuccess) {
@@ -285,10 +284,9 @@ modifyCFGForSuccessPreds(Optional<BasicBlockCloner> &Cloner) {
     if (!SuccessPreds.empty()) {
       // Create a copy of the BB as a landing BB.
       // for all SuccessPreds.
-      assert(!Cloner.hasValue());
-      Cloner.emplace(CCBBlock);
-      Cloner->cloneBlock();
-      SILBasicBlock *TargetSuccessBB = Cloner->getNewBB();
+      assert(!Cloner.wasCloned());
+      Cloner.cloneBlock();
+      SILBasicBlock *TargetSuccessBB = Cloner.getNewBB();
       auto *TI = TargetSuccessBB->getTerminator();
       SILBuilderWithScope Builder(TI);
       // This BB copy branches to SuccessBB.
@@ -343,6 +341,11 @@ bool CheckedCastBrJumpThreading::handleArgBBIsEntryBlock(SILBasicBlock *ArgBB,
 
 // Returns false if cloning required by jump threading cannot
 // be performed, because some of the constraints are violated.
+//
+// This does not check the constraint on address projections with out-of-block
+// uses. Those are rare enough that they don't need to be checked first for
+// efficiency, but they need to be gathered later, just before cloning, anyway
+// in order to sink the projections.
 bool CheckedCastBrJumpThreading::checkCloningConstraints() {
   // Check some cloning related constraints.
 
@@ -673,7 +676,9 @@ void CheckedCastBrJumpThreading::optimizeFunction() {
   Fn->verifyCriticalEdges();
 
   for (Edit *edit : Edits) {
-    Optional<BasicBlockCloner> Cloner;
+    BasicBlockCloner Cloner(edit->CCBBlock);
+    if (!Cloner.canCloneBlock())
+      continue;
 
     // Create a copy of the BB as a landing BB
     // for all FailurePreds.
@@ -684,12 +689,11 @@ void CheckedCastBrJumpThreading::optimizeFunction() {
     // Handle unknown preds.
     edit->modifyCFGForUnknownPreds();
 
-    if (Cloner.hasValue()) {
-      updateSSAAfterCloning(*Cloner.getPointer(), Cloner->getNewBB(),
-                            edit->CCBBlock);
+    if (Cloner.wasCloned()) {
+      Cloner.updateSSAAfterCloning();
 
-      if (!Cloner->getNewBB()->pred_empty())
-        BlocksForWorklist.push_back(Cloner->getNewBB());
+      if (!Cloner.getNewBB()->pred_empty())
+        BlocksForWorklist.push_back(Cloner.getNewBB());
     }
     if (!edit->CCBBlock->pred_empty())
       BlocksForWorklist.push_back(edit->CCBBlock);

lib/SILOptimizer/Utils/SILSSAUpdater.cpp

@@ -438,7 +438,7 @@ UseWrapper::UseWrapper(Operand *Use) {
 }
 
 /// Return the operand we wrap. Reconstructing branch operands.
-UseWrapper::operator Operand *() {
+Operand *UseWrapper::getOperand() {
   switch (Type) {
   case kRegularUse:
     return U;

test/DebugInfo/inlined-generics-basic.swift

@@ -43,8 +43,8 @@ public class C<R> {
   let r : R
   init(_ _r: R) { r = _r }
 
-  // SIL: // C.f<A>(_:)
-  // IR: define {{.*}} @"$s1A1CC1fyyqd__lF"
+  // SIL-LABEL: // C.f<A>(_:)
+  // IR-LABEL: define {{.*}} @"$s1A1CC1fyyqd__lF"
 #sourceLocation(file: "f.swift", line: 1)
   public func f<S>(_ s: S) {
     // SIL: debug_value_addr %0 : $*S, let, name "s", argno 1,{{.*}} scope [[F]]
@@ -57,7 +57,13 @@ public class C<R> {
     // IR: call {{.*}}3use
 #sourceLocation(file: "f.swift", line: 2)
     g(s)
-    // IR: dbg.value({{.*}}, metadata ![[GR_T:[0-9]+]]
+    // Jump-threading removes the basic block containing debug_value
+    // "t" before the second call to `g(r)`. When this happens, the
+    // ref_element_addr in that removed block is left with a single
+    // debug_value use, so they are both deleted. This means we have
+    // no debug value for "t" in the call to `g(r)`.
+    //   dbg.value({{.*}}, metadata ![[GR_T:[0-9]+]]
+
     // IR: dbg.value({{.*}}, metadata ![[GR_U:[0-9]+]]
     // IR: call {{.*}}3use
 #sourceLocation(file: "f.swift", line: 3)
@@ -81,6 +87,8 @@ public class C<R> {
     g(false)
   }
 }
+// SIL-LABEL: } // end sil function '$s1A1CC1fyyqd__lF'
+// IR-LABEL: ret void
 
 // IR: ![[BOOL:[0-9]+]] = !DICompositeType({{.*}}name: "Bool"
 // IR: ![[LET_BOOL:[0-9]+]] = !DIDerivedType(tag: DW_TAG_const_type, baseType: ![[BOOL]])
@@ -96,9 +104,12 @@ public class C<R> {
 // IR: ![[SP_GS_T]] = {{.*}}linkageName: "$s1A1gyyxlFqd___Ti5"
 // IR: ![[GS_U]] = !DILocalVariable(name: "u", {{.*}} scope: ![[SP_GS_U:[0-9]+]], {{.*}} type: ![[LET_TAU_1_0]])
 // IR: ![[SP_GS_U]] = {{.*}}linkageName: "$s1A1hyyxlFqd___Ti5"
-// IR: ![[GR_T]] = !DILocalVariable(name: "t", {{.*}} scope: ![[SP_GR_T:[0-9]+]], {{.*}}type: ![[LET_TAU_0_0]])
+
+// Debug info for this variable is removed. See the note above the call to g(r).
+//   ![[GR_T]] = !DILocalVariable(name: "t", {{.*}} scope: ![[SP_GR_T:[0-9]+]], {{.*}}type: ![[LET_TAU_0_0]])
 // S has the same generic parameter numbering s T and U.
-// IR: ![[SP_GR_T]] = {{.*}}linkageName: "$s1A1gyyxlF"
+//   ![[SP_GR_T]] = {{.*}}linkageName: "$s1A1gyyxlF"
+
 // IR: ![[GR_U]] = !DILocalVariable(name: "u", {{.*}} scope: ![[SP_GR_U:[0-9]+]], {{.*}}type: ![[LET_TAU_0_0]])
 // IR: ![[SP_GR_U]] = {{.*}}linkageName: "$s1A1hyyxlF"
 // IR: ![[GRS_T]] = !DILocalVariable(name: "t", {{.*}} scope: ![[SP_GRS_T:[0-9]+]], {{.*}}type: ![[LET_TUPLE:[0-9]+]]

test/SILOptimizer/simplify_cfg_address_phi.sil

@@ -134,3 +134,118 @@ bb4:
 bb5:
   return %val : $Builtin.Int32
 }
+
+// Test that debug_value_addr is not unnecessarilly lost during address projection sinking.
+public class CC<R> {
+  let r : R
+  init(_ _r: R) { r = _r }
+}
+
+sil @useAny : $@convention(thin) <V> (@in_guaranteed V) -> ()
+
+// CHECK-LABEL: sil @testDebugValue : $@convention(method) <R><S> (@in_guaranteed S, @guaranteed CC<R>, Bool) -> () {
+// CHECK: debug_value_addr %0 : $*S, let, name "u"
+// CHECK: apply %{{.*}}<S>(%0) : $@convention(thin) <τ_0_0> (@in_guaranteed τ_0_0) -> ()
+// CHECK: [[FIELD:%.*]] = ref_element_addr %1 : $CC<R>, #CC.r
+// CHECK: debug_value_addr [[FIELD]] : $*R, let, name "u"
+// CHECK: apply %10<R>([[FIELD]]) : $@convention(thin) <τ_0_0> (@in_guaranteed τ_0_0) -> ()
+// CHECK-LABEL: } // end sil function 'testDebugValue'
+sil @testDebugValue : $@convention(method) <R><S> (@in_guaranteed S, @guaranteed CC<R>, Bool) -> () {
+bb0(%0 : $*S, %1 : $CC<R>, %2 : $Bool):
+  %bool = struct_extract %2 : $Bool, #Bool._value
+  cond_br %bool, bb1, bb2
+
+bb1:
+  debug_value_addr %0 : $*S, let, name "u"
+  %f1 = function_ref @useAny : $@convention(thin) <τ_0_0> (@in_guaranteed τ_0_0) -> ()
+  %call1 = apply %f1<S>(%0) : $@convention(thin) <τ_0_0> (@in_guaranteed τ_0_0) -> ()
+  br bb2
+
+bb2:
+  %field = ref_element_addr %1 : $CC<R>, #CC.r
+  debug_value_addr %field : $*R, let, name "t"
+  cond_br %bool, bb3, bb4
+
+bb3:
+  debug_value_addr %field : $*R, let, name "u"
+  %f2 = function_ref @useAny : $@convention(thin) <τ_0_0> (@in_guaranteed τ_0_0) -> ()
+  %call2 = apply %f2<R>(%field) : $@convention(thin) <τ_0_0> (@in_guaranteed τ_0_0) -> ()
+  br bb4
+
+bb4:
+  %z = tuple ()
+  return %z : $()
+}
+
+// Test multiple uses and cloned allocation.
+//
+// project_box and struct_extract_addr will be sunk into three
+// different blocks, but only once per block.
+struct S {
+  @_hasStorage @_hasInitialValue var x: Int { get set }
+  init(x: Int = 0)
+  init()
+}
+sil @doNothing : $@convention(thin) (@inout Int) -> ()
+
+// CHECK-LABEL: sil @testMultiUse : $@convention(method) (Bool, @inout Int) -> () {
+// CHECK: bb0(%0 : $Bool, %1 : $*Int):
+// CHECK:   cond_br %{{.*}}, bb2, bb1
+// CHECK: bb1:
+// CHECK:   [[ALLOC1:%.*]] = alloc_box ${ var S }, var, name "s"
+// CHECK:   [[PROJ1:%.*]] = project_box [[ALLOC1]] : ${ var S }, 0
+// CHECK:   [[ADR1:%.*]] = struct_element_addr [[PROJ1]] : $*S, #S.x
+// CHECK:   store %{{.*}} to [[ADR1]] : $*Int
+// CHECK:   br bb3([[ALLOC1]] : ${ var S })
+// CHECK: bb2:
+// CHECK:   apply %{{.*}}(%1) : $@convention(thin) (@inout Int) -> ()
+// CHECK:   [[ALLOC2:%.*]] = alloc_box ${ var S }, var, name "s"
+// CHECK:   [[PROJ2:%.*]] = project_box [[ALLOC2]] : ${ var S }, 0
+// CHECK:   [[ADR2:%.*]] = struct_element_addr [[PROJ2]] : $*S, #S.x
+// CHECK:   store %{{.*}} to [[ADR2]] : $*Int
+// CHECK:   apply %{{.*}}([[ADR2]]) : $@convention(thin) (@inout Int) -> ()
+// CHECK:   br bb3([[ALLOC2]] : ${ var S })
+// CHECK: bb3([[BOXARG:%.*]] : ${ var S }):
+// CHECK:   [[PROJ3:%.*]] = project_box [[BOXARG]] : ${ var S }, 0
+// CHECK:   [[ADR3:%.*]] = struct_element_addr [[PROJ3]] : $*S, #S.x
+// CHECK:   apply %{{.*}}([[ADR3]]) : $@convention(thin) (@inout Int) -> ()
+// CHECK:   release_value [[BOXARG]] : ${ var S }
+// CHECK-LABEL: } // end sil function 'testMultiUse'
+sil @testMultiUse : $@convention(method) (Bool, @inout Int) -> () {
+bb0(%0 : $Bool, %1 : $*Int):
+  %bool = struct_extract %0 : $Bool, #Bool._value
+  cond_br %bool, bb1, bb2
+
+bb1:
+  %f1 = function_ref @doNothing : $@convention(thin) (@inout Int) -> ()
+  %call1 = apply %f1(%1) : $@convention(thin) (@inout Int) -> ()
+  br bb3
+
+bb2:
+  br bb3
+
+bb3:
+  %box3 = alloc_box ${ var S }, var, name "s"
+  %proj3 = project_box %box3 : ${ var S }, 0
+  %adr3 = struct_element_addr %proj3 : $*S, #S.x
+  cond_br %bool, bb4, bb5
+
+bb4:
+  %i4 = load %1 : $*Int
+  store %i4 to %adr3 : $*Int
+  %f2 = function_ref @doNothing : $@convention(thin) (@inout Int) -> ()
+  %call2 = apply %f2(%adr3) : $@convention(thin) (@inout Int) -> ()
+  br bb6
+
+bb5:
+  %i5 = load %1 : $*Int
+  store %i5 to %adr3 : $*Int
+  br bb6
+
+bb6:
+  %f6 = function_ref @doNothing : $@convention(thin) (@inout Int) -> ()
+  %call6 = apply %f6(%adr3) : $@convention(thin) (@inout Int) -> ()
+  release_value %box3 : ${ var S }
+  %z = tuple ()
+  return %z : $()
+}