SwiftCompilerSources for ownership, borrowing, and liveness #70887

atrick · 2024-01-12T18:51:13Z

Add BorrowUtils: OSSA infrastructure for borrow scopes.
Key APIs necessary for using OSSA.
- BorrowingInstruction
- BeginBorrowValue
- scopeEndingOperands
- BorrowIntroducers
- EnclosingValues
- innerAdjacentPhis

Add OwnershipLiveness.swift. Core OSSA APIs.
This is what you need to correctly analyze OSSA.
- computeLinearLiveness
- computeInteriorLiveness
- InteriorUseVisitor
- OwnershipUseVisitor
- LivenessBoundary

This incorporates review feedback from
#70460

atrick · 2024-01-12T18:51:25Z

@swift-ci test

atrick · 2024-01-13T04:24:13Z

@swift-ci test

atrick · 2024-01-13T18:33:32Z

This PR is blocked by a likely recent LoadableByAddress bug that happens to be exposed.

atrick · 2024-01-15T01:44:07Z

@swift-ci test

eeckstein

The APIs and utilities in OwnershipLiveness still need quite a lot of polishing, clarification and better documentation.

Note that I also added a few comments in the old PR #70460

eeckstein · 2024-01-15T08:26:27Z

SwiftCompilerSources/Sources/Optimizer/Utilities/BorrowUtils.swift

+/// or introduces a borrow scope, then `value` is the single
+/// introducer for itself.
+///
+/// Example:                                       // introducers:


great comment with the examples!

eeckstein · 2024-01-15T08:39:53Z

SwiftCompilerSources/Sources/Optimizer/Utilities/BorrowUtils.swift

+/// If `value` is a reborrow, then this either returns a dominating
+/// enclosing value or an outer adjacent phi.
+///
+/// Example:                                       // enclosing value:


Again, great that you added these examples!

Which brings me back to the question that I asked in the other PR: It looks like enclosing values are only different than borrow introducers for certain kind of instructions, e.g. BorrowInstructions.

Wouldn't it be easier to gather enclosing values by e.g.

var enclosingValues = Stack() gatherBorrowIntroducers(for: value, in: &borrowIntroducers) for bi in borrowIntroducers { if let bb = BeginBorrowedValue(bi) { gatherBorrowIntroducers(for: bb.baseOperand.value, in: &enclosingValues) } else { enclosingValues.append(bi.value) } }

... which maps 1-1 to the cases you listed in the comment?

That logic is stated in the simplest form in EnclosingValues.gather. It is called recursively with its own state for tracking reborrows.

Those top-level utility functions don't do anything but create a Stack state that's needed for analysis.

Note that EnclosingValues deals with reborrows, and BorrowIntroducers deals with guaranteed forwarding phis. It's a similar problem, so they both use mapToPhi. But it's also useful to know which you're handling, and splitting the utilities made it much easier for me to reason about the state of recursion.

That logic is stated in the simplest form in EnclosingValues.gather.

Right. But why is EnclosingValues.gather(forReborrow:) needed? Aren't enclosing values of a reborrow the same as its borrow introducers? So, shouldn't BorrowIntroducers.gather find the enclosing values?

EnclosingValues deals with reborrows, and BorrowIntroducers deals with guaranteed forwarding phis. It's a similar problem, so they both use mapToPhi. But it's also useful to know which you're handling, and splitting the utilities made it much easier for me to reason about the state of recursion.

SwiftCompilerSources/Sources/Optimizer/Utilities/BorrowUtils.swift

SwiftCompilerSources/Sources/Optimizer/Utilities/ForwardingUtils.swift

eeckstein · 2024-01-15T14:21:55Z

SwiftCompilerSources/Sources/Optimizer/Utilities/OwnershipLiveness.swift

+///
+/// TODO: Verify that pointerEscape is only called for live ranges in which
+/// `findPointerEscape()` returns true.
+protocol AddressUseVisitor {


Also here: AddressUseVisitor is actually derived from AddressDefUseWalker. In addition to AddressDefUseWalker it refines the treatment of leaf-uses. This is how you can build on top of AddressDefUseWalker:

@@ -445,15 +445,12 @@ extension InteriorUseVisitor { /// /// TODO: Verify that pointerEscape is only called for live ranges in which /// `findPointerEscape()` returns true. -protocol AddressUseVisitor { +protocol AddressUseVisitor: AddressDefUseWalker where Path == UnusedWalkingPath { var _context: Context { get } /// Start a def-use walk from and address. mutating func walkDownUses(ofAddress: Value) -> WalkResult - /// Start a def-use walk from an address operand. - mutating func walkDown(address: Operand) -> WalkResult - /// An address projection produces a single address result and does not /// escape its address operand in any other way. /// @@ -529,21 +526,13 @@ extension AddressUseVisitor { return walkDownUses(ofAddress: value) } - mutating func walkDown(address: Operand) -> WalkResult { + mutating func walkDown(address: Operand, path: UnusedWalkingPath = UnusedWalkingPath()) -> WalkResult { switch address.instruction { case let ba as BeginAccessInst: if handle(access: ba) == .abortWalk { return .abortWalk } return ba.endOperands.walk { leafUse(address: $0) } - - case let load as LoadBorrowInst: - if handle(borrow: load) == .abortWalk { - return .abortWalk - } - return BeginBorrowValue(load)!.scopeEndingOperands.walk { - leafUse(address: $0) - } case let markDep as MarkDependenceInst: if markDep.valueOperand == address { return walkDown(projection: markDep, of: address) @@ -561,21 +550,24 @@ extension AddressUseVisitor { */ // A potentially escaping value depends on this address. return pointerEscape(address: address) + default: + break + } + return walkDownDefault(address: address, path: path) + } + mutating func leafUse(address: Operand, path: UnusedWalkingPath) -> WalkResult { + switch address.instruction { + case let load as LoadBorrowInst: + if handle(borrow: load) == .abortWalk { + return .abortWalk + } + return BeginBorrowValue(load)!.scopeEndingOperands.walk { + leafUse(address: $0) + } case let pai as PartialApplyInst where pai.isOnStack: return dependent(value: pai, dependsOn: address) - case is StructElementAddrInst, is TupleElementAddrInst, - is InitEnumDataAddrInst, is UncheckedTakeEnumDataAddrInst, - is InitExistentialAddrInst, is OpenExistentialAddrInst, - is IndexAddrInst, is MarkUnresolvedNonCopyableValueInst, - is ProjectBlockStorageInst, is TailAddrInst, is StoreBorrowInst, - is UncheckedAddrCastInst, is MarkUninitializedInst, is DropDeinitInst, - is TuplePackElementAddrInst, is CopyableToMoveOnlyWrapperAddrInst, - is MoveOnlyWrapperToCopyableAddrInst: - let svi = address.instruction as! SingleValueInstruction - return walkDown(projection: svi, of: address) - case is ReturnInst, is ThrowInst, is YieldInst, is TryApplyInst, is SwitchEnumAddrInst, is CheckedCastAddrBranchInst, is SelectEnumAddrInst, is InjectEnumAddrInst,

That Walker convention is good but I don't want to rename AddressUseVisitor yet. Its purpose of is to visit all of the address uses, not just the leaf uses, partitioning them by the kind of use. This is the most general possible address use visitor, so there's no other sensible name.

Note that I originally wrote AddressUseVisitor as an extension of AddressDefUseWalker, even though it's obviously backward, just to keep up the appearance of reuse. It was complex and bug-prone, and the results were invalid.

For now, I need to separate this new infrastructure from the code that affects the rest of the compiler. So redesigning AddressDefUseWalker should be a separate step. In fact, the separate list of opcodes is necessary until we want to change the behavior of existing passes.

Literally the only thing you want to reuse here is the list of address projection opcodes. The simple, obvious way to do that is with an AddressProjection abstraction. That would solve any short term problem. In general, forcing inheritance to reuse logic leads to incomprehensible, buggy code.

Longer term, the two utilities should be integrated. There are multiple problems with the current design and it's not possible to write a simple address use visitor on top of it. One thing I'll point out is that AddressDefUseWalker.leafUse is just outright wrong. It's called on just any unrecognizable use, and that's not suitable for utilities that are designed to be complete.

AddressDefUseWalker should be built on top of AddressUseVisitor. It should simply override projection to update the path and it should be responsible for the def-use walk itself, rather than just visiting the use. Then AddressUseVisitor won't have to do any walk and everything will make sense and work naturally.

The simple, obvious way to do that is with an AddressProjection abstraction.

AddressDefUseWalker is the address projection abstraction. Its implementation mainly consists of a switch over all address projection instructions and "translating" that to a projection path.

I'll use AddressLifetimeDefUseWalker for this. Later, I think the other one should be renamedAddressPathDefUseWalker or AddressProjectionDefUseWalker. We should still add an AddressProjection protocol.

eeckstein · 2024-01-15T14:33:15Z

SwiftCompilerSources/Sources/Optimizer/Utilities/OwnershipLiveness.swift

+         is ProjectBlockStorageInst, is TailAddrInst, is StoreBorrowInst,
+         is UncheckedAddrCastInst, is MarkUninitializedInst, is DropDeinitInst,
+         is TuplePackElementAddrInst, is CopyableToMoveOnlyWrapperAddrInst,
+         is MoveOnlyWrapperToCopyableAddrInst:


This is the case block which should be handled by AddressDefUseWalker. This implementation already contains unhealthy code duplication because it's easy to add instructions here and forget in AddressDefUseWalker or vice versa.

As mentioned above, we should have one list of address projections, and it definitely should not be buried inside a walker! But for now, the two implementations are intentionally not in sync with each other. SIL verification should come first.

SwiftCompilerSources/Sources/Optimizer/Utilities/OwnershipLiveness.swift

eeckstein · 2024-01-15T15:12:12Z

SwiftCompilerSources/Sources/Optimizer/Utilities/OwnershipLiveness.swift

+  /// result and implement this as a fatalError.
+  mutating func visitPointerEscape(use: Operand) -> WalkResult
+
+  /// Handle begin_borrow, load_borrow, store_borrow, begin_apply.


I'm completely confused by this comment.

eeckstein · 2024-01-15T15:31:27Z

SwiftCompilerSources/Sources/Optimizer/Utilities/OwnershipLiveness.swift

+  }
+}
+
+/// Enumerate all special cases of ownership uses in a visitor


It's not clear to me what OwnershipUseVisitor does or should do. Also this comment is not really useful.
On the one hand it appears to wrap a visitor around the OperandOwnership enum. On the other hand it looks like it's also doing some kind of def-use walk.
The confusion already starts with that I'm not sure what the terms "outer" and "inner" scope/liferange/value actually refer to.

OwnershipUseVisitor is the one place that define the effects of operand ownership. We need this so we don't end up with ad-hoc utilities in OptUtils like InstructionRange.insert(borrowScopeOf) which is out of place, incomplete, and likely misused.

We really need to keep those new utilities understandable and easy to use. Also, it's important to make it clear how utilities relate to each other and when to use which utility.

atrick · 2024-01-16T06:56:34Z

@swift-ci test

eeckstein · 2024-01-16T10:02:35Z

SwiftCompilerSources/Sources/Optimizer/Utilities/BorrowUtils.swift

+      return
+    }
+    // avoid duplicates in the enclosingValues set.
+    var pushedEnclosingValues = ValueSet(context)


You cannot create a ValueSet in a recursive function. It will run out of bit positions (for the set) pretty quickly.
Can you define pushedEnclosingValues as member property of EnclosingValues? IIUC, a single set for the whole gathering process would be good enough to avoid duplicate entries in the result.

Thanks, i meant to convert that ValueSet into a cache. That's done now.

eeckstein · 2024-01-16T16:09:40Z

SwiftCompilerSources/Sources/Optimizer/Utilities/BorrowUtils.swift

+// of which dominates the value and introduces a borrow scope that
+// encloses all forwarded uses of the guaranteed value.
+//
+//   %1 = begin_borrow %0                // borrow introducer for %3


-> borrow introducer for %2

eeckstein · 2024-01-16T16:14:32Z

SwiftCompilerSources/Sources/Optimizer/Utilities/BorrowUtils.swift

+//     %5 = owned value           
+//     %6 = begin_borrow %5       %6                   %5
+//     br bb3(%5, %6)             
+//   bb3(%phi: @owned,            invalid              %0


%0 -> none (?)

eeckstein · 2024-01-16T16:27:08Z

SwiftCompilerSources/Sources/Optimizer/Utilities/BorrowUtils.swift

+    switch self {
+    case .beginBorrow, .storeBorrow:
+      let svi = instruction as! SingleValueInstruction
+      return svi.uses.walk {


you can also write svi.uses.filterUsers(ofType: EndBorrowInst.self).walk ...

eeckstein · 2024-01-16T16:31:09Z

SwiftCompilerSources/Sources/Optimizer/Utilities/BorrowUtils.swift

+//     cond_br ..., bb1, bb2      
+//   bb1:                         
+//     %1 = owned value           
+//     %2 = begin_borrow %0       %2                   %1


%0 -> %1

eeckstein · 2024-01-16T16:36:50Z

SwiftCompilerSources/Sources/Optimizer/Utilities/BorrowUtils.swift

+//     %6 = begin_borrow %0       %6                   %0
+//     %7 = struct (%6)           %6                   %6
+//     br bb3(%6, %7)             
+//   bb3(%reborrow: @guaranteed,  %reborrow            %0


Aren't we already printing reborrows with a @reborrow attribute (instead of @guaranteed)?

That would be great. For some reason we still print @guaranteed. I'll change the comments to @reborrow to anticipate fixing this.

eeckstein · 2024-01-16T16:55:11Z

SwiftCompilerSources/Sources/Optimizer/Utilities/OwnershipLiveness.swift

+  let context: FunctionPassContext
+  var _context: Context { context }
+
+  let function: Function


you can do

var function: Function { definingValue.parentFunction }

eeckstein · 2024-01-16T16:57:44Z

SwiftCompilerSources/Sources/Optimizer/Utilities/OwnershipLiveness.swift

+///   %0 = begin_borrow %outerValue
+///   %1 = begin_borrow %0
+///   end_borrow %1        // inner "use point" of %0
+///   end_borrow %0        // outer use of %1


shouldn't this be an outer use of %0?

eeckstein · 2024-01-16T17:00:44Z

SwiftCompilerSources/Sources/Optimizer/Utilities/OwnershipLiveness.swift

+    }
+  }
+
+  /// Visit only those uses of an value within an inner borrow scope


atrick · 2024-01-17T08:01:34Z

@swift-ci test

eeckstein

lgtm!

eeckstein · 2024-01-17T14:14:17Z

SwiftCompilerSources/Sources/Optimizer/Utilities/BorrowUtils.swift

+    case let arg as Argument where arg.isReborrow:
+      self = .reborrow(Phi(arg)!)
+    default:
+      if value.definingInstruction is BeginApplyInst {


nit, in case you are pushing a new version anyway: you can write

case let mvir as MultipleValueInstructionResult where mvir.parentInstruction is BeginApplyInst:

Then you can also define the case payload as case beginApply(MultipleValueInstructionResult) which avoids the optional-nil-check when getting its parent instruction.

And rename `uses.lifetimeEndingUses` to `uses.endingLifetime`

Also fix the memory binding within stack access.

Format comments so they are readable on github and 80-col tools.

Key APIs necessary for using OSSA. - BorrowingInstruction - BeginBorrowValue - scopeEndingOperands - BorrowIntroducers - EnclosingValues - innerAdjacentPhis These need to be complete to be correct.

This is what you need to correctly analyze OSSA. - computeLinearLiveness - computeInteriorLiveness - InteriorUseVisitor - OwnershipUseVisitor - LivenessBoundary Along with BorrowUtils.swift, all of our OSSA transformations are built on top of these fundamentals. With these APIs, we can build anything OSSA-related in SwiftCompilerSources. These utilities are immediately needed for borrowed arguments and lifetime dependence. In the near future, we can also use them to complete OSSA lifetimes and *correctly* fixup OSSA after transformation without introducing lots of copies and creating lots of incorrect corner cases.

Along with motivating unit tests.

To occur in the order that people will likely want to read.

atrick · 2024-01-17T16:42:12Z

@swift-ci smoke test

atrick · 2024-01-17T17:07:34Z

@swift-ci smoke test

atrick · 2024-01-17T23:52:31Z

@swift-ci smoke test linux

atrick · 2024-01-17T23:53:22Z

@swift-ci smoke test windows

atrick · 2024-01-18T00:31:31Z

@swift-ci smoke test linux

atrick · 2024-01-18T00:31:42Z

@swift-ci smoke test windows

We have language features that depends on swift_in_compiler. Any platform that does not support this build feature does not really support the language, so we must continue to disable tests.

atrick · 2024-01-18T02:43:18Z

@swift-ci smoke test

atrick requested review from meg-gupta and nate-chandler January 12, 2024 18:51

atrick requested a review from eeckstein as a code owner January 12, 2024 18:51

atrick force-pushed the ownership-liveness branch from 42a7cb9 to b8df083 Compare January 13, 2024 02:12

atrick mentioned this pull request Jan 13, 2024

SwiftCompilerSources support for OSSA and lifetime dependence #70460

Closed

atrick force-pushed the ownership-liveness branch 2 times, most recently from c46e94a to 94d1b39 Compare January 13, 2024 04:01

atrick force-pushed the ownership-liveness branch from 94d1b39 to 68527d6 Compare January 15, 2024 01:41

eeckstein reviewed Jan 15, 2024

View reviewed changes

atrick force-pushed the ownership-liveness branch from 68527d6 to 7d19418 Compare January 16, 2024 06:56

eeckstein reviewed Jan 16, 2024

View reviewed changes

atrick force-pushed the ownership-liveness branch from 7d19418 to 320d798 Compare January 17, 2024 07:43

eeckstein approved these changes Jan 17, 2024

View reviewed changes

eeckstein reviewed Jan 17, 2024

View reviewed changes

atrick added 9 commits January 17, 2024 08:37

Add Swift SIL instructions needed by AddressUseVisitor

d8b9010

Add OperandOwnership.endsLifetime

f69dfe4

And rename `uses.lifetimeEndingUses` to `uses.endingLifetime`

Add Stack.Marker and Stack.Segment utilities.

87af731

Also fix the memory binding within stack access.

Add SingleInlineArray utility.

a756d38

ForwardingUtils comment formatting.

5759d42

Format comments so they are readable on github and 80-col tools.

ForwardingUtils.swift cleanup. Make the API easier to extend.

2cac9fb

Add BorrowUtils: OSSA infrastructure for borrow scopes.

1646a66

Key APIs necessary for using OSSA. - BorrowingInstruction - BeginBorrowValue - scopeEndingOperands - BorrowIntroducers - EnclosingValues - innerAdjacentPhis These need to be complete to be correct.

BorrowIntroducer unit tests

8a2ae0b

SwiftCompilerSources: add UseList ignoreTypeDependence

e4d8277

atrick added 8 commits January 17, 2024 08:37

Ownership liveness in Swift unit tests.

ea31471

AddressUseVisitor

9a822b7

OwnershipUtils review feedback: Comments and examples.

d445754

OwnershipUtils review feedback: code formatting.

c10c545

Add a reborrow cache to EnclosingValues.

196ae17

Along with motivating unit tests.

Reorder code in OwnershipUtils.swift.

fa27b4b

To occur in the order that people will likely want to read.

Rename AddressUseVisitor to AddressLifetimeDefUseWalker.

b6b45f0

atrick force-pushed the ownership-liveness branch 2 times, most recently from 58f670d to 8aa6833 Compare January 17, 2024 16:40

atrick enabled auto-merge January 17, 2024 16:42

atrick added 2 commits January 17, 2024 09:07

Update SIL-Utilities.md for new ownership utils.

801000f

Update OwnershipLiveness comments for code review.

8341b73

atrick force-pushed the ownership-liveness branch from 8aa6833 to 8341b73 Compare January 17, 2024 17:07

Disable all new tests on Windows.

24f6ed2

We have language features that depends on swift_in_compiler. Any platform that does not support this build feature does not really support the language, so we must continue to disable tests.

atrick merged commit dd3f6ef into swiftlang:main Jan 18, 2024

SwiftCompilerSources for ownership, borrowing, and liveness #70887

SwiftCompilerSources for ownership, borrowing, and liveness #70887

Uh oh!

Conversation

atrick commented Jan 12, 2024

Uh oh!

atrick commented Jan 12, 2024

Uh oh!

atrick commented Jan 13, 2024

Uh oh!

atrick commented Jan 13, 2024

Uh oh!

atrick commented Jan 15, 2024

Uh oh!

eeckstein left a comment • 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!

Uh oh!

Uh oh!

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!

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!

eeckstein Jan 16, 2024 • 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!

atrick commented Jan 16, 2024

Uh oh!

eeckstein Jan 16, 2024 • 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!

Choose a reason for hiding this comment

Uh oh!

eeckstein left a comment •

edited

Loading

eeckstein Jan 16, 2024 •

edited

Loading

eeckstein Jan 16, 2024 •

edited

Loading

eeckstein Jan 17, 2024 •

edited

Loading