Update isGuaranteedForwarding api and delete isGuaranteedForwardingPhi api

Author: meg-gupta

include/swift/SIL/OwnershipUtils.h

@@ -69,50 +69,58 @@ struct BorrowedValue;
 // TODO: encapsulate in a ForwardingInstruction abstraction
 //===----------------------------------------------------------------------===//
 
-/// Is the opcode that produces \p value capable of forwarding guaranteed
+/// Is the opcode that produces \p value capable of forwarding inner guaranteed
 /// values?
 ///
 /// This may be true even if the current instance of the instruction is not a
 /// GuaranteedForwarding. If true, then the operation may be trivially rewritten
 /// with Guaranteed ownership.
-bool canOpcodeForwardGuaranteedValues(SILValue value);
+bool canOpcodeForwardInnerGuaranteedValues(SILValue value);
 
-/// Is the opcode that consumes \p use capable of forwarding guaranteed values?
+/// Is the opcode that consumes \p use capable of forwarding inner guaranteed
+/// values?
 ///
 /// This may be true even if \p use is not a GuaranteedForwarding. If true, then
 /// the operation may be trivially rewritten with Guaranteed ownership.
-bool canOpcodeForwardGuaranteedValues(Operand *use);
+bool canOpcodeForwardInnerGuaranteedValues(Operand *use);
 
 // This is the use-def equivalent of use->getOperandOwnership() ==
 // OperandOwnership::GuaranteedForwarding.
 inline bool isGuaranteedForwarding(SILValue value) {
-  assert(value->getOwnershipKind() == OwnershipKind::Guaranteed);
-  return canOpcodeForwardGuaranteedValues(value);
-}
-
-/// Returns true if it is a forwarding phi
-inline bool isGuaranteedForwardingPhi(SILValue value) {
   if (value->getOwnershipKind() != OwnershipKind::Guaranteed) {
     return false;
   }
-  if (isa<BeginBorrowInst>(value) || isa<LoadBorrowInst>(value)) {
-    return false;
+  // NOTE: canOpcodeForwardInnerGuaranteedValues returns true for transformation
+  // terminator results.
+  if (canOpcodeForwardInnerGuaranteedValues(value) ||
+      isa<SILFunctionArgument>(value)) {
+    return true;
   }
+  // If not a phi, return false
   auto *phi = dyn_cast<SILPhiArgument>(value);
   if (!phi || !phi->isPhi()) {
-    return true;
+    return false;
   }
-  bool isGuaranteedForwardingPhi = true;
-  phi->visitTransitiveIncomingPhiOperands(
-      [&](auto *phi, auto *operand) -> bool {
-        if (isa<BeginBorrowInst>(operand->get()) ||
-            isa<LoadBorrowInst>(operand->get())) {
-          isGuaranteedForwardingPhi = false;
-          return false;
-        }
-        return true;
-      });
-
+  // For a phi, if we find GuaranteedForwarding phi operand on any incoming
+  // path, we return true. Additional verification is added to ensure
+  // GuaranteedForwarding phi operands are found on zero or all paths in the
+  // OwnershipVerifier.
+  bool isGuaranteedForwardingPhi = false;
+  phi->visitTransitiveIncomingPhiOperands([&](auto *, auto *op) -> bool {
+    auto opValue = op->get();
+    assert(opValue->getOwnershipKind().isCompatibleWith(
+        OwnershipKind::Guaranteed));
+    if (canOpcodeForwardInnerGuaranteedValues(opValue) ||
+        isa<SILFunctionArgument>(opValue)) {
+      isGuaranteedForwardingPhi = true;
+      return false;
+    }
+    auto *phi = dyn_cast<SILPhiArgument>(opValue);
+    if (!phi || !phi->isPhi()) {
+      return false;
+    }
+    return true;
+  });
   return isGuaranteedForwardingPhi;
 }
 
@@ -527,7 +535,7 @@ class BorrowedValueKind {
                        })) {
         return Kind::Invalid;
       }
-      if (isGuaranteedForwardingPhi(value)) {
+      if (isGuaranteedForwarding(value)) {
         return Kind::Invalid;
       }
       return Kind::Phi;

lib/SIL/IR/OperandOwnership.cpp

@@ -27,9 +27,6 @@ bool swift::checkOperandOwnershipInvariants(const Operand *operand,
     // Must be a valid BorrowingOperand.
     return bool(BorrowingOperand(const_cast<Operand *>(operand)));
   }
-  if (opOwnership == OperandOwnership::GuaranteedForwarding) {
-    return canOpcodeForwardGuaranteedValues(const_cast<Operand *>(operand));
-  }
   return true;
 }
 
@@ -430,7 +427,7 @@ OperandOwnership OperandOwnershipClassifier::visitBranchInst(BranchInst *bi) {
       bi->getDestBB()->getArgument(getOperandIndex())->getOwnershipKind();
 
   if (destBlockArgOwnershipKind == OwnershipKind::Guaranteed) {
-    return isGuaranteedForwardingPhi(getValue())
+    return isGuaranteedForwarding(getValue())
                ? OperandOwnership::GuaranteedForwardingPhi
                : OperandOwnership::Reborrow;
   }

lib/SIL/Utils/OwnershipUtils.cpp

@@ -84,17 +84,23 @@ bool swift::hasPointerEscape(BorrowedValue value) {
   return false;
 }
 
-bool swift::canOpcodeForwardGuaranteedValues(SILValue value) {
-  if (auto *inst = value->getDefiningInstructionOrTerminator()) {
-    if (auto *mixin = OwnershipForwardingMixin::get(inst)) {
+bool swift::canOpcodeForwardInnerGuaranteedValues(SILValue value) {
+  // If we have an argument from a transforming terminator, we can forward
+  // guaranteed.
+  if (auto *arg = dyn_cast<SILArgument>(value))
+    if (auto *ti = arg->getSingleTerminator())
+      if (ti->mayHaveTerminatorResult())
+        return OwnershipForwardingMixin::get(ti)->preservesOwnership();
+
+  if (auto *inst = value->getDefiningInstruction())
+    if (auto *mixin = OwnershipForwardingMixin::get(inst))
       return mixin->preservesOwnership() &&
-        !isa<OwnedFirstArgForwardingSingleValueInst>(inst);
-    }
-  }
+             !isa<OwnedFirstArgForwardingSingleValueInst>(inst);
+
   return false;
 }
 
-bool swift::canOpcodeForwardGuaranteedValues(Operand *use) {
+bool swift::canOpcodeForwardInnerGuaranteedValues(Operand *use) {
   if (auto *mixin = OwnershipForwardingMixin::get(use->getUser()))
     return mixin->preservesOwnership() &&
            !isa<OwnedFirstArgForwardingSingleValueInst>(use->getUser());
@@ -1208,11 +1214,12 @@ bool swift::getAllBorrowIntroducingValues(SILValue inputValue,
   if (inputValue->getOwnershipKind() != OwnershipKind::Guaranteed)
     return false;
 
-  SmallVector<SILValue, 32> worklist;
-  worklist.emplace_back(inputValue);
+  SmallSetVector<SILValue, 32> worklist;
+  worklist.insert(inputValue);
 
-  while (!worklist.empty()) {
-    SILValue value = worklist.pop_back_val();
+  // worklist grows in this loop.
+  for (unsigned idx = 0; idx < worklist.size(); idx++) {
+    SILValue value = worklist[idx];
 
     // First check if v is an introducer. If so, stash it and continue.
     if (auto scopeIntroducer = BorrowedValue(value)) {
@@ -1230,11 +1237,26 @@ bool swift::getAllBorrowIntroducingValues(SILValue inputValue,
     // Otherwise if v is an ownership forwarding value, add its defining
     // instruction
     if (isGuaranteedForwarding(value)) {
-      if (auto *i = value->getDefiningInstructionOrTerminator()) {
-        llvm::copy(i->getNonTypeDependentOperandValues(),
-                   std::back_inserter(worklist));
+      if (auto *i = value->getDefiningInstruction()) {
+        for (SILValue opValue : i->getNonTypeDependentOperandValues()) {
+          worklist.insert(opValue);
+        }
         continue;
       }
+
+      // Otherwise, we should have a block argument that is defined by a single
+      // predecessor terminator.
+      auto *arg = cast<SILPhiArgument>(value);
+      if (arg->isTerminatorResult()) {
+        if (auto *forwardedOper = arg->forwardedTerminatorResultOperand()) {
+          worklist.insert(forwardedOper->get());
+          continue;
+        }
+      }
+      arg->visitIncomingPhiOperands([&](auto *operand) {
+        worklist.insert(operand->get());
+        return true;
+      });
     }
 
     // Otherwise, this is an introducer we do not understand. Bail and return

lib/SIL/Verifier/SILOwnershipVerifier.cpp

@@ -280,7 +280,7 @@ bool SILValueOwnershipChecker::gatherUsers(
   // this value forwards guaranteed ownership. In such a case, we are going to
   // validate it as part of the borrow introducer from which the forwarding
   // value originates. So we can just return true and continue.
-  if (isGuaranteedForwarding(value))
+  if (canOpcodeForwardInnerGuaranteedValues(value))
     return true;
 
   // Ok, we have some sort of borrow introducer. We need to recursively validate
@@ -554,10 +554,6 @@ bool SILValueOwnershipChecker::checkValueWithoutLifetimeEndingUses(
     if (isGuaranteedForwarding(value)) {
       return true;
     }
-    auto *phi = dyn_cast<SILPhiArgument>(value);
-    if (phi && isGuaranteedForwardingPhi(phi)) {
-      return true;
-    }
   }
 
   // If we have an unowned value, then again there is nothing left to do.
@@ -675,8 +671,7 @@ bool SILValueOwnershipChecker::checkUses() {
   // Check if we are an instruction that forwards guaranteed
   // ownership. In such a case, we are a subobject projection. We should not
   // have any lifetime ending uses.
-  if (value->getOwnershipKind() == OwnershipKind::Guaranteed &&
-      isGuaranteedForwarding(value)) {
+  if (isGuaranteedForwarding(value)) {
     if (!isSubobjectProjectionWithLifetimeEndingUses(value,
                                                      lifetimeEndingUsers)) {
       return false;

lib/SILOptimizer/SemanticARC/OwnershipLiveRange.cpp

@@ -69,9 +69,12 @@ OwnershipLiveRange::OwnershipLiveRange(SILValue value)
     // DISCUSSION: For now we do not support forwarding instructions with
     // multiple non-trivial arguments since we would need to optimize all of
     // the non-trivial arguments at the same time.
+    //
+    // NOTE: Today we do not support TermInsts for simplicity... we /could/
+    // support it though if we need to.
     auto *ti = dyn_cast<TermInst>(user);
-    if ((ti && !ti->forwardedOperand()) ||
-        !canOpcodeForwardGuaranteedValues(op) ||
+    if ((ti && !ti->mayHaveTerminatorResult()) ||
+        !canOpcodeForwardInnerGuaranteedValues(op) ||
         1 !=
             count_if(user->getNonTypeDependentOperandValues(), [&](SILValue v) {
               return v->getOwnershipKind() == OwnershipKind::Owned;

test/SIL/OwnershipVerifier/arguments.sil

@@ -54,40 +54,13 @@ bb1(%2 : @guaranteed $Builtin.NativeObject):
 
 // CHECK-LABEL: Error#: 0. Begin Error in Function: 'leak_along_path'
 // CHECK: Error! Found a leak due to a consuming post-dominance failure!
-// CHECK: Value: %2 = argument of bb1 : $Builtin.NativeObject
+// CHECK: Value: %3 = argument of bb1 : $Builtin.NativeObject
 // CHECK: Post Dominating Failure Blocks:
 // CHECK: bb3
 // CHECK: Error#: 0. End Error in Function: 'leak_along_path'
 //
 // CHECK-NOT: Error#: {{[0-9][0-9]*}}. End Error in Function: 'leak_along_path'
 sil [ossa] @leak_along_path : $@convention(thin) (@guaranteed Builtin.NativeObject) -> () {
-bb0(%0 : @guaranteed $Builtin.NativeObject):
-  br bb1(%0 : $Builtin.NativeObject)
-
-bb1(%1 : @guaranteed $Builtin.NativeObject):
-  cond_br undef, bb2, bb3
-
-bb2:
-  end_borrow %1 : $Builtin.NativeObject
-  br bb4
-
-bb3:
-  br bb4
-
-bb4:
-  %9999 = tuple()
-  return %9999 : $()
-}
-
-// CHECK-LABEL: Error#: 0. Begin Error in Function: 'leak_along_path_2'
-// CHECK: Error! Found a leak due to a consuming post-dominance failure!
-// CHECK: Value: %3 = argument of bb1 : $Builtin.NativeObject
-// CHECK: Post Dominating Failure Blocks:
-// CHECK: bb3
-// CHECK: Error#: 0. End Error in Function: 'leak_along_path_2'
-//
-// CHECK-NOT: Error#: {{[0-9][0-9]*}}. End Error in Function: 'leak_along_path_2'
-sil [ossa] @leak_along_path_2 : $@convention(thin) (@guaranteed Builtin.NativeObject) -> () {
 bb0(%0 : @guaranteed $Builtin.NativeObject):
   %1 = begin_borrow %0 : $Builtin.NativeObject
   br bb1(%1 : $Builtin.NativeObject)
@@ -109,53 +82,16 @@ bb4:
 
 // Make sure that we only flag the subargument leak and not the parent
 // argument. Also check for over consuming due to phi nodes.
-//
+
 // CHECK-LABEL: Error#: 0. Begin Error in Function: 'leak_along_subarg_path'
 // CHECK: Error! Found a leak due to a consuming post-dominance failure!
-// CHECK: Value: %6 = argument of bb4 : $Builtin.NativeObject
+// CHECK: Value: %7 = argument of bb3 : $Builtin.NativeObject
 // CHECK: Post Dominating Failure Blocks:
 // CHECK: bb6
 // CHECK: Error#: 0. End Error in Function: 'leak_along_subarg_path'
 //
 // CHECK-NOT: Error#: {{[0-9][0-9]*}}. End Error in Function: 'leak_along_subarg_path'
 sil [ossa] @leak_along_subarg_path : $@convention(thin) (@guaranteed Builtin.NativeObject) -> () {
-bb0(%0 : @guaranteed $Builtin.NativeObject):
-  br bb1(%0 : $Builtin.NativeObject)
-
-bb1(%1 : @guaranteed $Builtin.NativeObject):
-  cond_br undef, bb2, bb3
-
-bb2:
-  br bb7
-
-bb3:
-  br bb4(%1 : $Builtin.NativeObject)
-
-bb4(%2 : @guaranteed $Builtin.NativeObject):
-  cond_br undef, bb5, bb6
-
-bb5:
-  end_borrow %2 : $Builtin.NativeObject
-  br bb7
-
-bb6:
-  br bb7
-
-bb7:
-  end_borrow %1 : $Builtin.NativeObject
-  %9999 = tuple()
-  return %9999 : $()
-}
-
-// CHECK-LABEL: Error#: 0. Begin Error in Function: 'leak_along_subarg_path_2'
-// CHECK: Error! Found a leak due to a consuming post-dominance failure!
-// CHECK: Value: %7 = argument of bb3 : $Builtin.NativeObject
-// CHECK: Post Dominating Failure Blocks:
-// CHECK: bb6
-// CHECK: Error#: 0. End Error in Function: 'leak_along_subarg_path_2'
-//
-// CHECK-NOT: Error#: {{[0-9][0-9]*}}. End Error in Function: 'leak_along_subarg_path_2'
-sil [ossa] @leak_along_subarg_path_2 : $@convention(thin) (@guaranteed Builtin.NativeObject) -> () {
 bb0(%0 : @guaranteed $Builtin.NativeObject):
   %1 = begin_borrow %0 : $Builtin.NativeObject
   br bb1(%1 : $Builtin.NativeObject)

test/SIL/OwnershipVerifier/use_verifier.sil

@@ -1206,7 +1206,8 @@ bb1:
 
 bb2:
   %5 = struct_extract %1 : $NonTrivialStructWithTrivialField, #NonTrivialStructWithTrivialField.owner
-  br bb3(%5 : $Builtin.NativeObject)
+  %6 = begin_borrow %5 : $Builtin.NativeObject
+  br bb3(%6 : $Builtin.NativeObject)
 
 bb3(%7 : @guaranteed $Builtin.NativeObject):
   end_borrow %7 : $Builtin.NativeObject
@@ -1366,7 +1367,6 @@ bb2:
   br bb3(%f2thick : $@callee_owned () -> ())
 
 bb3(%fUnknown : @guaranteed $@callee_owned () -> ()):
-  end_borrow %fUnknown : $@callee_owned () -> ()
   %9999 = tuple()
   return %9999 : $()
 }

test/SILOptimizer/redundant_phi_elimination_ossa.sil

@@ -183,8 +183,6 @@ bb2:
   br bb3(%0 : $FakeOptional<Klass>, %0 : $FakeOptional<Klass>)
 
 bb3(%1 : @guaranteed $FakeOptional<Klass>, %2 : @guaranteed $FakeOptional<Klass>):
-  end_borrow %1 : $FakeOptional<Klass>
-  end_borrow %2 : $FakeOptional<Klass>
   %res = tuple ()
   return %res : $()
 }
@@ -195,17 +193,18 @@ bb3(%1 : @guaranteed $FakeOptional<Klass>, %2 : @guaranteed $FakeOptional<Klass>
 sil [ossa] @test_redundantguaranteedphiarg2 : $@convention(thin) () -> () {
 bb0:
   %0 = enum $FakeOptional<Klass>, #FakeOptional.none!enumelt 
+  %b = begin_borrow %0 : $FakeOptional<Klass>
   cond_br undef, bb1, bb2
 
 bb1:
-  br bb3(%0 : $FakeOptional<Klass>, %0 : $FakeOptional<Klass>)
+  br bb3(%0 : $FakeOptional<Klass>, %b : $FakeOptional<Klass>)
 
 bb2:
-  br bb3(%0 : $FakeOptional<Klass>, %0 : $FakeOptional<Klass>)
+  br bb3(%0 : $FakeOptional<Klass>, %b : $FakeOptional<Klass>)
 
 bb3(%1 : @owned $FakeOptional<Klass>, %2 : @guaranteed $FakeOptional<Klass>):
-  destroy_value %1 : $FakeOptional<Klass>
   end_borrow %2 : $FakeOptional<Klass>
+  destroy_value %1 : $FakeOptional<Klass>
   %res = tuple ()
   return %res : $()
 }
@@ -216,13 +215,15 @@ bb3(%1 : @owned $FakeOptional<Klass>, %2 : @guaranteed $FakeOptional<Klass>):
 sil [ossa] @test_redundantguaranteedphiarg3 : $@convention(thin) () -> () {
 bb0:
   %0 = enum $FakeOptional<Klass>, #FakeOptional.none!enumelt 
+  %b1 = begin_borrow %0 : $FakeOptional<Klass>
+  %b2 = begin_borrow %0 : $FakeOptional<Klass>
   cond_br undef, bb1, bb2
 
 bb1:
-  br bb3(%0 : $FakeOptional<Klass>, %0 : $FakeOptional<Klass>)
+  br bb3(%b1 : $FakeOptional<Klass>, %b2 : $FakeOptional<Klass>)
 
 bb2:
-  br bb3(%0 : $FakeOptional<Klass>, %0 : $FakeOptional<Klass>)
+  br bb3(%b1 : $FakeOptional<Klass>, %b2 : $FakeOptional<Klass>)
 
 bb3(%1 : @guaranteed $FakeOptional<Klass>, %2 : @guaranteed $FakeOptional<Klass>):
   cond_br undef, bb4, bb5