Fix BorrowedFromInst operand ownership. #79605
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
@swift-ci test |
|
@swift-ci benchmark |
|
@swift-ci test source compatibility |
|
No benchmark effect |
eeckstein
approved these changes
Feb 25, 2025
SwiftCompilerSources/Sources/Optimizer/Utilities/BorrowUtils.swift
Outdated
Show resolved
Hide resolved
SwiftCompilerSources/Sources/Optimizer/Utilities/OwnershipLiveness.swift
Outdated
Show resolved
Hide resolved
It was too easy to confuse the "inner" vs. "outer" value. Use less ambigous names: visitAllUses vs visitInnerScopeUses
Extracted from the functional changes for clarity.
This instruction obviously borrows its base value. Liveness extends to the uses of the result. Code that switches on OperandOwnership assumes that an Instantaneous use does not propagate any information that may extend liveness.
All instructions with a "Borrow" operand ownership must be valid BorrowingInstructions.
This encourages the client to check whether InteriorLivenessResult is valid or escaping before using the instruction range.
If it is not a reborrow, then it produces a dependent value, just like mark_dependence.
At least the Swift utilities should rely on borrowed-from.
Only return false if the visitor returns false. Clients were ignoring the result. If the BorrowingOperand does not create a borrow scope, call visitUnknownUse instead. Until we have complete lifetimes, to avoid breaking code that cannot handle dead defs, consider a dead borrow scope to be an unknown use.
Check if the forwarded value is trivial, not the base value. Presumably, we won't call this visitor at all if the base is trivial. Record a dependent non-Escapable values as a pointer-escape. InteriorUseWalker does not handle lifetime dependencies. That requires LifetimeDependenceDefUseWalker.
Inner scopes must all be reported because the walker assumes they are complete. It is up to the client to either complete them or recursively follow them.
To handle borrowing operands that produce a dependence value but do not create a nested borrow scope. This includes non-reborrow borrowed-from and guaranteed mark_dependence [nonescaping].
It is buggy/misleading because of dead-def handling. The current operand may be re-pushed if it is a dead borrow scope.
borrowed-from instructions have different ownership semantics if the are attached to reborrows vs. guaranteed forwarding phis.
This API only makes sense for a scoped borrow-introducer such as: - reborrow - owned mark_dependence Borrowing operands that forward guaranteed values do not have scope-ending uses.
Along with a few similar mark_dependence cases.
The extra complexity for traversing phis is not needed now that it handles borrowed-from instructions. Remove the redundant logic because it complicates the liveness algorithm and generates confusing results.
The extra complexity for traversing phis is not needed now that it handles borrowed-from instructions. Remove the redundant logic because it complicates the liveness algorithm and generates confusing results.
This consolidates the rules for borrow scopes so we can simplify interior liveness.
Add liveness support for dependent values: borrowed-from & mark_dependence so they aren't reported as unknown uses.
Add liveness support for dependent values: borrowed-from & mark_dependence so they aren't reported as unknown uses. Centralize the logic in BorrowingOperand::getScopeIntroducingUserResult and BorrowingOperand::getDependentUserResult().
to handle borrowed-from (as a ForwardingInstruction).
atrick
force-pushed
the
fix-borrowedfrom
branch
from
3b99214 to
fc9a20f
Compare
Remove complexity around reborrows. They no longer need special treatment.
Add support to all kinds of borrowing instructions so we visit all their inner uses.
and rename visitOwnedDependentUses
Set the visitInnerUses flag. This is only a quick, partial fix. InteriorUseWalker does not generate complete liveness for two reasons 1. pointer escapes. The client must always check for escapes before assuming complete liveness. 2. dead end blocks. Until we have complete OSSA lifetimes, the algorithm for handling nested borrows is incorrect. The visitInnerUses flag works around this problem, but it isn't well tested and I'm not sure it's properly records escapes yet.
For borrowed-from of a reborrow, the value is the block argument. So we need to look through the borrowed-from user.
Always visit the use even if it is the beginning of an inner borrow. This provides a "better" liveness result in the case of dead borrows and gives the client a chance to see/intercept all uses.
atrick
force-pushed
the
fix-borrowedfrom
branch
from
33202a9 to
e23234e
Compare
A pointer escape means the liveness is incomplete. Don't hoist destroys with incomplete liveness.
interiorPointerUse is dealing with addresses, so the memory still needs to be alive regardless of the type.
DestroyHoisting is using this utility. This requires complete liveness. But we can't rely on complete liveness because complete lifetimes is not enabled. Instead, we use a visitInnerUses flag to try to ignore borrow scopes. That flag was never properly implemented. Make an attempt here.
atrick
force-pushed
the
fix-borrowedfrom
branch
from
e23234e to
6e75813
Compare
|
@eeckstein I added a few fixes to DestroyHoisting to unblock this PR and potentially unblock #79521. I haven't checked whether it fixes @nate-chandler's PR yet. Just speculating. |
|
@swift-ci test |
|
@swift-ci benchmark |
|
@swift-ci test source compatibility |
|
@swift-ci build toolchain |
|
@swift-ci test source compatibility |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is a pile of interrelated liveness fixes. It is actually safer to combine them into one PR so no one is exposed to intermediate partly-broken states. I noticed these issues when reviewing fixes for OSSA lifetime bugs and auditing the code. Begining to fix the problems exposed other problems. 37 commits later I reached a steady state.
The borrowed-from instruction borrows its enclosing value. Liveness extends to the uses of the result. However, it was marked as an InstantaneousUse. Code that switches on OperandOwnership assumes that an InstantaneousUse does not propagate any information that may extend liveness. So we were not getting correct liveness information.
Fix interior liveness to consistently handle inner borrows. Enforce rules for which borrowing operands create a new scope vs. those that forward guaranteed values. With both borrowed-from and mark_dependence, this depends on the context:
Handle borrowed-from and mark_dependence instructions of all flavors.
Follow dependent values to avoid unnecessary pointer escapes.
Conservatively report pointer escapes when they happen.
Consider the pointer escaping instruction also part of liveness.
Conservatively report a begin_borrow, or other borrowing instruction without any uses as an unknown use themselves. This hack will go away with complete OSSA.
Interior liveness is designed to be used with complete OSSA lifetimes. But we cannot rely on those yet. Meanwhile, passes like DestroyHoisting have started relying on complete liveness information. To workaround this problem, we need a special mode for interior liveness: visitInnerUses. This mode tries to find all the uses within inner borrow scopes. This mode is horribly complicated. The mode existed but was very incomplete and did not properly bail-out. Handle most of the important cases and add bail-outs where needed. Ensure the bail-outs are respected.
There are still plenty of FIXMEs and issues with this. But I fixed as much as I could.
Fix BorrowedValue::visitInteriorPointerOperandHelper to follow forwarding instructions.
Fix InteriorUseWalker.interiorPointerUse to follow address dependencies even when the addressable type is trivial.
These utilities need to behave the same way, or as close as possible, because passes that use the utilities are ported over without considering what's under the hood.
This made the internals of the utility very complicated with redundant logic. It was impossible to test support from borrowed-from.
Distinguish between scoped vs. unscoped borrowed-from instructions. They have different meaninings for ownership and liveness analysis.
--- C++
BorrowedFromInst::isReborrow
MarkDependenceInst::hasScopedLifetime
BorrowingOperand::getDependentUserResult()
--- Swift
MarkDependenceInst.hasScopedLifetime
BorrowingInstruction.scopedValue
BorrowingInstruction.dependentValue
BorrowingInstruction.innerValue
visit all interior uses
don't ignore pointer escapes.
Known Effects
fixes DestroyHoisting use-after frees
improves some optimizations that benefit from borrowed-from, like SILCombine's read-only CoW buffer optimization
CopyPropagation becomes more conservative with dead-borrows. But we should avoid those rather than adding complexity to handle them.