Merge pull request #63276 from nate-chandler/rdar104635319

nate-chandler · web-flow · commit 107d8ffe484d · 2023-01-28T15:08:04.000-08:00
[CanonicalOSSALifetime] Respect access scope on flag.
diff --git a/include/swift/SILOptimizer/Utils/CanonicalizeOSSALifetime.h b/include/swift/SILOptimizer/Utils/CanonicalizeOSSALifetime.h
@@ -222,6 +222,11 @@ class CanonicalizeOSSALifetime final {
   /// lifetime boundary in case we need to emit diagnostics.
   std::function<void(Operand *)> moveOnlyFinalConsumingUse;
 
+  // If present, will be used to ensure that the lifetime is not shortened to
+  // end inside an access scope which it previously enclosed.  (Note that ending
+  // before such an access scope is fine regardless.)
+  //
+  // For details, see extendLivenessThroughOverlappingAccess.
   NonLocalAccessBlockAnalysis *accessBlockAnalysis;
   // Lazily initialize accessBlocks only when
   // extendLivenessThroughOverlappingAccess is invoked.
diff --git a/lib/SILOptimizer/Mandatory/MoveOnlyAddressChecker.cpp b/lib/SILOptimizer/Mandatory/MoveOnlyAddressChecker.cpp
@@ -859,7 +859,6 @@ struct MoveOnlyChecker {
   ConsumeInfo consumes;
 
   MoveOnlyChecker(SILFunction *fn, DeadEndBlocks *deBlocks,
-                  NonLocalAccessBlockAnalysis *accessBlockAnalysis,
                   DominanceInfo *domTree)
       : fn(fn), deleter(), canonicalizer(), diagnosticEmitter() {
     deleter.setCallbacks(std::move(
@@ -868,7 +867,7 @@ struct MoveOnlyChecker {
             moveIntroducersToProcess.remove(mvi);
           instToDelete->eraseFromParent();
         })));
-    canonicalizer.init(fn, accessBlockAnalysis, domTree, deleter);
+    canonicalizer.init(fn, domTree, deleter);
     diagnosticEmitter.init(fn, &canonicalizer);
   }
 
@@ -1966,13 +1965,11 @@ class MoveOnlyCheckerPass : public SILFunctionTransform {
            "Should only run on Raw SIL");
     LLVM_DEBUG(llvm::dbgs() << "===> MoveOnly Addr Checker. Visiting: "
                             << fn->getName() << '\n');
-    auto *accessBlockAnalysis = getAnalysis<NonLocalAccessBlockAnalysis>();
     auto *dominanceAnalysis = getAnalysis<DominanceAnalysis>();
     DominanceInfo *domTree = dominanceAnalysis->get(fn);
     auto *deAnalysis = getAnalysis<DeadEndBlocksAnalysis>()->get(fn);
 
-    if (MoveOnlyChecker(getFunction(), deAnalysis, accessBlockAnalysis, domTree)
-            .checkFunction()) {
+    if (MoveOnlyChecker(getFunction(), deAnalysis, domTree).checkFunction()) {
       invalidateAnalysis(SILAnalysis::InvalidationKind::Instructions);
     }
   }
diff --git a/lib/SILOptimizer/Mandatory/MoveOnlyObjectChecker.cpp b/lib/SILOptimizer/Mandatory/MoveOnlyObjectChecker.cpp
@@ -314,8 +314,7 @@ struct MoveOnlyChecker {
 
   MoveOnlyChecker(SILFunction *fn, DeadEndBlocks *deBlocks) : fn(fn) {}
 
-  void check(NonLocalAccessBlockAnalysis *accessBlockAnalysis,
-             DominanceInfo *domTree);
+  void check(DominanceInfo *domTree);
 
   /// After we have emitted a diagnostic, we need to clean up the instruction
   /// stream by converting /all/ copies of move only typed things to use
@@ -336,8 +335,7 @@ struct MoveOnlyChecker {
 //                             MARK: Main Routine
 //===----------------------------------------------------------------------===//
 
-void MoveOnlyChecker::check(NonLocalAccessBlockAnalysis *accessBlockAnalysis,
-                            DominanceInfo *domTree) {
+void MoveOnlyChecker::check(DominanceInfo *domTree) {
   auto callbacks =
       InstModCallbacks().onDelete([&](SILInstruction *instToDelete) {
         if (auto *mvi = dyn_cast<MarkMustCheckInst>(instToDelete))
@@ -346,7 +344,7 @@ void MoveOnlyChecker::check(NonLocalAccessBlockAnalysis *accessBlockAnalysis,
       });
   InstructionDeleter deleter(std::move(callbacks));
   OSSACanonicalizer canonicalizer;
-  canonicalizer.init(fn, accessBlockAnalysis, domTree, deleter);
+  canonicalizer.init(fn, domTree, deleter);
   DiagnosticEmitter diagnosticEmitter;
   diagnosticEmitter.init(fn, &canonicalizer);
 
@@ -508,13 +506,12 @@ class MoveOnlyCheckerPass : public SILFunctionTransform {
     LLVM_DEBUG(llvm::dbgs() << "===> MoveOnly Object Checker. Visiting: "
                             << fn->getName() << '\n');
 
-    auto *accessBlockAnalysis = getAnalysis<NonLocalAccessBlockAnalysis>();
     auto *dominanceAnalysis = getAnalysis<DominanceAnalysis>();
     DominanceInfo *domTree = dominanceAnalysis->get(fn);
     auto *deAnalysis = getAnalysis<DeadEndBlocksAnalysis>()->get(fn);
 
     MoveOnlyChecker checker(getFunction(), deAnalysis);
-    checker.check(accessBlockAnalysis, domTree);
+    checker.check(domTree);
     if (checker.changed) {
       invalidateAnalysis(SILAnalysis::InvalidationKind::Instructions);
     }
diff --git a/lib/SILOptimizer/Mandatory/MoveOnlyObjectChecker.h b/lib/SILOptimizer/Mandatory/MoveOnlyObjectChecker.h
@@ -53,17 +53,17 @@ struct OSSACanonicalizer {
 
   OSSACanonicalizer() {}
 
-  void init(SILFunction *fn, NonLocalAccessBlockAnalysis *accessBlockAnalysis,
-            DominanceInfo *domTree, InstructionDeleter &deleter) {
+  void init(SILFunction *fn, DominanceInfo *domTree,
+            InstructionDeleter &deleter) {
     auto foundConsumingUseNeedingCopy = std::function<void(Operand *)>(
         [&](Operand *use) { consumingUsesNeedingCopy.push_back(use); });
     auto foundConsumingUseNotNeedingCopy = std::function<void(Operand *)>(
         [&](Operand *use) { finalConsumingUses.push_back(use); });
 
     canonicalizer.emplace(
         false /*pruneDebugMode*/, !fn->shouldOptimize() /*maximizeLifetime*/,
-        accessBlockAnalysis, domTree, deleter, foundConsumingUseNeedingCopy,
-        foundConsumingUseNotNeedingCopy);
+        nullptr /*accessBlockAnalysis*/, domTree, deleter,
+        foundConsumingUseNeedingCopy, foundConsumingUseNotNeedingCopy);
   }
 
   void clear() {
diff --git a/lib/SILOptimizer/UtilityPasses/UnitTestRunner.cpp b/lib/SILOptimizer/UtilityPasses/UnitTestRunner.cpp
@@ -373,6 +373,9 @@ struct MultiDefLivenessTest : UnitTest {
 // Arguments:
 // - bool: pruneDebug
 // - bool: maximizeLifetimes
+// - bool: "respectAccessScopes", whether to contract lifetimes to end within
+//         access scopes which they previously enclosed but can't be hoisted
+//         before
 // - SILValue: value to canonicalize
 // Dumps:
 // - function after value canonicalization
@@ -384,9 +387,11 @@ struct CanonicalizeOSSALifetimeTest : UnitTest {
     DominanceInfo *domTree = dominanceAnalysis->get(getFunction());
     auto pruneDebug = arguments.takeBool();
     auto maximizeLifetimes = arguments.takeBool();
+    auto respectAccessScopes = arguments.takeBool();
     InstructionDeleter deleter;
-    CanonicalizeOSSALifetime canonicalizer(pruneDebug, maximizeLifetimes, accessBlockAnalysis,
-                                           domTree, deleter);
+    CanonicalizeOSSALifetime canonicalizer(
+        pruneDebug, maximizeLifetimes,
+        respectAccessScopes ? accessBlockAnalysis : nullptr, domTree, deleter);
     auto value = arguments.takeValue();
     canonicalizer.canonicalizeValueLifetime(value);
     getFunction()->dump();
diff --git a/lib/SILOptimizer/Utils/CanonicalizeOSSALifetime.cpp b/lib/SILOptimizer/Utils/CanonicalizeOSSALifetime.cpp
@@ -1028,7 +1028,9 @@ bool CanonicalizeOSSALifetime::canonicalizeValueLifetime(SILValue def) {
     clearLiveness();
     return false;
   }
-  extendLivenessThroughOverlappingAccess();
+  if (accessBlockAnalysis) {
+    extendLivenessThroughOverlappingAccess();
+  }
   // Step 2: compute original boundary
   PrunedLivenessBoundary originalBoundary;
   findOriginalBoundary(originalBoundary);
diff --git a/test/SILOptimizer/canonicalize_ossa_lifetime_unit.sil b/test/SILOptimizer/canonicalize_ossa_lifetime_unit.sil
@@ -0,0 +1,47 @@
+// RUN: %target-sil-opt -unit-test-runner %s -o /dev/null 2>&1 | %FileCheck %s
+
+class C {}
+
+// When access scopes are respected, the lifetime which previously extended
+// beyond the access scope still extends beyond it.
+// CHECK-LABEL: begin running test 1 of 2 on retract_value_lifetime_into_access_scope_when_access_scopes_not_respected: canonicalize-ossa-lifetime with: true, false, true, @trace
+// CHECK-LABEL: sil [ossa] @retract_value_lifetime_into_access_scope_when_access_scopes_not_respected {{.*}} {
+// CHECK:       {{bb[0-9]+}}([[ADDR:%[^,]+]] :
+// CHECK:         [[INSTANCE:%[^,]+]] = apply
+// CHECK:         [[COPY:%[^,]+]] = copy_value [[INSTANCE]]
+// CHECK:         [[ACCESS:%[^,]+]] = begin_access [modify] [static] [[ADDR]]
+// CHECK:         store [[COPY]] to [init] [[ACCESS]]
+// CHECK:         end_access [[ACCESS]]
+// CHECK:         destroy_value [[INSTANCE]]
+// CHECK-LABEL: } // end sil function 'retract_value_lifetime_into_access_scope_when_access_scopes_not_respected'
+// CHECK-LABEL: end running test 1 of 2 on retract_value_lifetime_into_access_scope_when_access_scopes_not_respected: canonicalize-ossa-lifetime with: true, false, true, @trace
+
+// When access scopes are not respected, the lifetime which previously extended
+// beyond the access scope is retracted into the scope.
+// CHECK-LABEL: begin running test 2 of 2 on retract_value_lifetime_into_access_scope_when_access_scopes_not_respected: canonicalize-ossa-lifetime with: true, false, false, @trace
+// CHECK-LABEL: sil [ossa] @retract_value_lifetime_into_access_scope_when_access_scopes_not_respected {{.*}} {
+// CHECK:       {{bb[0-9]+}}([[ADDR:%[^,]+]] :
+// CHECK:         [[INSTANCE:%[^,]+]] = apply
+// CHECK:         [[ACCESS:%[^,]+]] = begin_access [modify] [static] [[ADDR]]
+// CHECK:         store [[INSTANCE]] to [init] [[ACCESS]]
+// CHECK:         end_access [[ACCESS]]
+// CHECK-LABEL: } // end sil function 'retract_value_lifetime_into_access_scope_when_access_scopes_not_respected'
+// CHECK-LABEL: end running test 2 of 2 on retract_value_lifetime_into_access_scope_when_access_scopes_not_respected: canonicalize-ossa-lifetime with: true, false, false, @trace
+sil [ossa] @retract_value_lifetime_into_access_scope_when_access_scopes_not_respected : $@convention(thin) () -> @out C {
+bb0(%addr : $*C):
+  %instance = apply undef() : $@convention(thin) () -> @owned C
+  debug_value [trace] %instance : $C
+                                                         // respect access scopes
+                                                         // VVVV
+  test_specification "canonicalize-ossa-lifetime true false true @trace"
+  test_specification "canonicalize-ossa-lifetime true false false @trace"
+                                                         // ^^^^^
+                                                         // respect access scopes
+  %copy = copy_value %instance : $C
+  %access = begin_access [modify] [static] %addr : $*C
+  store %copy to [init] %access : $*C
+  end_access %access : $*C
+  destroy_value %instance : $C
+  %retval = tuple ()
+  return %retval : $()
+}
diff --git a/test/SILOptimizer/unit_test.sil b/test/SILOptimizer/unit_test.sil
@@ -156,11 +156,11 @@ sil [ossa] @getC : $@convention(thin) () -> @owned C
 sil [ossa] @borrowC : $@convention(thin) (@guaranteed C) -> ()
 sil [ossa] @takeC : $@convention(thin) (@owned C) -> ()
 
-// CHECK-LABEL: begin running test 1 of 1 on fn: canonicalize-ossa-lifetime with: true, true, @trace
-// CHECK-LABEL: end running test 1 of 1 on fn: canonicalize-ossa-lifetime with: true, true, @trace
+// CHECK-LABEL: begin running test 1 of 1 on fn: canonicalize-ossa-lifetime with: true, true, true, @trace
+// CHECK-LABEL: end running test 1 of 1 on fn: canonicalize-ossa-lifetime with: true, true, true, @trace
 sil [ossa] @fn : $@convention(thin) () -> () {
 entry:
-    test_specification "canonicalize-ossa-lifetime true true @trace"
+    test_specification "canonicalize-ossa-lifetime true true true @trace"
     %getC = function_ref @getC : $@convention(thin) () -> @owned C
     %c = apply %getC() : $@convention(thin) () -> @owned C
     debug_value [trace] %c : $C
