Merge pull request #66723 from meg-gupta/improvefindpointerescape

meg-gupta · web-flow · commit 28df449d38b3 · 2023-06-21T21:04:21.000-07:00
Refactor swift::findPointerEscape and handle additional cases
diff --git a/include/swift/SIL/OwnershipUtils.h b/include/swift/SIL/OwnershipUtils.h
@@ -90,13 +90,13 @@ inline bool isForwardingConsume(SILValue value) {
 //                        Ownership Def-Use Utilities
 //===----------------------------------------------------------------------===//
 
-bool hasPointerEscape(BorrowedValue value);
+bool findPointerEscape(BorrowedValue value);
 
 /// Whether the specified OSSA-lifetime introducer has a pointer escape.
 ///
 /// precondition: \p value introduces an OSSA-lifetime, either a BorrowedValue
 /// can be constructed from it or it's an owned value
-bool hasPointerEscape(SILValue value);
+bool findPointerEscape(SILValue value);
 
 /// Find leaf "use points" of \p guaranteedValue that determine its lifetime
 /// requirement. Return true if no PointerEscape use was found.
diff --git a/lib/SIL/Utils/AddressWalker.cpp b/lib/SIL/Utils/AddressWalker.cpp
@@ -25,16 +25,6 @@ AddressUseKind TransitiveAddressWalker::walk(SILValue projectedAddress) && {
   // When we exit, set the result to be invalidated so we can't use this again.
   SWIFT_DEFER { didInvalidate = true; };
 
-  // If the projectedAddress is dead, it is itself a leaf use. Since we don't
-  // have an operand for it, simply bail. Dead projectedAddress is unexpected.
-  //
-  // TODO: store_borrow is currently an InteriorPointer with no uses, so we end
-  // up bailing. It should be in a dependence scope instead. It's not clear why
-  // it produces an address at all.
-  if (projectedAddress->use_empty()) {
-    return AddressUseKind::PointerEscape;
-  }
-
   StackList<Operand *> worklist(projectedAddress->getFunction());
   SmallPtrSet<Operand *, 32> visitedOperands;
 
diff --git a/lib/SIL/Utils/OwnershipUtils.cpp b/lib/SIL/Utils/OwnershipUtils.cpp
@@ -25,33 +25,52 @@
 
 using namespace swift;
 
-bool swift::hasPointerEscape(BorrowedValue original) {
-  ValueWorklist worklist(original->getFunction());
-  worklist.push(*original);
+bool swift::findPointerEscape(SILValue original) {
+  if (original->getOwnershipKind() != OwnershipKind::Owned &&
+      original->getOwnershipKind() != OwnershipKind::Guaranteed) {
+    return false;
+  }
 
-  if (auto *phi = SILArgument::asPhi(*original)) {
+  ValueWorklist worklist(original->getFunction());
+  worklist.push(original);
+  if (auto *phi = SILArgument::asPhi(original)) {
     phi->visitTransitiveIncomingPhiOperands([&](auto *phi, auto *operand) {
       worklist.pushIfNotVisited(operand->get());
       return true;
     });
   }
 
   while (auto value = worklist.pop()) {
-    for (auto *op : value->getUses()) {
-      switch (op->getOperandOwnership()) {
-      case OperandOwnership::ForwardingUnowned:
+    for (auto use : value->getUses()) {
+      switch (use->getOperandOwnership()) {
       case OperandOwnership::PointerEscape:
+      case OperandOwnership::ForwardingUnowned:
         return true;
-
+      case OperandOwnership::ForwardingConsume: {
+        auto *branch = dyn_cast<BranchInst>(use->getUser());
+        if (!branch) {
+          // Non-phi forwarding consumes end the lifetime of an owned value.
+          break;
+        }
+        auto *phi = branch->getDestBB()->getArgument(use->getOperandNumber());
+        worklist.pushIfNotVisited(phi);
+        break;
+      }
+      case OperandOwnership::Borrow: {
+        auto borrowOp = BorrowingOperand(use);
+        if (auto borrowValue = borrowOp.getBorrowIntroducingUserResult()) {
+          worklist.pushIfNotVisited(borrowValue.value);
+        }
+        break;
+      }
       case OperandOwnership::Reborrow: {
-        SILArgument *phi = PhiOperand(op).getValue();
+        SILArgument *phi = PhiOperand(use).getValue();
         worklist.pushIfNotVisited(phi);
         break;
       }
-
       case OperandOwnership::GuaranteedForwarding: {
         // This may follow guaranteed phis.
-        ForwardingOperand(op).visitForwardedValues([&](SILValue result) {
+        ForwardingOperand(use).visitForwardedValues([&](SILValue result) {
           // Do not include transitive uses with 'none' ownership
           if (result->getOwnershipKind() == OwnershipKind::None)
             return true;
@@ -60,42 +79,11 @@ bool swift::hasPointerEscape(BorrowedValue original) {
         });
         break;
       }
-      default:
-        break;
-      }
-    }
-  }
-  return false;
-}
-
-bool swift::hasPointerEscape(SILValue original) {
-  if (auto borrowedValue = BorrowedValue(original)) {
-    return hasPointerEscape(borrowedValue);
-  }
-  assert(original->getOwnershipKind() == OwnershipKind::Owned);
-
-  ValueWorklist worklist(original->getFunction());
-  worklist.push(original);
-  if (auto *phi = SILArgument::asPhi(original)) {
-    phi->visitTransitiveIncomingPhiOperands([&](auto *phi, auto *operand) {
-      worklist.pushIfNotVisited(operand->get());
-      return true;
-    });
-  }
-  while (auto value = worklist.pop()) {
-    for (auto use : value->getUses()) {
-      switch (use->getOperandOwnership()) {
-      case OperandOwnership::PointerEscape:
-      case OperandOwnership::ForwardingUnowned:
-        return true;
-      case OperandOwnership::ForwardingConsume: {
-        auto *branch = dyn_cast<BranchInst>(use->getUser());
-        if (!branch) {
-          // Non-phi forwarding consumes end the lifetime of an owned value.
-          break;
+      case OperandOwnership::InteriorPointer: {
+        if (InteriorPointerOperand(use).findTransitiveUses() !=
+            AddressUseKind::NonEscaping) {
+          return true;
         }
-        auto *phi = branch->getDestBB()->getArgument(use->getOperandNumber());
-        worklist.pushIfNotVisited(phi);
         break;
       }
       default:
@@ -2282,27 +2270,27 @@ bool swift::isRedundantMoveValue(MoveValueInst *mvi) {
   //
   // Check this in two ways, one cheaper than the other.
 
-  // First, avoid calling hasPointerEscape(original).
+  // First, avoid calling findPointerEscape(original).
   //
   // If the original value is not a phi (a phi's incoming values might have
   // escaping uses) and its only user is the move, then it doesn't escape. Also
   // if its only user is the move, then its only _consuming_ user is the move.
   auto *singleUser =
       original->getSingleUse() ? original->getSingleUse()->getUser() : nullptr;
   if (mvi == singleUser && !SILArgument::asPhi(original)) {
-    assert(!hasPointerEscape(original));
+    assert(!findPointerEscape(original));
     assert(original->getSingleConsumingUse()->getUser() == mvi);
     // - !escaping(original)
     // - singleConsumingUser(original) == move
     return true;
   }
 
-  // Second, call hasPointerEscape(original).
+  // Second, call findPointerEscape(original).
   //
   // Explicitly check both
   // - !escaping(original)
   // - singleConsumingUser(original) == move
-  auto originalHasEscape = hasPointerEscape(original);
+  auto originalHasEscape = findPointerEscape(original);
   auto *singleConsumingUser = original->getSingleConsumingUse()
                                   ? original->getSingleConsumingUse()->getUser()
                                   : nullptr;
@@ -2311,6 +2299,6 @@ bool swift::isRedundantMoveValue(MoveValueInst *mvi) {
   }
 
   // (3) Escaping matches?  (Expensive check, saved for last.)
-  auto moveHasEscape = hasPointerEscape(mvi);
+  auto moveHasEscape = findPointerEscape(mvi);
   return moveHasEscape == originalHasEscape;
 }
diff --git a/lib/SILOptimizer/SemanticARC/CopyValueOpts.cpp b/lib/SILOptimizer/SemanticARC/CopyValueOpts.cpp
@@ -511,7 +511,7 @@ static bool tryJoinIfDestroyConsumingUseInSameBlock(
   // Check whether the uses considered immediately above are all effectively
   // instantaneous uses. Pointer escapes propagate values ways that may not be
   // discoverable.
-  if (hasPointerEscape(operand)) {
+  if (findPointerEscape(operand)) {
     return false;
   }
 
diff --git a/lib/SILOptimizer/Transforms/DeadCodeElimination.cpp b/lib/SILOptimizer/Transforms/DeadCodeElimination.cpp
@@ -277,7 +277,7 @@ void DCE::markLive() {
       case SILInstructionKind::DestroyValueInst: {
         auto phi = PhiValue(I.getOperand(0));
         // Disable DCE of phis which are lexical or may have a pointer escape.
-        if (phi && (phi->isLexical() || hasPointerEscape(phi))) {
+        if (phi && (phi->isLexical() || findPointerEscape(phi))) {
           markInstructionLive(&I);
         }
         // The instruction is live only if it's operand value is also live
@@ -287,7 +287,7 @@ void DCE::markLive() {
       case SILInstructionKind::EndBorrowInst: {
         auto phi = PhiValue(I.getOperand(0));
         // If there is a pointer escape or phi is lexical, disable DCE.
-        if (phi && (hasPointerEscape(phi) || phi->isLexical())) {
+        if (phi && (findPointerEscape(phi) || phi->isLexical())) {
           markInstructionLive(&I);
         }
         // The instruction is live only if it's operand value is also live
diff --git a/lib/SILOptimizer/UtilityPasses/UnitTestRunner.cpp b/lib/SILOptimizer/UtilityPasses/UnitTestRunner.cpp
@@ -281,7 +281,7 @@ struct OwnershipUtilsHasPointerEscape : UnitTest {
   OwnershipUtilsHasPointerEscape(UnitTestRunner *pass) : UnitTest(pass) {}
   void invoke(Arguments &arguments) override {
     auto value = arguments.takeValue();
-    auto has = hasPointerEscape(value);
+    auto has = findPointerEscape(value);
     value->print(llvm::errs());
     auto *boolString = has ? "true" : "false";
     llvm::errs() << boolString << "\n";
diff --git a/test/SILOptimizer/copy_propagation_borrow.sil b/test/SILOptimizer/copy_propagation_borrow.sil
@@ -501,6 +501,7 @@ bb0:
   %copy1 = copy_value %borrow1 : $C
   %borrow2 = begin_borrow %copy1 : $C
   %addr = ref_element_addr %borrow2 : $C, #C.a
+  %ptr = address_to_pointer %addr : $*Int64 to $Builtin.RawPointer
   cond_br undef, bb1, bb2
 bb1:
   // inside use
@@ -1201,4 +1202,3 @@ bb0(%0 : @guaranteed $C):
   %7 = tuple ()
   return %7 : $()
 }
-
diff --git a/test/SILOptimizer/liveness_unit.sil b/test/SILOptimizer/liveness_unit.sil
@@ -222,19 +222,13 @@ bb4(%phi : @owned $C):
   return %99 : $()
 }
 
-// A dead (ref_element_addr) projection is treated like a pointer
-// escape for convenience. Make sure the pointer escape bubbles up
-// through the phi and casts.
-//
 // CHECK-LABEL: testSSADeadRefElementAddr: ssa-liveness
 // CHECK: SSA lifetime analysis: %0 = argument of bb0 : $C
-// CHECK-NEXT: Incomplete liveness: Escaping address
 // CHECK-NEXT: bb0: LiveOut
 // CHECK-NEXT: bb2: LiveOut
 // CHECK-NEXT: bb3: LiveWithin
 // CHECK-NEXT: bb1: LiveOut
-// CHECK: last user:
-// CHECK-SAME: ref_element_addr %6 : $D, #D.object
+// CHECK: ref_element_addr %6 : $D, #D.object
 // CHECK-NEXT-LABEL: end running test 1 of 1 on testSSADeadRefElementAddr: ssa-liveness
 sil [ossa] @testSSADeadRefElementAddr : $@convention(thin) (@guaranteed C) -> () {
 bb0(%0 : @guaranteed $C):

Original file line number	Diff line number	Diff line change
`@@ -511,7 +511,7 @@ static bool tryJoinIfDestroyConsumingUseInSameBlock(`
`511`	`511`	`// Check whether the uses considered immediately above are all effectively`
`512`	`512`	`// instantaneous uses. Pointer escapes propagate values ways that may not be`
`513`	`513`	`// discoverable.`
`514`		`- if (hasPointerEscape(operand)) {`
	`514`	`+ if (findPointerEscape(operand)) {`
`515`	`515`	`return false;`
`516`	`516`	`}`
`517`	`517`