Merge pull request #85855 from eeckstein/fix-silcombine

DeadCodeElimination,SILCombine: don't insert destroys for removed values in dead-end blocks

Author: eeckstein

lib/SILOptimizer/SILCombiner/SILCombinerApplyVisitors.cpp

@@ -1561,14 +1561,6 @@ SILInstruction *SILCombiner::legacyVisitApplyInst(ApplyInst *AI) {
   }
 
   if (SF) {
-    if (SF->hasSemanticsAttr(semantics::ARRAY_UNINITIALIZED)) {
-      UserListTy Users;
-      // If the uninitialized array is only written into then it can be removed.
-      if (recursivelyCollectARCUsers(Users, AI)) {
-        if (eraseApply(AI, Users))
-          return nullptr;
-      }
-    }
     if (SF->hasSemanticsAttr(semantics::ARRAY_GET_CONTIGUOUSARRAYSTORAGETYPE)) {
       auto silTy = AI->getType();
       auto storageTy = AI->getType().getASTType();

lib/SILOptimizer/Transforms/DeadCodeElimination.cpp

@@ -203,6 +203,7 @@ class DCE {
 
   void markValueLive(SILValue V);
   void markInstructionLive(SILInstruction *Inst);
+  void markOwnedDeadValueLive(SILValue v);
   void markTerminatorArgsLive(SILBasicBlock *Pred, SILBasicBlock *Succ,
                               size_t ArgIndex);
   void markControllingTerminatorsLive(SILBasicBlock *Block);
@@ -265,6 +266,20 @@ void DCE::markInstructionLive(SILInstruction *Inst) {
   Worklist.push_back(Inst);
 }
 
+void DCE::markOwnedDeadValueLive(SILValue v) {
+  if (v->getOwnershipKind() == OwnershipKind::Owned) {
+    // When an owned value has no lifetime ending uses it means that it is in a
+    // dead-end region. We must not remove and inserting compensating destroys
+    // for it because that would potentially destroy the value too early.
+    // TODO: we can remove this once we have complete OSSA lifetimes
+    for (Operand *use : v->getUses()) {
+      if (use->isLifetimeEnding())
+        return;
+    }
+    markValueLive(v);
+  }
+}
+
 /// Gets the producing instruction of a cond_fail condition. Currently these
 /// are overflow builtins but may be extended to other instructions in the
 /// future.
@@ -331,6 +346,9 @@ void DCE::markLive() {
   // to be live in the sense that they are not trivially something we
   // can delete by examining only that instruction.
   for (auto &BB : *F) {
+    for (SILArgument *arg : BB.getArguments()) {
+      markOwnedDeadValueLive(arg);
+    }
     for (auto &I : BB) {
       switch (I.getKind()) {
       case SILInstructionKind::CondFailInst: {
@@ -395,6 +413,9 @@ void DCE::markLive() {
       default:
         if (seemsUseful(&I))
           markInstructionLive(&I);
+        for (SILValue result : I.getResults()) {
+          markOwnedDeadValueLive(result);
+        }
       }
     }
   }

test/SILOptimizer/dead_code_elimination_ossa.sil

@@ -12,6 +12,7 @@ public struct S {
 typealias Int1 = Builtin.Int1
 
 class C {}
+class D: C {}
 
 sil @getC : $@convention(thin) () -> @owned C
 sil @barrier : $@convention(thin) () -> ()
@@ -534,3 +535,32 @@ bb2:
   %17 = tuple ()
   return %17
 }
+
+// CHECK-LABEL: sil [ossa] @dont_insert_destroy_for_uninitialized_object :
+// CHECK-NOT:     destroy_value
+// CHECK-LABEL: } // end sil function 'dont_insert_destroy_for_uninitialized_object'
+sil [ossa] @dont_insert_destroy_for_uninitialized_object : $@convention(thin) () -> () {
+bb0:
+  %0 = alloc_ref $D      // D might not be initialized due to dead-end loop -> will lead to crash if it is destroyed here
+  %1 = upcast %0 to $C
+  br bb1
+
+bb1:
+  br bb1
+}
+
+// CHECK-LABEL: sil [ossa] @dont_insert_arg_destroy_for_uninitialized_object :
+// CHECK-NOT:     destroy_value
+// CHECK-LABEL: } // end sil function 'dont_insert_arg_destroy_for_uninitialized_object'
+sil [ossa] @dont_insert_arg_destroy_for_uninitialized_object : $@convention(thin) () -> () {
+bb0:
+  %0 = alloc_ref $D      // D might not be initialized due to dead-end loop -> will lead to crash if it is destroyed here
+  br bb1(%0)
+
+bb1(%2 : @owned $D):
+  br bb2
+
+bb2:
+  br bb2
+}
+

test/SILOptimizer/deadend-loop-crash.swift

@@ -0,0 +1,35 @@
+// RUN: %target-run-simple-swift(-O -Xllvm -sil-disable-pass=deadobject-elim) | %FileCheck %s
+
+// REQUIRES: executable_test
+
+#if canImport(Darwin)
+  import Darwin
+#elseif canImport(Glibc)
+  import Glibc
+#elseif os(WASI)
+  import WASILibc
+#elseif canImport(Android)
+  import Android
+#elseif os(Windows)
+  import CRT
+#else
+#error("Unsupported platform")
+#endif
+
+@inline(never)
+public func hiddenExit() {
+  // CHECK: okay
+  print("okay")
+  exit(0)
+}
+
+func foo() -> Never {
+  while true {
+    hiddenExit()
+  }
+}
+
+func bar(_ x: Any...) {
+}
+
+bar(foo())

test/SILOptimizer/sil_combine_apply.sil

@@ -1037,3 +1037,22 @@ bb3:
   %rv = tuple()
   return %rv : $()
 }
+
+
+sil [_semantics "array.uninitialized"] [ossa] @adoptStorage : $@convention(thin) (@owned _ContiguousArrayStorage<Any>, Int) -> (@owned Array<Any>, UnsafeMutablePointer<Any>)
+
+// CHECK-LABEL: sil [ossa] @dont_remove_dead_adopt_storage :
+// CHECK-NOT:     destroy_value
+// CHECK-LABEL: } // end sil function 'dont_remove_dead_adopt_storage'
+sil [ossa] @dont_remove_dead_adopt_storage : $@convention(thin) (Int) -> () {
+bb0(%0 : $Int):
+  %1 = integer_literal $Builtin.Word, 1
+  %2 = alloc_ref [tail_elems $Any * %1] $_ContiguousArrayStorage<Any>
+  %3 = function_ref @adoptStorage : $@convention(thin) (@owned _ContiguousArrayStorage<Any>, Int) -> (@owned Array<Any>, UnsafeMutablePointer<Any>)
+  %4 = apply %3(%2, %0) : $@convention(thin) (@owned _ContiguousArrayStorage<Any>, Int) -> (@owned Array<Any>, UnsafeMutablePointer<Any>)
+  // Array buffer is not initialized due to dead-end loop -> will lead to crash if buffer is destroyed here
+  br bb1
+bb1:
+  br bb1
+}
+