[move-only] Fix SILOptimizer code motion to preserve value deinits

Multiple code motion and ARC related passes were removing struct/enum
deinits.

Passes fixed include:
- SILCombine
- EarlyCodeMotion
- ReleaseHoisting
- *many* passes that rely on ARC analysis (RCIndentity)

Author: atrick

include/swift/SILOptimizer/Utils/InstOptUtils.h

@@ -141,6 +141,16 @@ void collectUsesOfValue(SILValue V,
 /// value itself)
 void eraseUsesOfValue(SILValue value);
 
+/// Return true if \p type is a value type (struct/enum) that requires
+/// deinitialization beyond destruction of its members.
+bool hasValueDeinit(SILType type);
+
+/// Return true if \p value has a value type (struct/enum) that requires
+/// deinitialization beyond destruction of its members.
+inline bool hasValueDeinit(SILValue value) {
+  return hasValueDeinit(value->getType());
+}
+
 /// Gets the concrete value which is stored in an existential box.
 /// Returns %value in following pattern:
 ///

lib/SILOptimizer/Analysis/RCIdentityAnalysis.cpp

@@ -12,6 +12,7 @@
 
 #include "swift/SILOptimizer/Analysis/RCIdentityAnalysis.h"
 #include "swift/SILOptimizer/Analysis/DominanceAnalysis.h"
+#include "swift/SILOptimizer/Utils/InstOptUtils.h"
 #include "swift/SIL/SILInstruction.h"
 #include "swift/SIL/DynamicCasts.h"
 #include "llvm/Support/CommandLine.h"
@@ -79,26 +80,29 @@ static SILValue stripRCIdentityPreservingInsts(SILValue V) {
   // the struct is equivalent to a ref count operation on the extracted
   // member. Strip off the extract.
   if (auto *SEI = dyn_cast<StructExtractInst>(V))
-    if (SEI->isFieldOnlyNonTrivialField())
+    if (SEI->isFieldOnlyNonTrivialField() && !hasValueDeinit(SEI->getOperand()))
       return SEI->getOperand();
 
   // If we have a struct instruction with only one non-trivial stored field, the
   // only reference count that can be modified is the non-trivial field. Return
   // the non-trivial field.
-  if (auto *SI = dyn_cast<StructInst>(V))
-    if (SILValue NewValue = SI->getUniqueNonTrivialFieldValue())
-      return NewValue;
-
+  if (auto *SI = dyn_cast<StructInst>(V)) {
+    if (!hasValueDeinit(SI)) {
+      if (SILValue NewValue = SI->getUniqueNonTrivialFieldValue())
+        return NewValue;
+    }
+  }
   // If we have an unchecked_enum_data, strip off the unchecked_enum_data.
-  if (auto *UEDI = dyn_cast<UncheckedEnumDataInst>(V))
-    return UEDI->getOperand();
-
+  if (auto *UEDI = dyn_cast<UncheckedEnumDataInst>(V)) {
+    if (!hasValueDeinit(UEDI->getOperand()))
+      return UEDI->getOperand();
+  }
   // If we have an enum instruction with a payload, strip off the enum to
   // expose the enum's payload.
-  if (auto *EI = dyn_cast<EnumInst>(V))
-    if (EI->hasOperand())
+  if (auto *EI = dyn_cast<EnumInst>(V)) {
+    if (EI->hasOperand() && !hasValueDeinit(EI))
       return EI->getOperand();
-
+  }
   // If we have a tuple_extract that is extracting the only non trivial member
   // of a tuple, a retain_value on the tuple is equivalent to a retain_value on
   // the extracted value.
@@ -114,8 +118,10 @@ static SILValue stripRCIdentityPreservingInsts(SILValue V) {
       return NewValue;
 
   if (auto *result = SILArgument::isTerminatorResult(V)) {
-    if (auto *forwardedOper = result->forwardedTerminatorResultOperand())
-      return forwardedOper->get();
+    if (auto *forwardedOper = result->forwardedTerminatorResultOperand()) {
+      if (!hasValueDeinit(forwardedOper->get()))
+        return forwardedOper->get();
+    }
   }
 
   // Handle useless single-predecessor phis for legacy reasons. (Although these
@@ -307,7 +313,7 @@ static SILValue allIncomingValuesEqual(
 SILValue RCIdentityFunctionInfo::stripRCIdentityPreservingArgs(SILValue V,
                                                       unsigned RecursionDepth) {
   auto *A = dyn_cast<SILPhiArgument>(V);
-  if (!A) {
+  if (!A || !A->isPhi()) {
     return SILValue();
   }
 
@@ -503,7 +509,7 @@ static bool isNonOverlappingTrivialAccess(SILValue value) {
   if (auto *SEI = dyn_cast<StructExtractInst>(value)) {
     // If the struct we are extracting from only has one non trivial element and
     // we are not extracting from that element, this is an ARC escape.
-    return SEI->isTrivialFieldOfOneRCIDStruct();
+    return SEI->isTrivialFieldOfOneRCIDStruct() && !hasValueDeinit(SEI);
   }
 
   return false;

lib/SILOptimizer/SILCombiner/SILCombinerMiscVisitors.cpp

@@ -866,7 +866,7 @@ SILInstruction *SILCombiner::visitReleaseValueInst(ReleaseValueInst *RVI) {
 
     // retain_value of an enum_inst where we know that it has a payload can be
     // reduced to a retain_value on the payload.
-    if (EI->hasOperand()) {
+    if (EI->hasOperand() && !hasValueDeinit(EI)) {
       return Builder.createReleaseValue(RVI->getLoc(), EI->getOperand(),
                                         RVI->getAtomicity());
     }

lib/SILOptimizer/Transforms/SILCodeMotion.cpp

@@ -564,6 +564,10 @@ bool BBEnumTagDataflowState::visitReleaseValueInst(ReleaseValueInst *RVI) {
   if (FindResult == ValueToCaseMap.end())
     return false;
 
+  // If the enum has a deinit, preserve the original release.
+  if (hasValueDeinit(RVI->getOperand()))
+    return false;
+
   // If we do not have any argument, just delete the release value.
   if (!(*FindResult)->second->hasAssociatedValues()) {
     RVI->eraseFromParent();
@@ -621,6 +625,10 @@ bool BBEnumTagDataflowState::hoistDecrementsIntoSwitchRegions(
       continue;
     }
 
+    // If the enum has a deinit, preserve the original release.
+    if (hasValueDeinit(Op))
+      return false;
+
     auto &EnumBBCaseList = (*R)->second;
     // If we don't have an enum tag for each predecessor of this BB, bail since
     // we do not know how to handle that BB.
@@ -1508,6 +1516,10 @@ static bool tryToSinkRefCountAcrossSwitch(SwitchEnumInst *Switch,
       RCIA->getRCIdentityRoot(Switch->getOperand()))
     return false;
 
+  // If the enum has a deinit, preserve the original release.
+  assert(!hasValueDeinit(Ptr) &&
+         "enum with deinit is not RC-identical to its payload");
+
   // If S has a default case bail since the default case could represent
   // multiple cases.
   //
@@ -1578,6 +1590,10 @@ static bool tryToSinkRefCountAcrossSelectEnum(CondBranchInst *CondBr,
       RCIA->getRCIdentityRoot(SEI->getEnumOperand()))
     return false;
 
+  // If the enum has a deinit, preserve the original release.
+  assert(!hasValueDeinit(Ptr) &&
+         "enum with deinit is not RC-identical to its payload");
+
   // Work out which enum element is the true branch, and which is false.
   // If the enum only has 2 values and its tag isn't the true branch, then we
   // know the true branch must be the other tag.

lib/SILOptimizer/Utils/InstOptUtils.cpp

@@ -309,6 +309,15 @@ void swift::eraseUsesOfValue(SILValue v) {
   }
 }
 
+bool swift::hasValueDeinit(SILType type) {
+  // Do not look inside an aggregate type that has a user-deinit, for which
+  // memberwise-destruction is not equivalent to aggregate destruction.
+  if (auto *nominal = type.getNominalOrBoundGenericNominal()) {
+    return nominal->getValueTypeDestructor() != nullptr;
+  }
+  return false;
+}
+
 SILValue swift::
 getConcreteValueOfExistentialBox(AllocExistentialBoxInst *existentialBox,
                                   SILInstruction *ignoreUser) {

test/SILOptimizer/earlycodemotion_moveonly.sil

@@ -0,0 +1,38 @@
+// RUN: %target-sil-opt -enable-sil-verify-all %s -early-codemotion -retain-sinking -enable-experimental-feature MoveOnlyEnumDeinits | %FileCheck %s
+
+sil_stage canonical
+
+import Builtin
+
+struct FileDescriptor: ~Copyable {
+  var fd: Builtin.Int64
+
+  init(fd: Builtin.Int64)
+  deinit
+}
+
+enum MaybeFileDescriptor: ~Copyable {
+  case some(FileDescriptor)
+  case nothing
+
+  consuming func discard() { discard self }
+
+  deinit
+}
+
+
+// Test that a release_value is not replaced for a enum-with-deinit.
+// Doing so would forget the deinit.
+//
+// CHECK-LABEL: sil hidden [noinline] @testEnumDeinit : $@convention(thin) () -> () {
+// CHECK:   release_value %{{.*}} : $MaybeFileDescriptor
+// CHECK-LABEL: } // end sil function 'testEnumDeinit'
+sil hidden [noinline] @testEnumDeinit : $@convention(thin) () -> () {
+bb0:
+  %0 = integer_literal $Builtin.Int64, 0
+  %26 = struct $FileDescriptor (%0 : $Builtin.Int64)
+  %38 = enum $MaybeFileDescriptor, #MaybeFileDescriptor.some!enumelt, %26 : $FileDescriptor
+  release_value %38 : $MaybeFileDescriptor
+  %42 = tuple ()
+  return %42 : $()
+}

test/SILOptimizer/late_release_hoisting.sil

@@ -31,6 +31,15 @@ class HasHasObj {
   var ho : HasObj
 }
 
+public final class C {}
+
+@_moveOnly
+struct FD {
+  var c = C()
+
+  deinit {}
+}
+
 sil @$s4test8HasInt64CfD : $@convention(method) (@owned HasInt64) -> () {
 [%0: noescape **]
 }
@@ -308,3 +317,16 @@ bb0(%0 : $Int64, %1 : $HasObj):
   strong_release %lo : $HasHasInt64
   return %o : $AnyObject
 }
+
+// Test that a struct-with-deinit is not RC-identical to its single member.
+//
+// CHECK-LABEL: sil hidden @fdDeinit : $@convention(method) (@owned FD) -> () {
+// CHECK:         strong_release %{{.*}} : $C
+// CHECK-LABEL: } // end sil function 'fdDeinit'
+sil hidden @fdDeinit : $@convention(method) (@owned FD) -> () {
+bb0(%0 : $FD):
+  %2 = struct_extract %0 : $FD, #FD.c
+  strong_release %2 : $C
+  %4 = tuple ()
+  return %4 : $()
+}

test/SILOptimizer/sil_combine_moveonly.sil

@@ -1,4 +1,4 @@
-// RUN: %target-sil-opt -enable-sil-verify-all -sil-combine %s | %FileCheck %s
+// RUN: %target-sil-opt -enable-experimental-feature MoveOnlyEnumDeinits -enable-sil-verify-all -sil-combine %s | %FileCheck %s
 
  sil_stage canonical
 
@@ -8,21 +8,52 @@
    deinit {}
  }
 
- // Test that a release_value is not removed for a struct-with-deinit.
- // Doing so would forget the deinit.
- //
- // public func testStructDeinit() {
- //   var s = S()
- // }
- //
- // CHECK-LABEL: sil @testStructDeinit : $@convention(thin) () -> () {
- // CHECK:         release_value %{{.*}} : $S
- // CHECK-LABEL: } // end sil function 'testStructDeinit'
- sil @testStructDeinit : $@convention(thin) () -> () {
- bb0:
-   %0 = struct $S ()
-   release_value %0 : $S
-   %5 = tuple ()
-   return %5 : $()
- }
- 
+struct FileDescriptor: ~Copyable {
+  var fd: Builtin.Int64
+
+  init(fd: Builtin.Int64)
+  deinit
+}
+
+enum MaybeFileDescriptor: ~Copyable {
+  case some(FileDescriptor)
+  case nothing
+
+  consuming func discard() { discard self }
+
+  deinit
+}
+
+// Test that a release_value is not removed for a struct-with-deinit.
+// Doing so would forget the deinit.
+//
+// public func testStructDeinit() {
+//   var s = S()
+// }
+//
+// CHECK-LABEL: sil @testStructDeinit : $@convention(thin) () -> () {
+// CHECK:         release_value %{{.*}} : $S
+// CHECK-LABEL: } // end sil function 'testStructDeinit'
+sil @testStructDeinit : $@convention(thin) () -> () {
+bb0:
+  %0 = struct $S ()
+  release_value %0 : $S
+  %5 = tuple ()
+  return %5 : $()
+}
+
+// Test that a release_value is not replaced for a enum-with-deinit.
+// Doing so would forget the deinit.
+//
+// CHECK-LABEL: sil hidden [noinline] @testEnumDeinit : $@convention(thin) () -> () {
+// CHECK:   release_value %{{.*}} : $MaybeFileDescriptor
+// CHECK-LABEL: } // end sil function 'testEnumDeinit'
+sil hidden [noinline] @testEnumDeinit : $@convention(thin) () -> () {
+bb0:
+  %0 = integer_literal $Builtin.Int64, 0
+  %26 = struct $FileDescriptor (%0 : $Builtin.Int64)
+  %38 = enum $MaybeFileDescriptor, #MaybeFileDescriptor.some!enumelt, %26 : $FileDescriptor
+  release_value %38 : $MaybeFileDescriptor
+  %42 = tuple ()
+  return %42 : $()
+}