[4.0] [sil-combine] When deleting a dead partial_apply, insert a destroy for all non-trivial captured values.

lib/SILOptimizer/SILCombiner/SILCombinerMiscVisitors.cpp

@@ -32,6 +32,12 @@
 using namespace swift;
 using namespace swift::PatternMatch;
 
+/// This flag is used to disable alloc stack optimizations to ease testing of
+/// other SILCombine optimizations.
+static llvm::cl::opt<bool>
+    DisableAllocStackOpts("sil-combine-disable-alloc-stack-opts",
+                          llvm::cl::init(false));
+
 SILInstruction*
 SILCombiner::visitAllocExistentialBoxInst(AllocExistentialBoxInst *AEBI) {
 
@@ -334,6 +340,11 @@ struct AllocStackAnalyzer : SILInstructionVisitor<AllocStackAnalyzer> {
 } // end anonymous namespace
 
 SILInstruction *SILCombiner::visitAllocStackInst(AllocStackInst *AS) {
+  // If we are testing SILCombine and we are asked not to eliminate
+  // alloc_stacks, just return.
+  if (DisableAllocStackOpts)
+    return nullptr;
+
   AllocStackAnalyzer Analyzer(AS);
   Analyzer.analyze();
 

lib/SILOptimizer/Utils/Local.cpp

@@ -891,6 +891,13 @@ static bool useDoesNotKeepClosureAlive(const SILInstruction *I) {
   }
 }
 
+// *HEY YOU, YES YOU, PLEASE READ*. Even though a textual partial apply is
+// printed with the convention of the closed over function upon it, all
+// non-inout arguments to a partial_apply are passed at +1. This includes
+// arguments that will eventually be passed as guaranteed or in_guaranteed to
+// the closed over function. This is because the partial apply is building up a
+// boxed aggregate to send off to the closed over function. Of course when you
+// call the function, the proper conventions will be used.
 void swift::releasePartialApplyCapturedArg(SILBuilder &Builder, SILLocation Loc,
                                            SILValue Arg, SILParameterInfo PInfo,
                                            InstModCallbacks Callbacks) {
@@ -901,51 +908,51 @@ void swift::releasePartialApplyCapturedArg(SILBuilder &Builder, SILLocation Loc,
   if (PInfo.isIndirectMutating())
     return;
 
-  if (isa<AllocStackInst>(Arg)) {
-    return;
-  }
-
   // If we have a trivial type, we do not need to put in any extra releases.
   if (Arg->getType().isTrivial(Builder.getModule()))
     return;
 
-  // Otherwise, we need to destroy the argument.
-  if (Arg->getType().isObject()) {
-    // If we have qualified ownership, we should just emit a destroy value.
-    if (Arg->getFunction()->hasQualifiedOwnership()) {
-      Callbacks.CreatedNewInst(Builder.createDestroyValue(Loc, Arg));
-      return;
-    }
-
-    if (Arg->getType().hasReferenceSemantics()) {
-      auto U = Builder.emitStrongRelease(Loc, Arg);
-      if (U.isNull())
-        return;
+  // Otherwise, we need to destroy the argument. If we have an address, just
+  // emit a destroy_addr.
+  if (Arg->getType().isAddress()) {
+    SILInstruction *NewInst = Builder.emitDestroyAddrAndFold(Loc, Arg);
+    Callbacks.CreatedNewInst(NewInst);
+    return;
+  }
 
-      if (auto *SRI = U.dyn_cast<StrongRetainInst *>()) {
-        Callbacks.DeleteInst(SRI);
-        return;
-      }
+  // Otherwise, we have an object. We emit the most optimized form of release
+  // possible for that value.
 
-      Callbacks.CreatedNewInst(U.get<StrongReleaseInst *>());
-      return;
-    }
+  // If we have qualified ownership, we should just emit a destroy value.
+  if (Arg->getFunction()->hasQualifiedOwnership()) {
+    Callbacks.CreatedNewInst(Builder.createDestroyValue(Loc, Arg));
+    return;
+  }
 
-    auto U = Builder.emitReleaseValue(Loc, Arg);
+  if (Arg->getType().hasReferenceSemantics()) {
+    auto U = Builder.emitStrongRelease(Loc, Arg);
     if (U.isNull())
       return;
 
-    if (auto *RVI = U.dyn_cast<RetainValueInst *>()) {
-      Callbacks.DeleteInst(RVI);
+    if (auto *SRI = U.dyn_cast<StrongRetainInst *>()) {
+      Callbacks.DeleteInst(SRI);
       return;
     }
 
-    Callbacks.CreatedNewInst(U.get<ReleaseValueInst *>());
+    Callbacks.CreatedNewInst(U.get<StrongReleaseInst *>());
+    return;
+  }
+
+  auto U = Builder.emitReleaseValue(Loc, Arg);
+  if (U.isNull())
+    return;
+
+  if (auto *RVI = U.dyn_cast<RetainValueInst *>()) {
+    Callbacks.DeleteInst(RVI);
     return;
   }
 
-  SILInstruction *NewInst = Builder.emitDestroyAddrAndFold(Loc, Arg);
-  Callbacks.CreatedNewInst(NewInst);
+  Callbacks.CreatedNewInst(U.get<ReleaseValueInst *>());
 }
 
 /// For each captured argument of PAI, decrement the ref count of the captured

test/SILOptimizer/sil_combine.sil

@@ -37,6 +37,8 @@ sil [global_init] @global_init_fun : $@convention(thin) () -> Builtin.RawPointer
 
 sil @user : $@convention(thin) (@owned Builtin.NativeObject) -> ()
 
+sil @unknown : $@convention(thin) () -> ()
+
 //////////////////////
 // Simple DCE Tests //
 //////////////////////
@@ -1755,7 +1757,6 @@ sil [readonly] @read_only_in : $@convention(thin) (@in SSS) -> ()
 struct MyError : Error {
 }
 
-sil @unknown : $@convention(thin) () -> ()
 sil [readonly] @readonly_throwing : $@convention(thin) (@owned ZZZ) -> (ZZZ, @error Error)
 
 sil [readonly] @readonly_owned : $@convention(thin) (@owned B) -> @owned B
@@ -2266,36 +2267,6 @@ bb0(%0 : $Builtin.Int64, %1 : $Builtin.RawPointer, %2 : $Builtin.RawPointer):
   return %13 : $()
 }
 
-// Make sure that we handle partial_apply captured arguments correctly.
-sil @sil_combine_partial_apply_callee : $@convention(thin) (@in Builtin.NativeObject, @inout Builtin.NativeObject, Builtin.NativeObject, @owned Builtin.NativeObject, @guaranteed Builtin.NativeObject) -> ()
-
-// CHECK-LABEL: sil @sil_combine_partial_apply_caller : $@convention(thin) (@in Builtin.NativeObject, @inout Builtin.NativeObject, Builtin.NativeObject, @owned Builtin.NativeObject, @guaranteed Builtin.NativeObject) -> () {
-// CHECK: bb0([[ARG1:%.*]] : $*Builtin.NativeObject, [[ARG2:%.*]] : $*Builtin.NativeObject, [[ARG3:%.*]] : $Builtin.NativeObject, [[ARG4:%.*]] : $Builtin.NativeObject, [[ARG5:%.*]] : $Builtin.NativeObject):
-// CHECK-NEXT: strong_retain [[ARG3]]
-// CHECK-NEXT: strong_retain [[ARG4]]
-// CHECK-NEXT: strong_retain [[ARG5]]
-// CHECK-NEXT: strong_release [[ARG3]]
-// CHECK-NEXT: strong_release [[ARG4]]
-// CHECK-NEXT: strong_release [[ARG5]]
-// CHECK-NEXT: strong_release [[ARG4]]
-// CHECK-NEXT: destroy_addr [[ARG1]]
-sil @sil_combine_partial_apply_caller : $@convention(thin) (@in Builtin.NativeObject, @inout Builtin.NativeObject, Builtin.NativeObject, @owned Builtin.NativeObject, @guaranteed Builtin.NativeObject) -> () {
-bb0(%0 : $*Builtin.NativeObject, %1 : $*Builtin.NativeObject, %2 : $Builtin.NativeObject, %3 : $Builtin.NativeObject, %4 : $Builtin.NativeObject):
-  %100 = function_ref @sil_combine_partial_apply_callee : $@convention(thin) (@in Builtin.NativeObject, @inout Builtin.NativeObject, Builtin.NativeObject, @owned Builtin.NativeObject, @guaranteed Builtin.NativeObject) -> ()
-  %101 = alloc_stack $Builtin.NativeObject
-  copy_addr %0 to [initialization] %101 : $*Builtin.NativeObject
-  strong_retain %2 : $Builtin.NativeObject
-  strong_retain %3 : $Builtin.NativeObject
-  strong_retain %4 : $Builtin.NativeObject
-  %102 = partial_apply %100(%101, %1, %2, %3, %4) : $@convention(thin) (@in Builtin.NativeObject, @inout Builtin.NativeObject, Builtin.NativeObject, @owned Builtin.NativeObject, @guaranteed Builtin.NativeObject) -> ()
-  strong_release %102 : $@callee_owned () -> ()
-  strong_release %3 : $Builtin.NativeObject
-  destroy_addr %0 : $*Builtin.NativeObject
-  dealloc_stack %101 : $*Builtin.NativeObject
-  %9999 = tuple()
-  return %9999 : $()
-}
-
 // CHECK-LABEL: sil @cmp_zext_peephole
 // CHECK: bb0([[Arg1:%.*]] : $Builtin.Word, [[Arg2:%.*]] : $Builtin.Word):
 // CHECK:   [[ZA1:%.*]] = builtin "zextOrBitCast_Word_Int64"([[Arg1]] : $Builtin.Word) : $Builtin.Int64

test/SILOptimizer/sil_combine_apply.sil

@@ -0,0 +1,98 @@
+// RUN: %target-sil-opt -assume-parsing-unqualified-ownership-sil -enable-sil-verify-all %s -sil-combine -verify-skip-unreachable-must-be-last -sil-combine-disable-alloc-stack-opts | %FileCheck %s
+
+import Builtin
+
+//////////////////
+// Declarations //
+//////////////////
+
+sil @unknown : $@convention(thin) () -> ()
+
+/////////////////////////////////
+// Tests for SILCombinerApply. //
+/////////////////////////////////
+
+// Make sure that we handle partial_apply captured arguments correctly.
+//
+// We use custom types here to make it easier to pattern match with FileCheck.
+struct S1 { var x: Builtin.NativeObject }
+struct S2 { var x: Builtin.NativeObject }
+struct S3 { var x: Builtin.NativeObject }
+struct S4 { var x: Builtin.NativeObject }
+struct S5 { var x: Builtin.NativeObject }
+struct S6 { var x: Builtin.NativeObject }
+struct S7 { var x: Builtin.NativeObject }
+struct S8 { var x: Builtin.NativeObject }
+sil @sil_combine_partial_apply_callee : $@convention(thin) (@in S1, @in S2, @in_guaranteed S3, @in_guaranteed S4, @inout S5, S6, @owned S7, @guaranteed S8) -> ()
+
+// *NOTE PLEASE READ*. If this test case looks funny to you, it is b/c partial
+// apply is funny. Specifically, even though a partial apply has the conventions
+// of the function on it, arguments to the partial apply (that will be passed
+// off to the function) must /always/ be passed in at +1. This is because the
+// partial apply is building up a boxed aggregate to send off to the closed over
+// function. Of course when you call the function, the proper conventions will
+// be used.
+//
+// CHECK-LABEL: sil @sil_combine_dead_partial_apply : $@convention(thin) (@in S2, @in S4, @inout S5, S6, @owned S7, @guaranteed S8) -> () {
+// CHECK: bb0([[IN_ARG:%.*]] : $*S2, [[INGUARANTEED_ARG:%.*]] : $*S4, [[INOUT_ARG:%.*]] : $*S5, [[UNOWNED_ARG:%.*]] : $S6, [[OWNED_ARG:%.*]] : $S7, [[GUARANTEED_ARG:%.*]] : $S8):
+//
+// CHECK: function_ref unknown
+// CHECK: [[UNKNOWN_FUNC:%.*]] = function_ref @unknown
+// CHECK-NEXT: [[IN_ADDRESS:%.*]] = alloc_stack $S1
+// CHECK-NEXT: [[INGUARANTEED_ADDRESS:%.*]] = alloc_stack $S3
+//
+// CHECK-NEXT: apply [[UNKNOWN_FUNC]]()
+//
+// Then make sure that the destroys are placed after the destroy_value of the
+// partial_apply (which is after this apply)...
+// CHECK-NEXT: apply [[UNKNOWN_FUNC]]()
+//
+// CHECK-NEXT: destroy_addr [[IN_ADDRESS]]
+// CHECK-NEXT: destroy_addr [[IN_ARG]]
+// CHECK-NEXT: destroy_addr [[INGUARANTEED_ADDRESS]]
+// CHECK-NEXT: destroy_addr [[INGUARANTEED_ARG]]
+// CHECK-NEXT: release_value [[UNOWNED_ARG]]
+// CHECK-NEXT: release_value [[OWNED_ARG]]
+// CHECK-NEXT: release_value [[GUARANTEED_ARG]]
+//
+// ... but before the function epilog.
+// CHECK-NEXT: apply [[UNKNOWN_FUNC]]()
+// CHECK-NEXT: dealloc_stack [[INGUARANTEED_ADDRESS]]
+// CHECK-NEXT: dealloc_stack [[IN_ADDRESS]]
+// CHECK-NEXT: tuple
+// CHECK-NEXT: return
+// CHECK-NEXT: } // end sil function 'sil_combine_dead_partial_apply'
+sil @sil_combine_dead_partial_apply : $@convention(thin) (@in S2, @in S4, @inout S5, S6, @owned S7, @guaranteed S8) -> () {
+bb0(%1 : $*S2, %2 : $*S4, %4 : $*S5, %5 : $S6, %6 : $S7, %7 : $S8):
+  %8 = function_ref @unknown : $@convention(thin) () -> ()
+  %9 = function_ref @sil_combine_partial_apply_callee : $@convention(thin) (@in S1, @in S2, @in_guaranteed S3, @in_guaranteed S4, @inout S5, S6, @owned S7, @guaranteed S8) -> ()
+
+  // This is for the @in alloc_stack case.
+  %10 = alloc_stack $S1
+  // This is for the @in_guaranteed alloc_stack case.
+  %11 = alloc_stack $S3
+
+  // Marker of space in between the alloc_stack and the partial_apply
+  apply %8() : $@convention(thin) () -> ()
+
+  // Now call the partial apply. We use the "unknown" function call after the
+  // partial apply to ensure that we are truly placing releases at the partial
+  // applies release rather than right afterwards.
+  %102 = partial_apply %9(%10, %1, %11, %2, %4, %5, %6, %7) : $@convention(thin) (@in S1, @in S2, @in_guaranteed S3, @in_guaranteed S4, @inout S5, S6, @owned S7, @guaranteed S8) -> ()
+
+  // Marker of space in between partial_apply and the release of %102.
+  apply %8() : $@convention(thin) () -> ()
+
+  strong_release %102 : $@callee_owned () -> ()
+
+  apply %8() : $@convention(thin) () -> ()
+
+  // Epilog.
+
+  // Cleanup the stack locations.
+  dealloc_stack %11 : $*S3
+  dealloc_stack %10 : $*S1
+
+  %9999 = tuple()
+  return %9999 : $()
+}