Merge pull request swiftlang#35057 from eeckstein/fix-dead-object-elim

DeadObjectElimination: don't remove a dead alloc_ref which has a store to a non-trivial property.

Author: eeckstein

lib/SILOptimizer/Transforms/DeadObjectElimination.cpp

@@ -251,21 +251,24 @@ static bool canZapInstruction(SILInstruction *Inst, bool acceptRefCountInsts,
 /// Analyze the use graph of AllocRef for any uses that would prevent us from
 /// zapping it completely.
 static bool
-hasUnremovableUsers(SILInstruction *AllocRef, UserList &Users,
+hasUnremovableUsers(SILInstruction *AllocRef, UserList *Users,
                     bool acceptRefCountInsts, bool onlyAcceptTrivialStores) {
   SmallVector<SILInstruction *, 16> Worklist;
   Worklist.push_back(AllocRef);
 
   LLVM_DEBUG(llvm::dbgs() << "    Analyzing Use Graph.");
 
+  SmallVector<RefElementAddrInst *, 8> refElementAddrs;
+  bool deallocationMaybeInlined = false;
+
   while (!Worklist.empty()) {
     SILInstruction *I = Worklist.pop_back_val();
 
     LLVM_DEBUG(llvm::dbgs() << "        Visiting: " << *I);
 
     // Insert the instruction into our InvolvedInstructions set.  If we have
     // already seen it, then don't reprocess all of the uses.
-    if (!Users.insert(I)) {
+    if (Users && !Users->insert(I)) {
       LLVM_DEBUG(llvm::dbgs() << "        Already seen skipping...\n");
       continue;
     }
@@ -276,6 +279,13 @@ hasUnremovableUsers(SILInstruction *AllocRef, UserList &Users,
       return true;
     }
 
+    if (auto *rea = dyn_cast<RefElementAddrInst>(I)) {
+      if (!rea->getType().isTrivial(*rea->getFunction()))
+        refElementAddrs.push_back(rea);
+    } else if (isa<SetDeallocatingInst>(I)) {
+      deallocationMaybeInlined = true;
+    }
+
     // At this point, we can remove the instruction as long as all of its users
     // can be removed as well. Scan its users and add them to the worklist for
     // recursive processing.
@@ -299,6 +309,20 @@ hasUnremovableUsers(SILInstruction *AllocRef, UserList &Users,
     }
   }
 
+  if (deallocationMaybeInlined) {
+    // The alloc_ref is not destructed by a strong_release which is calling the
+    // deallocator (destroying all stored properties).
+    // In non-OSSA we cannot reliably track the lifetime of non-trivial stored
+    // properties. Removing the dead alloc_ref might leak a property value.
+    // TODO: in OSSA we can replace stores to properties with a destroy_value.
+    for (RefElementAddrInst *rea : refElementAddrs) {
+      // Re-run the check with not accepting non-trivial stores.
+      if (hasUnremovableUsers(rea, nullptr, acceptRefCountInsts,
+                              /*onlyAcceptTrivialStores*/ true))
+        return true;
+    }
+  }
+
   return false;
 }
 
@@ -736,7 +760,7 @@ bool DeadObjectElimination::processAllocRef(AllocRefInst *ARI) {
   // Our destructor has no side effects, so if we can prove that no loads
   // escape, then we can completely remove the use graph of this alloc_ref.
   UserList UsersToRemove;
-  if (hasUnremovableUsers(ARI, UsersToRemove,
+  if (hasUnremovableUsers(ARI, &UsersToRemove,
                           /*acceptRefCountInsts=*/ !HasSideEffects,
                           /*onlyAcceptTrivialStores*/false)) {
     LLVM_DEBUG(llvm::dbgs() << "    Found a use that cannot be zapped...\n");
@@ -756,7 +780,7 @@ bool DeadObjectElimination::processAllocStack(AllocStackInst *ASI) {
   // Trivial types don't have destructors.
   bool isTrivialType = ASI->getElementType().isTrivial(*ASI->getFunction());
   UserList UsersToRemove;
-  if (hasUnremovableUsers(ASI, UsersToRemove, /*acceptRefCountInsts=*/ true,
+  if (hasUnremovableUsers(ASI, &UsersToRemove, /*acceptRefCountInsts=*/ true,
       /*onlyAcceptTrivialStores*/!isTrivialType)) {
     LLVM_DEBUG(llvm::dbgs() << "    Found a use that cannot be zapped...\n");
     return false;
@@ -773,7 +797,7 @@ bool DeadObjectElimination::processAllocStack(AllocStackInst *ASI) {
 
 bool DeadObjectElimination::processKeyPath(KeyPathInst *KPI) {
   UserList UsersToRemove;
-  if (hasUnremovableUsers(KPI, UsersToRemove, /*acceptRefCountInsts=*/ true,
+  if (hasUnremovableUsers(KPI, &UsersToRemove, /*acceptRefCountInsts=*/ true,
       /*onlyAcceptTrivialStores*/ false)) {
     LLVM_DEBUG(llvm::dbgs() << "    Found a use that cannot be zapped...\n");
     return false;

test/SILOptimizer/dead_alloc_elim.sil

@@ -14,7 +14,11 @@ class TrivialDestructor {
   deinit { }
 }
 
+class Kl {}
+
 class NontrivialDestructor {
+  @_hasStorage var p : Kl 
+  @_hasStorage var i : Int 
   init()
 }
 
@@ -79,6 +83,53 @@ sil @devirtualized_destructor : $@convention(thin) () -> () {
   return %1 : $()
 }
 
+// In non-OSSA we cannot remove alloc_refs with stores to non-trivial properties.
+//
+// CHECK-LABEL: sil @store_to_non_trivial_property1
+// CHECK:   alloc_ref [stack] $NontrivialDestructor
+// CHECK: } // end sil function 'store_to_non_trivial_property1'
+sil @store_to_non_trivial_property1 : $@convention(thin) (@owned Kl) -> () {
+bb0(%0 : $Kl):
+  %20 = alloc_ref [stack] $NontrivialDestructor
+  %21 = ref_element_addr %20 : $NontrivialDestructor, #NontrivialDestructor.p
+  store %0 to %21 : $*Kl
+  set_deallocating %20 : $NontrivialDestructor
+  destroy_addr %21 : $*Kl
+  dealloc_ref [stack] %20 : $NontrivialDestructor
+  %r = tuple ()
+  return %r : $()
+}
+
+// CHECK-LABEL: sil @store_to_non_trivial_property2
+// CHECK:   alloc_ref [stack] $NontrivialDestructor
+// CHECK: } // end sil function 'store_to_non_trivial_property2'
+sil @store_to_non_trivial_property2 : $@convention(thin) (@owned Kl) -> () {
+bb0(%0 : $Kl):
+  %20 = alloc_ref [stack] $NontrivialDestructor
+  %21 = ref_element_addr %20 : $NontrivialDestructor, #NontrivialDestructor.p
+  store %0 to %21 : $*Kl
+  strong_release %20 : $NontrivialDestructor
+  dealloc_ref [stack] %20 : $NontrivialDestructor
+  %r = tuple ()
+  return %r : $()
+}
+
+// CHECK-LABEL: sil @store_to_trivial_property
+// CHECK-NOT: alloc_ref
+// CHECK: } // end sil function 'store_to_trivial_property'
+sil @store_to_trivial_property : $@convention(thin) (Int) -> () {
+bb0(%0 : $Int):
+  %20 = alloc_ref [stack] $NontrivialDestructor
+  %21 = ref_element_addr %20 : $NontrivialDestructor, #NontrivialDestructor.i
+  store %0 to %21 : $*Int
+  set_deallocating %20 : $NontrivialDestructor
+  dealloc_ref %20 : $NontrivialDestructor
+  dealloc_ref [stack] %20 : $NontrivialDestructor
+  %r = tuple ()
+  return %r : $()
+}
+
+
 // We load/use a pointer from the alloc_ref, do nothing.
 //
 // CHECK-LABEL: sil @trivial_destructor_load : $@convention(thin) () -> () {