Merge pull request #19103 from atrick/cpf-bug

Fix a bug in CopyForwarding. Bailout during destoy hoisting.

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 : $()
+}