Support @guaranteed forwarding phis #61505

meg-gupta · 2022-10-08T07:21:13Z

We required borrow scopes to pass @guaranteed forwarding values as phi operands. With this change we eliminate unnecessary borrow scopes making it easier for optimizations to work with @guaranteed forwarding values.

// This SIL:

%outer = begin_borrow %_ : $T
  cond_br _, bb1, bb2
bb1:
  %cast1 = unchecked_ref_cast %outer : $T to U
  %inner1 = begin_borrow %cast1 : $U
  br bb3(%inner1 : $U)
bb2:
  %cast2 = unchecked_ref_cast %outer : $T to U
  %inner2 = begin_borrow %cast2 : $U
  br bb3(%inner1 : $U)
bb3(%phi : $U):
  //...
  end_borrow %phi : $U
  end_borrow %outer : $T

// Can now be written as:

  %outer = begin_borrow %_ : $T
  cond_br _, bb1, bb2
bb1:
  %cast1 = unchecked_ref_cast %outer : $T to U
w  br bb3(%cast1 : $U)
bb2:
  %cast2 = unchecked_ref_cast %outer : $T to U
  br bb3(%cast2 : $U)
bb3(%phi : $U):
  //...
  end_borrow %outer : $T

meg-gupta · 2022-10-08T07:23:05Z

@swift-ci test

meg-gupta · 2022-10-08T07:23:31Z

@swift-ci test source compatibility

meg-gupta · 2022-10-08T07:23:39Z

@swift-ci benchmark

meg-gupta · 2022-10-09T03:38:05Z

@swift-ci test

meg-gupta · 2022-10-09T05:24:22Z

@swift-ci test

meg-gupta · 2022-10-10T17:05:06Z

@swift-ci test

meg-gupta · 2022-10-10T19:37:12Z

@swift-ci test

meg-gupta · 2022-10-11T05:19:22Z

@swift-ci test Linux platform

meg-gupta · 2022-10-11T17:24:55Z

@swift-ci test macOS platform

meg-gupta · 2022-10-11T17:25:03Z

@swift-ci benchmark

meg-gupta · 2022-10-11T17:25:15Z

@swift-ci test source compatibility

atrick · 2022-10-11T21:09:21Z

lib/SIL/IR/OperandOwnership.cpp

  if (destBlockArgOwnershipKind == OwnershipKind::Guaranteed) {
-    return OperandOwnership::Reborrow;
+    auto phiOp = getValue();


I wonder why you can't just call isGuaranteedForwardingPhi here instead of duplicating all the logic.

atrick · 2022-10-11T22:43:33Z

lib/SIL/Utils/OwnershipUtils.cpp

@@ -339,6 +345,17 @@ bool swift::findExtendedUsesOfSimpleBorrowedValue(
      recordUse(use);
      break;

+    case OperandOwnership::GuaranteedForwardingPhi: {


findExtendedUsesOfSimpleBorrowedValue is finding "copy-extended" uses within the "simple" borrow scope. It doesn't look past reborrows. I think it expects all uses to be dominated by borrowedValue.

So I think it should only follow the uses of a guaranteed phi if that phi is dominated by borrowedValue

findExtendedUsesOfSimpleBorrowedValue is just recording the use of Reborrow, and the clients are bailing out if there is a Reborrow before calling this api. I changed findExtendedUsesOfSimpleBorrowedValue to return false when there is a Reborrow (similar to findUsesOfSimpleValue). With that, all collected GuranteedForwardingPhi ops will have a dominated borrow root.

In OperandOwnership::GuaranteedForwardingPhi: { please comment "borrowedValue is known to dominate this phi because if any reborrow exists then this utility returns false

atrick · 2022-10-11T23:06:23Z

lib/SIL/Utils/OwnershipUtils.cpp

@@ -842,6 +859,85 @@ bool BorrowedValue::visitTransitiveLifetimeEndingUses(
  return true;
 }

+bool BorrowedValue::visitTransitiveGuaranteedForwardingUses(


findTransitiveGuaranteed may reach the same instruction via multiple paths. We need to make sure we don't revisit the same instruction and have exponential path explosion

atrick · 2022-10-11T23:17:28Z

lib/SIL/Utils/OwnershipUtils.cpp

+  return true;
+}
+
+bool BorrowedValue::visitTransitiveGuaranteedForwardingPhiUses(


Let's make sure that these utilities are only called on borrow-introducing values.

How can we make it clear that this utility is intentionally skipping over the first level of phi uses (that wasn't clear to me).

I wasn't accurate when I said visitTransitiveGuaranteedForwardingPhis these will only be called on borrow value introducers. I have moved it out of BorrowedValue and added assert on values it can accept.

Naming: visitTransitiveGuaranteedForwardingPhis maybe something like
visitGuaranteedForwardingPhisForSSAValue

atrick · 2022-10-11T23:19:48Z

lib/SIL/Utils/OwnershipUtils.cpp

+  }
+
+  for (unsigned i = 0; i < guaranteedForwardingOps.size(); i++) {
+    for (auto val : guaranteedForwardingOps[i]->getUser()->getResults()) {


If we make the structural SIL assumption that all GuaranteedForwarding results are borrowed values then we should check that somewhere

I added a TODO. There are other places in SILOwnershipVerifier where we make the same assumption. I'll take it up as a follow on task.

Maybe a simple assert can be added to verifyOperandOwnership? Such as canOpcodeForwardGuaranteedValues must be true for the user instruction...

atrick · 2022-10-11T23:49:32Z

lib/SIL/Utils/OwnershipUtils.cpp

+    visitGuaranteedForwardingPhiBaseValuePair(phiValue, newBaseValue);
+
+    collectGuaranteedForwardingPhis(phiValue, newBaseValue);
+    collectGuaranteedForwardingPhis(newBaseValue, newBaseValue);


Explain: if an adjacent reborrow is found in the same block as the guaranteed phi, then set newBaseValue to the reborrow.

If we didn't find a reborrow, then don't call this
collectGuaranteedForwardingPhis(newBaseValue, newBaseValue);

atrick · 2022-10-11T23:55:36Z

lib/SIL/Utils/OwnershipUtils.cpp

+         borrow.kind == BorrowedValueKind::LoadBorrow);
+  // worklist stores (guaranteed forwarding phi, base value) pairs.
+  // We need a SetVector to make sure we don't revisit the same pair again.
+  SmallSetVector<std::tuple<PhiOperand, SILValue>, 4> worklist;


Do we really want to visit the same phi multiple times with a different base value each time?
I worry that could cause an infinite worklist.
Can we assert that a guaranteed phi always has the same borrow base?
Why is the worklist tracking operands? A phi should be identified by the its value.
Instead of std::tuple<PhiOperand, SILValue> worklist
Maybe it should be
ValueSet visitedPhis
`SmallVector<std::tuple<SILValue, SILValue> worklist // (guaranteedPhi, borrowBase)

We can have different base values on different paths:

sil [ossa] @test_forwarded_separate_base_values : $@convention(thin) (@owned Wrapper1) -> () { bb0(%0 : @owned $Wrapper1): %outer = begin_borrow %0 : $Wrapper1 cond_br undef, bb1, bb2 bb1: %unrelated = begin_borrow %0 : $Wrapper1 %ex1 = struct_extract %outer : $Wrapper1, #Wrapper1.val1 br bb3(%ex1 : $Wrapper2, %unrelated : $Wrapper1) bb2: %inner = begin_borrow %0 : $Wrapper1 %ex2 = struct_extract %inner : $Wrapper1, #Wrapper1.val1 br bb3(%ex2 : $Wrapper2, %inner : $Wrapper1) bb3(%phi1 : @guaranteed $Wrapper2, %phi2 : @guaranteed $Wrapper1): %f = function_ref @use_wrapper2 : $@convention(thin) (@guaranteed Wrapper2) -> () apply %f(%phi1) : $@convention(thin) (@guaranteed Wrapper2) -> () end_borrow %phi2 : $Wrapper1 end_borrow %outer : $Wrapper1 destroy_value %0 : $Wrapper1 %9999 = tuple() return %9999 : $() }

Here %phi1 has %phi2 as base value on one path and %outer as base value in another.

Comment long term, would like a "single-base" invariant!

Instead of saying we want a "single-base" invariant. Please add the .sil above as a basic OSSA verification test case. Also please add the same test where the base is an owned value. That you can refer to the fact that a guaranteed phi can have multiple bases in a comment here, along with a stripped down version of the SIL. This comment and example should also appear at the top of the reborrow verifier!

atrick · 2022-10-11T23:58:00Z

lib/SIL/Utils/OwnershipUtils.cpp

+  return true;
+}
+
+bool BorrowedValue::visitExtendedTransitiveGuaranteedForwardingPhiUses(


The combination of BorrowedValue::visitTransitiveGuaranteedForwardingPhiUses and BorrowedValue::visitExtendedTransitiveGuaranteedForwardingPhiUses only makes sense as a single utility:
BorrowedValue::visitExtendedTransitiveGuaranteedForwardingPhis
that "finds all reachable guaranteed phis within a simple borrow scope"

We shouldn't need two levels of loops. We can have one loop that uses the same visited set and worklist.
We can assert that we can't have a guaranteed phi that is a direct use of borrowedValue.

I just have one visitor now: visitTransitiveGuaranteedForwardingPhis which finds all reachable guaranteed forwarding phis within a simple borrow scope

One more thing... the first level of traversal can find more guaranteed forwarding phis (if we start from a guaranteed forwarding phi)

atrick · 2022-10-12T00:04:15Z

lib/SIL/Utils/PrunedLiveness.cpp

@@ -295,6 +296,13 @@ SimpleLiveRangeSummary PrunedLiveRange<LivenessWithDefs>::updateForDef(SILValue
      });
      break;
    }
+    case OperandOwnership::GuaranteedForwardingPhi: {
+      SILArgument *phi = cast<BranchInst>(use->getUser())


I think the PhiOperand abstraction handles/hides all the BranchInst/DestBB stuff

meg-gupta · 2022-10-12T08:21:16Z

@swift-ci test

meg-gupta · 2022-10-13T21:40:34Z

@swift-ci test

meg-gupta · 2022-10-13T22:10:08Z

@swift-ci test macOS Platform

meg-gupta · 2022-10-14T16:17:24Z

@swift-ci test

meg-gupta · 2022-10-14T19:10:28Z

@swift-ci test windows platform

atrick · 2022-10-14T23:19:31Z

lib/SIL/Utils/OwnershipUtils.cpp

+  // Iterate guaranteedForwardingOps and call the visitor on
+  // GuaranteedForwardingPhi uses.
+  for (unsigned i = 0; i < guaranteedForwardingOps.size(); i++) {
+    for (auto val : guaranteedForwardingOps[i]->getUser()->getResults()) {


Is this loop necessary? All guaranteed forwarding op results were handled in the loop above. Why not call visitor directly on any phi uses in the above loop?

@atrick
We need to process GuaranteedForwarding op results transitively, to find all GuaranteedForwardingPhi uses.

%b = begin_borrow ... %ex1 = struct_extract %b ... %ex2 = struct_extract %ex1 ... .... br bbdest(%ex2)

We need the second loop to find %ex2

Thanks for clarifying. I have fixed this.

meg-gupta · 2022-10-18T22:00:49Z

@swift-ci test

meg-gupta · 2022-10-19T03:21:35Z

include/swift/SIL/SILValue.h

@@ -919,7 +922,8 @@ inline OwnershipConstraint OperandOwnership::getOwnershipConstraint() {
  case OperandOwnership::ForwardingConsume:
    return {OwnershipKind::Owned, UseLifetimeConstraint::LifetimeEnding};
  case OperandOwnership::InteriorPointer:
-  case OperandOwnership::ForwardingBorrow:
+  case OperandOwnership::GuaranteedForwarding:
+  case OperandOwnership::GuaranteedForwardingPhi:
    return {OwnershipKind::Guaranteed,
            UseLifetimeConstraint::NonLifetimeEnding};


@atrick I fixed GuaranteedForwardingPhi to NonLifetimeEnding!

atrick

LGTM

meg-gupta · 2022-10-19T21:28:32Z

@swift-ci test macOS Platform

path for an optional type.

…guaranteed

…guaranteed forwarding phis

…pport

…oyHoisting

meg-gupta · 2022-10-20T02:55:58Z

Resolved merge conflict...

meg-gupta · 2022-10-20T02:56:14Z

@swift-ci smoke test and merge

Followup for #61505

meg-gupta force-pushed the guaranteedphis branch from 8686d06 to 14fbee4 Compare October 8, 2022 07:22

meg-gupta force-pushed the guaranteedphis branch from 14fbee4 to c0bf82e Compare October 9, 2022 03:37

meg-gupta force-pushed the guaranteedphis branch from c0bf82e to ff3d553 Compare October 9, 2022 05:22

meg-gupta force-pushed the guaranteedphis branch from ff3d553 to 598119b Compare October 10, 2022 07:18

meg-gupta force-pushed the guaranteedphis branch from 598119b to 9cb83c3 Compare October 10, 2022 19:25

meg-gupta force-pushed the guaranteedphis branch from 9cb83c3 to c99d928 Compare October 11, 2022 05:18

meg-gupta marked this pull request as ready for review October 11, 2022 17:24

meg-gupta changed the title ~~[WIP] Support @guaranteed forwarding phis~~ Support @guaranteed forwarding phis Oct 11, 2022

meg-gupta requested review from atrick and nate-chandler October 11, 2022 17:26

atrick requested changes Oct 12, 2022

View reviewed changes

meg-gupta force-pushed the guaranteedphis branch from c99d928 to 68cf6a7 Compare October 12, 2022 08:16

meg-gupta force-pushed the guaranteedphis branch 2 times, most recently from 1eac4cf to cfa3bd5 Compare October 13, 2022 21:24

meg-gupta force-pushed the guaranteedphis branch from cfa3bd5 to 1f08b60 Compare October 14, 2022 02:01

atrick reviewed Oct 14, 2022

View reviewed changes

meg-gupta force-pushed the guaranteedphis branch from 1f08b60 to ebb4deb Compare October 18, 2022 22:00

meg-gupta commented Oct 19, 2022

View reviewed changes

atrick approved these changes Oct 19, 2022

View reviewed changes

meg-gupta added 11 commits October 19, 2022 19:54

Rename ForwardingBorrow -> GuaranteedForwarding

b1f7197

ClosureLifetimeFixup : Ensure borrow scope is inserted on both none/some

dccb5ac

path for an optional type.

Add verification to ensure we don't have redundant borrow scopes for @…

dfbb2bd

…guaranteed forwarding phis

Support @guaranteed forwarding phis

786eb94

Update SemanticARCOpts for @guaranteed forwarding phi support

9b01b87

Update DCE for @guaranteed forwarding phi support

5ede08c

Update castValueToABICompatibleType for @guaranteed forwarding phi su…

569b2aa

…pport

NFC: Remove unused code + reorg

ebd7a90

Update ConditionForwarding for @guaranteed forwarding phi support

4acfd4d

Add unit tests containing guaranteed forwarding phis for LexicalDestr…

0199ff2

…oyHoisting

Remove dead code in LoadBorrowImmutabilityChecker

283f15e

meg-gupta force-pushed the guaranteedphis branch from d5cfafb to 283f15e Compare October 20, 2022 02:55

swift-ci merged commit 24ec0ed into swiftlang:main Oct 20, 2022

meg-gupta mentioned this pull request Oct 21, 2022

Followup for #61505 #61676

Merged

meg-gupta added a commit that referenced this pull request Oct 23, 2022

Merge pull request #61676 from meg-gupta/followup

79565a4

Followup for #61505

compnerd added a commit that referenced this pull request Oct 25, 2022

Revert "Followup for #61505"

8345c30

Support @guaranteed forwarding phis #61505

Support @guaranteed forwarding phis #61505

Uh oh!

Conversation

meg-gupta commented Oct 8, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

meg-gupta commented Oct 8, 2022

Uh oh!

meg-gupta commented Oct 8, 2022

Uh oh!

meg-gupta commented Oct 8, 2022

Uh oh!

meg-gupta commented Oct 9, 2022

Uh oh!

meg-gupta commented Oct 9, 2022

Uh oh!

meg-gupta commented Oct 10, 2022

Uh oh!

meg-gupta commented Oct 10, 2022

Uh oh!

meg-gupta commented Oct 11, 2022

Uh oh!

meg-gupta commented Oct 11, 2022

Uh oh!

meg-gupta commented Oct 11, 2022

Uh oh!

meg-gupta commented Oct 11, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

meg-gupta Oct 12, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

atrick Oct 12, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

meg-gupta Oct 12, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

meg-gupta commented Oct 8, 2022 •

edited

Loading

meg-gupta Oct 12, 2022 •

edited

Loading

atrick Oct 12, 2022 •

edited

Loading

meg-gupta Oct 12, 2022 •

edited

Loading

meg-gupta Oct 12, 2022 •

edited

Loading

meg-gupta Oct 15, 2022 •

edited

Loading