Merge pull request #67205 from jckarter/allocbox-to-stack-move-only-closure-remnants

jckarter · web-flow · commit 631bcabd9e31 · 2023-07-11T08:42:12.000-07:00
Arrange for closure bodies promoted by AllocBoxToStack to have their originals removed by MoveOnlyChecker.
diff --git a/include/swift/AST/SemanticAttrs.def b/include/swift/AST/SemanticAttrs.def
@@ -128,22 +128,24 @@ SEMANTICS_ATTR(LIFETIMEMANAGEMENT_COPY, "lifetimemanagement.copy")
 SEMANTICS_ATTR(NO_PERFORMANCE_ANALYSIS, "no_performance_analysis")
 
 // A flag used to turn off moveonly diagnostics on a function due to an earlier
-// pass that ran.
-//
-// DISCUSSION: This is used in a few situations:
-//
-// 1. When allocbox to stack specializes a function, we do not want to emit move
-// errors twice, once on the specialized and once on the original
-// function. Instead, we put this on the original function.
-//
-// 2. If we emit a diagnose invalid escaping captures error due to an inout
-// being escaped into an escaping closure, we do not want to emit move errors in
-// the closure. This is because SILGen today assumes that we will error in such
-// cases and thus does not emit markers in said function for the inout. This
-// then causes us to emit spurious "found a copy of a noncopyable value" errors
-// that may cause the user to think there is a bug in the compiler.
+// pass that ran. If we emit a diagnose invalid escaping captures error due to
+// an inout being escaped into an escaping closure, we do not want to emit move
+// errors in the closure. This is because SILGen today assumes that we will error
+// in such cases and thus does not emit markers in said function for the inout.
+// This then causes us to emit spurious "found a copy of a noncopyable value"
+// errors that may cause the user to think there is a bug in the compiler.
 SEMANTICS_ATTR(NO_MOVEONLY_DIAGNOSTICS, "sil.optimizer.moveonly.diagnostic.ignore")
 
+// Tell the move-only checker to delete this function body instead of performing
+// diagnostics if the function is not used at the time of checking.
+// When allocbox to stack specializes a function, we do not want to emit move
+// errors twice, once on the specialized and once on the original
+// function. The semantics of the closure with regards to move-only values may
+// also be dependent on stack promotion. Therefore, we mark the original with
+// this attribute, so that after it's promoted, the original function gets deleted
+// instead of raising spurious diagnostics.
+SEMANTICS_ATTR(MOVEONLY_DELETE_IF_UNUSED, "sil.optimizer.moveonly.delete_if_unused")
+
 // Force the use of the frame pointer for the specified function
 SEMANTICS_ATTR(USE_FRAME_POINTER, "use_frame_pointer")
 
diff --git a/lib/SILOptimizer/Mandatory/MoveOnlyChecker.cpp b/lib/SILOptimizer/Mandatory/MoveOnlyChecker.cpp
@@ -157,9 +157,58 @@ class MoveOnlyCheckerPass : public SILFunctionTransform {
     assert(fn->getModule().getStage() == SILStage::Raw &&
            "Should only run on Raw SIL");
 
+    // If an earlier pass asked us to eliminate the function body if it's
+    // unused, and the function is in fact unused, do that now.
+    if (fn->hasSemanticsAttr(semantics::MOVEONLY_DELETE_IF_UNUSED)) {
+      if (fn->getRefCount() == 0
+          && !isPossiblyUsedExternally(fn->getLinkage(),
+                                       fn->getModule().isWholeModule())) {
+        LLVM_DEBUG(llvm::dbgs() << "===> Deleting unused function " << fn->getName() << "'s body that was marked for deletion\n");
+        // Remove all non-entry blocks.
+        auto entryBB = fn->begin();
+        auto nextBB = std::next(entryBB);
+        
+        while (nextBB != fn->end()) {
+          auto thisBB = nextBB;
+          ++nextBB;
+          thisBB->eraseFromParent();
+        }
+        
+        // Rewrite the entry block to only contain an unreachable.
+        auto loc = entryBB->begin()->getLoc();
+        entryBB->eraseAllInstructions(fn->getModule());
+        {
+          SILBuilder b(&*entryBB);
+          b.createUnreachable(loc);
+        }
+        
+        // If the function has shared linkage, reduce this version to private
+        // linkage, because we don't want the deleted-body form to win in any
+        // ODR shootouts.
+        if (fn->getLinkage() == SILLinkage::Shared) {
+          fn->setLinkage(SILLinkage::Private);
+        }
+        
+        invalidateAnalysis(SILAnalysis::InvalidationKind::FunctionBody);
+        return;
+      }
+      // If the function wasn't unused, let it continue into diagnostics.
+      // This would come up if a closure function somehow was used in different
+      // functions with different escape analysis results. This shouldn't really
+      // be possible, and we should try harder to make it impossible, but if
+      // it does happen now, the least bad thing to do is to proceed with
+      // move checking. This will either succeed and make sure the original
+      // function contains valid SIL, or raise errors relating to the use
+      // of the captures in an escaping way, which is the right thing to do
+      // if they are in fact escapable.
+      LLVM_DEBUG(llvm::dbgs() << "===> Function " << fn->getName()
+                              << " was marked to be deleted, but has uses. Continuing with move checking\n");
+      
+    }
+
     // If an earlier pass told use to not emit diagnostics for this function,
     // clean up any copies, invalidate the analysis, and return early.
-    if (getFunction()->hasSemanticsAttr(semantics::NO_MOVEONLY_DIAGNOSTICS)) {
+    if (fn->hasSemanticsAttr(semantics::NO_MOVEONLY_DIAGNOSTICS)) {
       if (cleanupNonCopyableCopiesAfterEmittingDiagnostic(getFunction()))
         invalidateAnalysis(SILAnalysis::InvalidationKind::Instructions);
       return;
@@ -169,7 +218,7 @@ class MoveOnlyCheckerPass : public SILFunctionTransform {
                << "===> MoveOnly Checker. Visiting: " << fn->getName() << '\n');
 
     MoveOnlyChecker checker(
-        getFunction(), getAnalysis<DominanceAnalysis>()->get(getFunction()),
+        fn, getAnalysis<DominanceAnalysis>()->get(fn),
         getAnalysis<PostOrderAnalysis>());
 
     checker.checkObjects();
@@ -180,12 +229,12 @@ class MoveOnlyCheckerPass : public SILFunctionTransform {
     // non-copyable copies into explicit variants below and let the user
     // recompile.
     if (!checker.diagnosticEmitter.emittedDiagnostic()) {
-      emitCheckerMissedCopyOfNonCopyableTypeErrors(getFunction(),
+      emitCheckerMissedCopyOfNonCopyableTypeErrors(fn,
                                                    checker.diagnosticEmitter);
     }
 
     checker.madeChange |=
-        cleanupNonCopyableCopiesAfterEmittingDiagnostic(getFunction());
+        cleanupNonCopyableCopiesAfterEmittingDiagnostic(fn);
 
     if (checker.madeChange)
       invalidateAnalysis(SILAnalysis::InvalidationKind::Instructions);
diff --git a/lib/SILOptimizer/Transforms/AllocBoxToStack.cpp b/lib/SILOptimizer/Transforms/AllocBoxToStack.cpp
@@ -1047,9 +1047,9 @@ specializeApplySite(SILOptFunctionBuilder &FuncBuilder, ApplySite Apply,
     ClonedFn = Cloner.getCloned();
     pass.T->addFunctionToPassManagerWorklist(ClonedFn, F);
 
-    // Set the moveonly ignore flag so we do not emit an error on the original
-    // function even though it is still around.
-    F->addSemanticsAttr(semantics::NO_MOVEONLY_DIAGNOSTICS);
+    // Set the moveonly delete-if-unused flag so we do not emit an error on the
+    // original once we promote all its current uses.
+    F->addSemanticsAttr(semantics::MOVEONLY_DELETE_IF_UNUSED);
 
     // If any of our promoted callee arg indices were originally noncopyable let
     // boxes, convert them from having escaping to having non-escaping
@@ -1213,51 +1213,9 @@ static void rewriteApplySites(AllocBoxToStackState &pass) {
     auto *FRI = cast<FunctionRefInst>(Apply.getCallee());
     Apply.getInstruction()->eraseFromParent();
 
-    if (FRI->use_empty()) {
-      auto referencedFn = FRI->getReferencedFunction();
+    // TODO: Erase from module if there are no more uses.
+    if (FRI->use_empty())
       FRI->eraseFromParent();
-
-      // TODO: Erase from module if there are no more uses.
-      // If the function has no remaining references, it should eventually
-      // be deleted. We can't do that from a function pass, since the function
-      // is still queued up for other passes to run after this one, but we
-      // can at least gut the implementation, since subsequent passes that
-      // rely on stack promotion to occur (particularly closure lifetime
-      // fixup and move-only checking) may not be able to proceed in a
-      // sensible way on the now non-canonical original implementation.
-      if (referencedFn->getRefCount() == 0
-          && !isPossiblyUsedExternally(referencedFn->getLinkage(),
-                                   referencedFn->getModule().isWholeModule())) {
-        LLVM_DEBUG(llvm::dbgs() << "*** Deleting original function " << referencedFn->getName() << "'s body since it is unused");
-        // Remove all non-entry blocks.
-        auto entryBB = referencedFn->begin();
-        auto nextBB = std::next(entryBB);
-        
-        while (nextBB != referencedFn->end()) {
-          auto thisBB = nextBB;
-          ++nextBB;
-          thisBB->eraseFromParent();
-        }
-        
-        // Rewrite the entry block to only contain an unreachable.
-        auto loc = entryBB->begin()->getLoc();
-        entryBB->eraseAllInstructions(referencedFn->getModule());
-        {
-          SILBuilder b(&*entryBB);
-          b.createUnreachable(loc);
-        }
-        
-        // Refresh the CFG in case we removed any function calls.
-        pass.CFGChanged = true;
-        
-        // If the function has shared linkage, reduce this version to private
-        // linkage, because we don't want the deleted-body form to win in any
-        // ODR shootouts.
-        if (referencedFn->getLinkage() == SILLinkage::Shared) {
-          referencedFn->setLinkage(SILLinkage::Private);
-        }
-      }
-    }
   }
 }
 
diff --git a/test/SILOptimizer/allocbox_to_stack.sil b/test/SILOptimizer/allocbox_to_stack.sil
@@ -398,7 +398,7 @@ bb0(%0 : $Int):
 }
 
 // CHECK-LABEL: sil private @$s6struct8useStack1tySi_tFSiycfU_Tf0s_n
-// CHECK-LABEL: sil private [_semantics "sil.optimizer.moveonly.diagnostic.ignore"] @$s6struct8useStack1tySi_tFSiycfU_
+// CHECK-LABEL: sil private [_semantics "sil.optimizer.moveonly.delete_if_unused"] @$s6struct8useStack1tySi_tFSiycfU_
 // struct.(useStack (t : Swift.Int) -> ()).(closure #1)
 sil private @$s6struct8useStack1tySi_tFSiycfU_ : $@convention(thin) (@owned <τ_0_0> { var τ_0_0 } <Int>) -> Int {
 bb0(%0 : $<τ_0_0> { var τ_0_0 } <Int>):
@@ -651,13 +651,9 @@ bb0(%0 : $Int, %1 : $*S<T>):
   return %9 : $Bool
 }
 
-// This closure body gets specialized with unboxed capture parameters, so
-// the original function body is stubbed out once the call sites are all
-// specialized.
-// CHECK-LABEL: sil private [_semantics "sil.optimizer.moveonly.diagnostic.ignore"] @closure1
-// CHECK: bb0
-// CHECK-NEXT: unreachable
+// CHECK-LABEL: sil shared [_semantics "sil.optimizer.moveonly.delete_if_unused"] @closure1
 sil shared @closure1 : $@convention(thin) <T where T : Count> (Int, @owned <τ_0_0 : Count> { var S<τ_0_0> } <T>) -> Bool {
+// CHECK: bb0
 bb0(%0 : $Int, %1 : $<τ_0_0 where τ_0_0 : Count> { var S<τ_0_0> } <T>):
   %2 = project_box %1 : $<τ_0_0 where τ_0_0 : Count> { var S<τ_0_0> } <T>, 0
   %3 = function_ref @inner : $@convention(thin) (@owned @callee_owned () -> Bool) -> Bool
@@ -668,6 +664,7 @@ bb0(%0 : $Int, %1 : $<τ_0_0 where τ_0_0 : Count> { var S<τ_0_0> } <T>):
   %7 = partial_apply %4<T>(%0, %5) : $@convention(thin) <τ_0_0 where τ_0_0 : Count> (Int, @owned <τ_0_0 : Count> { var S<τ_0_0> } <τ_0_0>) -> Bool
   %8 = apply %3(%7) : $@convention(thin) (@owned @callee_owned () -> Bool) -> Bool
   strong_release %1 : $<τ_0_0 where τ_0_0 : Count> { var S<τ_0_0> } <T>
+// CHECK: return
   return %8 : $Bool
 }
 
@@ -676,7 +673,7 @@ bb0(%0 : $Int, %1 : $<τ_0_0 where τ_0_0 : Count> { var S<τ_0_0> } <T>):
 // CHECK: return
 // CHECK-NOT: bb1
 
-// CHECK-LABEL: sil shared [_semantics "sil.optimizer.moveonly.diagnostic.ignore"] @closure2
+// CHECK-LABEL: sil shared [_semantics "sil.optimizer.moveonly.delete_if_unused"] @closure2
 sil shared @closure2 : $@convention(thin) <T where T : Count> (Int, @owned <τ_0_0 : Count> { var S<τ_0_0> } <T>) -> Bool {
 // CHECK: bb0
 bb0(%0 : $Int, %1 : $<τ_0_0 where τ_0_0 : Count> { var S<τ_0_0> } <T>):
diff --git a/test/SILOptimizer/allocbox_to_stack_ownership.sil b/test/SILOptimizer/allocbox_to_stack_ownership.sil
@@ -394,7 +394,7 @@ bb0(%0 : $Int):
 }
 
 // CHECK-LABEL: sil private [transparent] [ossa] @$s6struct8useStack1tySi_tFSiycfU_Tf0s_n
-// CHECK-LABEL: sil private [transparent] [_semantics "sil.optimizer.moveonly.diagnostic.ignore"] [ossa] @$s6struct8useStack1tySi_tFSiycfU_
+// CHECK-LABEL: sil private [transparent] [_semantics "sil.optimizer.moveonly.delete_if_unused"] [ossa] @$s6struct8useStack1tySi_tFSiycfU_
 // struct.(useStack (t : Swift.Int) -> ()).(closure #1)
 sil private [transparent] [ossa] @$s6struct8useStack1tySi_tFSiycfU_ : $@convention(thin) (@owned <τ_0_0> { var τ_0_0 } <Int>) -> Int {
 bb0(%0 : @owned $<τ_0_0> { var τ_0_0 } <Int>):
@@ -751,13 +751,9 @@ bb0(%0 : $Int, %1 : $*S<T>):
   return %9 : $Bool
 }
 
-// This closure body gets specialized with unboxed capture parameters, so
-// the original function body is stubbed out once the call sites are all
-// specialized.
-// CHECK-LABEL: sil private [_semantics "sil.optimizer.moveonly.diagnostic.ignore"] [ossa] @closure1
-// CHECK: bb0
-// CHECK: unreachable
+// CHECK-LABEL: sil shared [_semantics "sil.optimizer.moveonly.delete_if_unused"] [ossa] @closure1
 sil shared [ossa] @closure1 : $@convention(thin) <T where T : Count> (Int, @owned <τ_0_0 : Count> { var S<τ_0_0> } <T>) -> Bool {
+// CHECK: bb0
 bb0(%0 : $Int, %1 : @owned $<τ_0_0 where τ_0_0 : Count> { var S<τ_0_0> } <T>):
   %2 = project_box %1 : $<τ_0_0 where τ_0_0 : Count> { var S<τ_0_0> } <T>, 0
   %3 = function_ref @inner : $@convention(thin) (@owned @callee_owned () -> Bool) -> Bool
@@ -768,6 +764,7 @@ bb0(%0 : $Int, %1 : @owned $<τ_0_0 where τ_0_0 : Count> { var S<τ_0_0> } <T>)
   %7 = partial_apply %4<T>(%0, %5) : $@convention(thin) <τ_0_0 where τ_0_0 : Count> (Int, @owned <τ_0_0 : Count> { var S<τ_0_0> } <τ_0_0>) -> Bool
   %8 = apply %3(%7) : $@convention(thin) (@owned @callee_owned () -> Bool) -> Bool
   destroy_value %1 : $<τ_0_0 where τ_0_0 : Count> { var S<τ_0_0> } <T>
+// CHECK: return
   return %8 : $Bool
 }
 
@@ -776,7 +773,7 @@ bb0(%0 : $Int, %1 : @owned $<τ_0_0 where τ_0_0 : Count> { var S<τ_0_0> } <T>)
 // CHECK: return
 // CHECK-NOT: bb1
 
-// CHECK-LABEL: sil shared [_semantics "sil.optimizer.moveonly.diagnostic.ignore"] [ossa] @closure2
+// CHECK-LABEL: sil shared [_semantics "sil.optimizer.moveonly.delete_if_unused"] [ossa] @closure2
 sil shared [ossa] @closure2 : $@convention(thin) <T where T : Count> (Int, @owned <τ_0_0 : Count> { var S<τ_0_0> } <T>) -> Bool {
 // CHECK: bb0
 bb0(%0 : $Int, %1 : @owned $<τ_0_0 where τ_0_0 : Count> { var S<τ_0_0> } <T>):
diff --git a/test/SILOptimizer/moveonly_delete_if_unused.sil b/test/SILOptimizer/moveonly_delete_if_unused.sil
@@ -0,0 +1,37 @@
+// RUN: %target-sil-opt -module-name main -sil-move-only-checker -enable-sil-verify-all %s | %FileCheck %s
+
+// No uses, shared linkage. Can delete and make private
+// CHECK-LABEL: sil private{{.*}} @unused_shared
+// CHECK:         unreachable
+sil shared [_semantics "sil.optimizer.moveonly.delete_if_unused"] [ossa] @unused_shared : $@convention(thin) () -> () {
+entry:
+  return undef : $()
+}
+
+// No uses, private linkage. Can delete
+// CHECK-LABEL: sil private{{.*}} @unused_private
+// CHECK:         unreachable
+sil private [_semantics "sil.optimizer.moveonly.delete_if_unused"] [ossa] @unused_private : $@convention(thin) () -> () {
+entry:
+  return undef : $()
+}
+
+// Public linkage. Leave as is
+// CHECK-LABEL: sil {{.*}} @unused_public
+// CHECK:         [[F:%.*]] = function_ref @still_used
+// CHECK:         apply [[F]]()
+// CHECK:         return undef : $()
+sil public [_semantics "sil.optimizer.moveonly.delete_if_unused"] [ossa] @unused_public : $@convention(thin) () -> () {
+entry:
+  %f = function_ref @still_used : $@convention(thin) () -> ()
+  apply %f() : $@convention(thin) () -> ()
+  return undef : $()
+}
+
+// Private linkage but used. Leave as is
+// CHECK-LABEL: sil private{{.*}} @still_used
+// CHECK:         return undef : $()
+sil private [_semantics "sil.optimizer.moveonly.delete_if_unused"] [ossa] @still_used : $@convention(thin) () -> () {
+entry:
+  return undef : $()
+}
diff --git a/test/SILOptimizer/moveonly_promoted_closure.swift b/test/SILOptimizer/moveonly_promoted_closure.swift
@@ -0,0 +1,20 @@
+// RUN: %target-swift-frontend -emit-sil -verify %s
+// RUN: %target-swift-frontend -emit-sil -O -verify %s
+
+// rdar://110675352
+
+struct NonCopyableStruct: ~Copyable {}
+
+var globFn: () -> () = {}
+func forceEscaping(_ esc: @escaping () -> ()) {
+  globFn = esc
+}
+
+func closureDiagnosticsSimple() {
+  var s = NonCopyableStruct()
+  let f = {
+    _ = consume s
+    s = NonCopyableStruct()
+  }
+  f()
+}
