SIL: Treat store_borrow as borrowing its source, and have the move-only checker account for borrow scopes. #69169
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 Please test |
gottesmm
reviewed
Oct 13, 2023
| auto *nextInst = inst->getNextInstruction(); | ||
| LLVM_DEBUG(llvm::dbgs() << "ending after momentary use\n"; |
There was a problem hiding this comment.
I think you forgot to delete code here.
gottesmm
approved these changes
Oct 13, 2023
| @@ -159,7 +167,7 @@ bool CanonicalizeOSSALifetime::computeCanonicalLiveness() { | |||
| continue; | |||
| } | |||
| } | |||
| switch (use->getOperandOwnership()) { | |||
| switch (auto ownership = use->getOperandOwnership()) { | |||
There was a problem hiding this comment.
Cool, I didn't know this pattern worked. Where are you using the value?
There was a problem hiding this comment.
I was going to use it in the debug message below, but I forgot to. I can remove it.
| LLVM_DEBUG(llvm::dbgs() << " Canonicalizing: " << currentDef); | ||
|
|
||
| if (currentDef->getOwnershipKind() != OwnershipKind::Owned) { | ||
| LLVM_DEBUG(llvm::dbgs() << " not owned, never mind\n"); |
There was a problem hiding this comment.
I think you need to git-clang-format the patch. This needs indentation.
atrick
added a commit
to atrick/swift
that referenced
this pull request
Oct 14, 2023
Function call arguments were not being treated as liveness uses. Unblocks SIL: Treat store_borrow as borrowing its source, and have the move-only checker account for borrow scopes. swiftlang#69169 swiftlang#69169
atrick
approved these changes
Oct 14, 2023
| // to extend the lifetime of a strong reference being assigned into a weak | ||
| // reference. | ||
| // | ||
| // REQUIRES: further_investigation |
There was a problem hiding this comment.
If you don't mind waiting for #69184 to land, you can keep the diagnose_lifetime_issues test enabled.
…ly checker account for borrow scopes. When rewriting uses of a noncopyable value, the move-only checker failed to take into account the scope of borrowing uses when establishing the final lifetimes of values. One way this manifested was when borrowed values get reabstracted from value to in-memory representations, using a store_borrow instruction, the lifetime of the original borrow would be ended immediately after the store_borrow begins rather than after the matching end_borrow. Fix this by, first, changing `store_borrow` to be treated as a borrowing use of its source rather than an interior-pointer use; this should be more accurate overall since `store_borrow` borrows the entire source value for a well-scoped duration balanced by `end_borrow` instructions. That done, change MoveOnlyBorrowToDestructureUtils so that when it sees a borrow use, it ends the borrow at the end(s) of the use's borrow scope, instead of immediately after the beginning of the use.
jckarter
force-pushed
the
store-borrow-lifetime-nesting
branch
from
8f5afb6 to
1dcf271
Compare
|
@swift-ci Please test |
jckarter
pushed a commit
to jckarter/swift
that referenced
this pull request
Oct 19, 2023
Function call arguments were not being treated as liveness uses. Unblocks SIL: Treat store_borrow as borrowing its source, and have the move-only checker account for borrow scopes. swiftlang#69169 swiftlang#69169
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.
When rewriting uses of a noncopyable value, the move-only checker failed to take into account the scope of borrowing uses when establishing the final lifetimes of values. One way this manifested was when borrowed values get reabstracted from value to in-memory representations, using a store_borrow instruction, the lifetime of the original borrow would be ended immediately after the store_borrow begins rather than after the matching end_borrow. Fix this by, first, changing
store_borrowto be treated as a borrowing use of its source rather than an interior-pointer use; this should be more accurate overall sincestore_borrowborrows the entire source value for a well-scoped duration balanced byend_borrowinstructions. That done, change MoveOnlyBorrowToDestructureUtils so that when it sees a borrow use, it ends the borrow at the end(s) of the use's borrow scope, instead of immediately after the beginning of the use.