Fix a bug in CopyForwarding. Bailout during destoy hoisting.

Once the algorithm has begun hoisting destroys globally, there's no
way to cleanly bailout. The previous attempt to bailout could result
in an assert or lost destroy in release mode.

This is continued fall-out from changes in the previous release to
upstream SILGen or mandatory passes, such as PredictableMemOps, that
no longer preserve natural variable lifetimes.

In this case, we end up with SIL like this before CopyForwarding:

bb(%arg)
%local_addr = alloc_stack
store %arg to %local_addr
%payload = switch_enum(%arg)
retain %arg
store %arg to %some_addr
destroy_addr %local_addr
release_value %arg

We're attempting to hoist the destroy_addr to its last use, but can't
because the lifetimes of the alloc_stack (%local_addr) and the value
being stored on the stack (%arg) have become mixed up by an upstream
pass. We actually detect this situation now in order to bail-out of
destroy hoisting. Sadly, the bailout might only partially recover in
the case of interesting control flow, as happens in the test case's
Graph.init function. This triggers an assert, but in release mode it
simply drops the destroy.

Fixed <rdar://problem/43888666> [SR-8526]: Memory leak after switch in
release configuration.

Author: atrick

lib/SILOptimizer/Transforms/CopyForwarding.cpp

@@ -1200,8 +1200,19 @@ bool CopyForwarding::hoistDestroy(SILInstruction *DestroyPoint,
 
   // If DestroyPoint is a block terminator, we must hoist.
   bool MustHoist = (DestroyPoint == BB->getTerminator());
+  // If we haven't seen anything significant, avoid useless hoisting.
+  bool ShouldHoist = MustHoist;
+
+  auto tryToInsertHoistedDestroyAfter = [&](SILInstruction *afterInst) {
+    if (!ShouldHoist)
+      return false;
+    LLVM_DEBUG(llvm::dbgs() << "  Hoisting to Use:" << *afterInst);
+    SILBuilderWithScope(std::next(afterInst->getIterator()), afterInst)
+        .createDestroyAddr(DestroyLoc, CurrentDef);
+    HasChanged = true;
+    return true;
+  };
 
-  bool IsWorthHoisting = MustHoist;
   auto SI = DestroyPoint->getIterator(), SE = BB->begin();
   while (SI != SE) {
     --SI;
@@ -1219,10 +1230,10 @@ bool CopyForwarding::hoistDestroy(SILInstruction *DestroyPoint,
         // destroy_addr CurrentDef
         LLVM_DEBUG(llvm::dbgs() << "  Cannot hoist above stored value use:"
                                 << *Inst);
-        return false;
+        return tryToInsertHoistedDestroyAfter(Inst);
       }
-      if (!IsWorthHoisting && isa<ApplyInst>(Inst))
-        IsWorthHoisting = true;
+      if (!ShouldHoist && isa<ApplyInst>(Inst))
+        ShouldHoist = true;
       continue;
     }
     if (auto *CopyInst = dyn_cast<CopyAddrInst>(Inst)) {
@@ -1233,19 +1244,15 @@ bool CopyForwarding::hoistDestroy(SILInstruction *DestroyPoint,
           return true;
       }
     }
-    // We reached a user of CurrentDef. If we haven't seen anything significant,
-    // avoid useless hoisting.
-    if (!IsWorthHoisting)
-      return false;
-
-    LLVM_DEBUG(llvm::dbgs() << "  Hoisting to Use:" << *Inst);
-    SILBuilderWithScope(std::next(SI), Inst)
-        .createDestroyAddr(DestroyLoc, CurrentDef);
-    HasChanged = true;
-    return true;
+    return tryToInsertHoistedDestroyAfter(Inst);
   }
-  if (!DoGlobalHoisting)
+  if (!DoGlobalHoisting) {
+    // If DoGlobalHoisting is set, then we should never mark a DeadInBlock, so
+    // MustHoist should be false.
+    assert(!MustHoist &&
+           "Cannot hoist above a terminator with global hoisting disabled.");
     return false;
+  }
   DeadInBlocks.insert(BB);
   return true;
 }

test/SILOptimizer/copyforward.sil

@@ -883,3 +883,35 @@ bb0(%0 : $*AClass, %1 : $AClass):
   %999 = tuple ()
   return %999 : $()
 }
+
+// <rdar://problem/43888666> [SR-8526]: Memory leak after switch in release configuration
+// CHECK-LABEL: sil @testGlobalHoistToStoredValue : $@convention(thin) (@owned AClass, @inout AClass) -> () {
+// CHECK: bb0(%0 : $AClass, %1 : $*AClass):
+// CHECK-NEXT:   [[LOCAL:%.*]] = alloc_stack $AClass
+// CHECK-NEXT:   store %0 to [[LOCAL]] : $*AClass
+// CHECK-NEXT:   copy_addr [[LOCAL]] to %1 : $*AClass
+// CHECK-NEXT:   retain_value %0 : $AClass
+// CHECK-NEXT:   store %0 to %1 : $*AClass
+// CHECK-NEXT:   destroy_addr [[LOCAL]] : $*AClass
+// CHECK-NEXT:   br bb1
+// CHECK: bb1:
+// CHECK-NEXT:   dealloc_stack [[LOCAL]] : $*AClass
+// CHECK-NEXT:   release_value %0 : $AClass
+// CHECK-NEXT:   tuple ()
+// CHECK-NEXT:   return
+// CHECK-LABEL: } // end sil function 'testGlobalHoistToStoredValue'
+sil @testGlobalHoistToStoredValue : $@convention(thin) (@owned AClass, @inout AClass) -> () {
+bb0(%obj : $AClass, %ptr : $*AClass ):
+  %local = alloc_stack $AClass
+  store %obj to %local : $*AClass
+  copy_addr %local to %ptr : $*AClass
+  retain_value %obj : $AClass
+  store %obj to %ptr : $*AClass
+  br bb1
+bb1:
+  destroy_addr %local : $*AClass
+  dealloc_stack %local : $*AClass
+  release_value %obj : $AClass
+  %v = tuple ()
+  return %v : $()
+}