Arrange for closure bodies promoted by AllocBoxToStack to have their originals removed by MoveOnlyChecker.

jckarter · jckarter · commit 3d5285be6f04 · 2023-07-10T15:18:16.000-07:00
This is an improvement of #67031 which avoids deleting the closure function body during AllocBoxToStack, which still breaks pass invariants by modifying functions other than the currently-analyzed function. As a function pass, AllocBoxToStack also doesn't really know with certainty whether the original closure function is unused after stack promotion or not. We still want to eliminate the original when it may contain invalid SIL for move-only values that rely on the escape analysis for correct semantics, so rather than mark the original function to be *ignored* during move-only checking, mark it to be *deleted* by move-only checking if the function is in fact unused at that point. If the marked function is still used, we let it pass through move-only checking normally, which may cause redundant diagnostics but is the right thing to do since code is still potentially using the closure with escaping semantics. We should rearrange things to make this situation impossible in the future. rdar://110675352
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
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,7 +651,7 @@ bb0(%0 : $Int, %1 : $*S<T>):
   return %9 : $Bool
 }
 
-// CHECK-LABEL: sil shared [_semantics "sil.optimizer.moveonly.diagnostic.ignore"] @closure1
+// 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>):
@@ -673,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,7 +751,7 @@ bb0(%0 : $Int, %1 : $*S<T>):
   return %9 : $Bool
 }
 
-// CHECK-LABEL: sil shared [_semantics "sil.optimizer.moveonly.diagnostic.ignore"] [ossa] @closure1
+// 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>):
@@ -773,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()
+}
