Merge pull request #72635 from atrick/fix-lifedep-capture

Fix LifetimeDependenceDiagnostics to ignore closure captures

Author: atrick

SwiftCompilerSources/Sources/Optimizer/FunctionPasses/LifetimeDependenceDiagnostics.swift

@@ -29,9 +29,6 @@ private func log(prefix: Bool = true, _ message: @autoclosure () -> String) {
 let lifetimeDependenceDiagnosticsPass = FunctionPass(
   name: "lifetime-dependence-diagnostics")
 { (function: Function, context: FunctionPassContext) in
-  if !context.options.hasFeature(.NonescapableTypes) {
-    return
-  }
   log(prefix: false, "\n--- Diagnosing lifetime dependence in \(function.name)")
   log("\(function)")
 
@@ -45,7 +42,7 @@ let lifetimeDependenceDiagnosticsPass = FunctionPass(
     }
   }
   for instruction in function.instructions {
-    if let markDep = instruction as? MarkDependenceInst {
+    if let markDep = instruction as? MarkDependenceInst, markDep.isUnresolved {
       if let lifetimeDep = LifetimeDependence(markDep, context) {
         analyze(dependence: lifetimeDep, context)
       }

SwiftCompilerSources/Sources/Optimizer/FunctionPasses/LifetimeDependenceInsertion.swift

@@ -31,9 +31,6 @@ private func log(prefix: Bool = true, _ message: @autoclosure () -> String) {
 let lifetimeDependenceInsertionPass = FunctionPass(
   name: "lifetime-dependence-insertion")
 { (function: Function, context: FunctionPassContext) in
-  if !context.options.hasFeature(.NonescapableTypes) {
-    return
-  }
   log(prefix: false, "\n--- Inserting lifetime dependence markers in \(function.name)")
 
   for instruction in function.instructions {

SwiftCompilerSources/Sources/Optimizer/FunctionPasses/LifetimeDependenceScopeFixup.swift

@@ -31,9 +31,6 @@ private func log(prefix: Bool = true, _ message: @autoclosure () -> String) {
 let lifetimeDependenceScopeFixupPass = FunctionPass(
   name: "lifetime-dependence-scope-fixup")
 { (function: Function, context: FunctionPassContext) in
-  if !context.options.hasFeature(.NonescapableTypes) {
-    return
-  }
   log(prefix: false, "\n--- Scope fixup for lifetime dependence in \(function.name)")
 
   let localReachabilityCache = LocalVariableReachabilityCache()

lib/SILOptimizer/Mandatory/ConsumeOperatorCopyableAddressesChecker.cpp

@@ -1660,12 +1660,16 @@ bool DataflowState::process(
 
         {
           auto diag = diag::sil_movechecking_consuming_use_here;
-          diagnose(astContext, mvi->getLoc().getSourceLoc(), diag);
+          if (auto sourceLoc = mvi->getLoc().getSourceLoc()) {
+            diagnose(astContext, sourceLoc, diag);
+          }
         }
 
         {
           auto diag = diag::sil_movechecking_nonconsuming_use_here;
-          diagnose(astContext, iter->second->getLoc().getSourceLoc(), diag);
+          if (auto sourceLoc = iter->second->getLoc().getSourceLoc()) {
+            diagnose(astContext, sourceLoc, diag);
+          }
         }
 
         emittedSingleDiagnostic = true;
@@ -1690,13 +1694,17 @@ bool DataflowState::process(
 
           {
             auto diag = diag::sil_movechecking_consuming_use_here;
-            diagnose(astContext, mvi->getLoc().getSourceLoc(), diag);
+            if (auto sourceLoc = mvi->getLoc().getSourceLoc()) {
+              diagnose(astContext, sourceLoc, diag);
+            }
           }
 
           {
             auto diag = diag::sil_movechecking_nonconsuming_use_here;
             for (auto *user : iter->second->pairedUseInsts) {
-              diagnose(astContext, user->getLoc().getSourceLoc(), diag);
+              if (auto sourceLoc = user->getLoc().getSourceLoc()) {
+                diagnose(astContext, sourceLoc, diag);
+              }
             }
           }
 
@@ -2115,12 +2123,16 @@ bool ConsumeOperatorCopyableAddressesChecker::performSingleBasicBlockAnalysis(
     }
 
     auto diag = diag::sil_movechecking_consuming_use_here;
-    diagnose(astCtx, mvi->getLoc().getSourceLoc(), diag);
+    if (auto sourceLoc = mvi->getLoc().getSourceLoc()) {
+      diagnose(astCtx, sourceLoc, diag);
+    }
 
     {
       auto diag = diag::sil_movechecking_nonconsuming_use_here;
       for (auto *user : interestingClosureUsers) {
-        diagnose(astCtx, user->getLoc().getSourceLoc(), diag);
+        if (auto sourceLoc = user->getLoc().getSourceLoc()) {
+          diagnose(astCtx, sourceLoc, diag);
+        }
       }
     }
 
@@ -2157,12 +2169,16 @@ bool ConsumeOperatorCopyableAddressesChecker::performSingleBasicBlockAnalysis(
 
     {
       auto diag = diag::sil_movechecking_consuming_use_here;
-      diagnose(astCtx, mvi->getLoc().getSourceLoc(), diag);
+      if (auto sourceLoc = mvi->getLoc().getSourceLoc()) {
+        diagnose(astCtx, sourceLoc, diag);
+      }
     }
 
     {
       auto diag = diag::sil_movechecking_nonconsuming_use_here;
-      diagnose(astCtx, interestingUser->getLoc().getSourceLoc(), diag);
+      if (auto sourceLoc = interestingUser->getLoc().getSourceLoc()) {
+        diagnose(astCtx, sourceLoc, diag);
+      }
     }
 
     // We purposely continue to see if at least in simple cases, we can flag

lib/SILOptimizer/Mandatory/ConsumeOperatorCopyableValuesChecker.cpp

@@ -155,6 +155,8 @@ bool CheckerLivenessInfo::compute() {
             }
           }
         }
+        // FIXME: this ignores all other forms of Borrow ownership, such as
+        // partial_apply [onstack] and mark_dependence [nonescaping].
         break;
       }
       case OperandOwnership::GuaranteedForwarding:
@@ -250,8 +252,10 @@ void ConsumeOperatorCopyableValuesChecker::emitDiagnosticForMove(
   diagnose(astContext, getSourceLocFromValue(borrowedValue),
            diag::sil_movechecking_value_used_after_consume,
            borrowedValueName);
-  diagnose(astContext, mvi->getLoc().getSourceLoc(),
-           diag::sil_movechecking_consuming_use_here);
+  if (auto sourceLoc = mvi->getLoc().getSourceLoc()) {
+    diagnose(astContext, sourceLoc,
+             diag::sil_movechecking_consuming_use_here);
+  }
 
   // Then we do a bit of work to figure out where /all/ of the later uses than
   // mvi are so we can emit notes to the user telling them this is a problem
@@ -281,8 +285,10 @@ void ConsumeOperatorCopyableValuesChecker::emitDiagnosticForMove(
       case PrunedLiveness::NonLifetimeEndingUse:
       case PrunedLiveness::LifetimeEndingUse:
         LLVM_DEBUG(llvm::dbgs() << "Emitting note for in block use: " << inst);
-        diagnose(astContext, inst.getLoc().getSourceLoc(),
-                 diag::sil_movechecking_nonconsuming_use_here);
+        if (auto sourceLoc = inst.getLoc().getSourceLoc()) {
+          diagnose(astContext, sourceLoc,
+                   diag::sil_movechecking_nonconsuming_use_here);
+        }
         break;
       }
     }
@@ -340,8 +346,10 @@ void ConsumeOperatorCopyableValuesChecker::emitDiagnosticForMove(
         case PrunedLiveness::LifetimeEndingUse:
           LLVM_DEBUG(llvm::dbgs()
                      << "(3) Emitting diagnostic for user: " << inst);
-          diagnose(astContext, inst.getLoc().getSourceLoc(),
-                   diag::sil_movechecking_nonconsuming_use_here);
+          if (auto sourceLoc = inst.getLoc().getSourceLoc()) {
+            diagnose(astContext, sourceLoc,
+                     diag::sil_movechecking_nonconsuming_use_here);
+          }
           break;
         }
       }
@@ -366,8 +374,10 @@ void ConsumeOperatorCopyableValuesChecker::emitDiagnosticForMove(
       if (livenessInfo.nonLifetimeEndingUsesInLiveOut.contains(&inst)) {
         LLVM_DEBUG(llvm::dbgs()
                    << "(1) Emitting diagnostic for user: " << inst);
-        diagnose(astContext, inst.getLoc().getSourceLoc(),
-                 diag::sil_movechecking_nonconsuming_use_here);
+        if (auto sourceLoc = inst.getLoc().getSourceLoc()) {
+          diagnose(astContext, sourceLoc,
+                   diag::sil_movechecking_nonconsuming_use_here);
+        }
         continue;
       }
 
@@ -388,8 +398,10 @@ void ConsumeOperatorCopyableValuesChecker::emitDiagnosticForMove(
 
           LLVM_DEBUG(llvm::dbgs()
                      << "(2) Emitting diagnostic for user: " << inst);
-          diagnose(astContext, inst.getLoc().getSourceLoc(),
-                   diag::sil_movechecking_nonconsuming_use_here);
+          if (auto sourceLoc = inst.getLoc().getSourceLoc()) {
+            diagnose(astContext, sourceLoc,
+                     diag::sil_movechecking_nonconsuming_use_here);
+          }
         }
       }
     }

lib/SILOptimizer/Mandatory/MoveOnlyDiagnostics.cpp

@@ -60,7 +60,13 @@ static void diagnose(ASTContext &context, SILInstruction *inst, Diag<T...> diag,
   if (SilentlyEmitDiagnostics)
     return;
 
-  context.Diags.diagnose(loc.getSourceLoc(), diag, std::forward<U>(args)...);
+  auto sourceLoc = loc.getSourceLoc();
+  auto diagKind = context.Diags.declaredDiagnosticKindFor(diag.ID);
+  // Do not emit notes on invalid source locations.
+  if (!sourceLoc && diagKind == DiagnosticKind::Note) {
+    return;
+  }
+  context.Diags.diagnose(sourceLoc, diag, std::forward<U>(args)...);
 }
 
 template <typename... T, typename... U>

lib/SILOptimizer/PassManager/PassPipeline.cpp

@@ -171,13 +171,6 @@ static void addMandatoryDiagnosticOptPipeline(SILPassPipelinePlan &P) {
   // Check noImplicitCopy and move only types for objects and addresses.
   P.addMoveOnlyChecker();
 
-  // Check ~Escapable.
-  if (P.getOptions().EnableLifetimeDependenceDiagnostics) {
-    P.addLifetimeDependenceDiagnostics();
-  }
-  if (EnableDeinitDevirtualizer)
-    P.addDeinitDevirtualizer();
-
   // FIXME: rdar://122701694 (`consuming` keyword causes verification error on
   //        invalid SIL types)
   //
@@ -189,6 +182,19 @@ static void addMandatoryDiagnosticOptPipeline(SILPassPipelinePlan &P) {
   // No uses after consume operator of copyable value.
   P.addConsumeOperatorCopyableValuesChecker();
 
+  // Check ~Escapable.
+  if (P.getOptions().EnableLifetimeDependenceDiagnostics) {
+    P.addLifetimeDependenceDiagnostics();
+  }
+
+  // Devirtualize deinits early if requested.
+  //
+  // FIXME: why is DeinitDevirtualizer in the middle of the mandatory pipeline,
+  // and what passes/compilation modes depend on it? This pass is never executed
+  // or tested without '-Xllvm enable-deinit-devirtualizer'.
+  if (EnableDeinitDevirtualizer)
+    P.addDeinitDevirtualizer();
+
   // As a temporary measure, we also eliminate move only for non-trivial types
   // until we can audit the later part of the pipeline. Eventually, this should
   // occur before IRGen.

test/SILOptimizer/lifetime_dependence_borrow.swift

@@ -3,6 +3,7 @@
 // RUN:   -verify \
 // RUN:   -sil-verify-all \
 // RUN:   -module-name test \
+// RUN:   -enable-experimental-feature NoncopyableGenerics \
 // RUN:   -enable-experimental-feature NonescapableTypes
 
 // REQUIRES: asserts
@@ -15,8 +16,7 @@ struct CN: ~Copyable {
 }
 
 // Some Bufferview-ish thing.
-@_nonescapable
-struct BV {
+struct BV : ~Escapable {
   let p: UnsafeRawPointer
   let i: Int
 
@@ -35,8 +35,7 @@ struct BV {
 }
 
 // Some MutableBufferview-ish thing.
-@_nonescapable
-struct MBV : ~Copyable {
+struct MBV : ~Escapable, ~Copyable {
   let p: UnsafeRawPointer
   let i: Int
 
@@ -53,8 +52,7 @@ struct MBV : ~Copyable {
 }
 
 // Nonescapable wrapper.
-@_nonescapable
-struct NEBV {
+struct NEBV : ~Escapable {
   var bv: BV
 
   init(_ bv: consuming BV) {

test/SILOptimizer/lifetime_dependence_borrow_fail.swift

@@ -3,13 +3,13 @@
 // RUN:   -verify \
 // RUN:   -sil-verify-all \
 // RUN:   -module-name test \
+// RUN:   -enable-experimental-feature NoncopyableGenerics \
 // RUN:   -enable-experimental-feature NonescapableTypes
 
 // REQUIRES: asserts
 // REQUIRES: swift_in_compiler
 
-@_nonescapable
-struct BV {
+struct BV : ~Escapable {
   let p: UnsafeRawPointer
   let i: Int
 
@@ -34,8 +34,7 @@ struct NC : ~Copyable {
   }
 }
 
-@_nonescapable
-struct NE {
+struct NE : ~Escapable {
   let p: UnsafeRawPointer
   let i: Int
 

test/SILOptimizer/lifetime_dependence_closure.swift

@@ -4,6 +4,7 @@
 // RUN:   -sil-verify-all \
 // RUN:   -module-name test \
 // RUN:   -disable-experimental-parser-round-trip \
+// RUN:   -enable-experimental-feature NoncopyableGenerics \
 // RUN:   -enable-experimental-feature NonescapableTypes
 
 // REQUIRES: asserts
@@ -15,8 +16,7 @@ struct NCInt: ~Copyable {
   init(_ value: Int) { self.value = value }
 }
 
-@_nonescapable
-struct NEInt /*: ~Escapable*/ {
+struct NEInt : ~Escapable {
   let value: Int
 
   init(borrowed: borrowing NCInt) -> dependsOn(borrowed) Self {