6.1: [OSSACanonicalizeOwned] Fix liveness passed to completion. #78674
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
The pass is structured to drain an instruction worklist and perform a sequence of operations on each popped instruction. Extract that sequence of operations into a new processInstruction function. Enables testing the sequence on a single instruction.
According to the documentation, ``` It's ok if postDomBlocks has duplicates or extraneous blocks, as long as they jointly post-dominate all live blocks that aren't on dead-end paths. ``` Previously, though, duplicates resulted in trapping: each element of `postDomBlocks` is pushed into a `BasicBlockWorklist`; when the second occurrence of an element in `postDomBlocks` was encountered, `BasicBlockWorklist::push` would trigger an assertion failure. Here, `pushIfNotVisited` is called instead.
To determine where a lifetime ends within dead-end blocks, OSSACanonicalizeOwned uses OSSACompleteLifetime's visitAvailabilityBoundary. This API takes a liveness which it uses to walk forward to the availability boundary. Specifically, the liveness passed from OSSACanonicalizeOwned to OSSACompleteLifetime is a variation of OSSACanonicalizeOwned's own liveness (it has destroys added). There is a mismatch in the characteristics of livenesses created by OSSACanonicalizeOwned and OSSACompleteLifetime: The former deals with not only direct uses of a value but also uses of its copies; that introduces the possibility for consuming uses in the middle of liveness. The latter on the other hand deals only with uses of a single value (nestedly, but at each level of nesting only a single value). Passing a liveness from the former to the latter without handling this mismatch is incorrect: OSSACompleteLifetime understands consuming uses to always end a lifetime, even when they are in the middle of a copy-extended liveness. The result is that OSSACompleteLifetime produces non-sensical results when provided with such a liveness. To address this, fixup the liveness passed from OSSACanonicalizeOwned to OSSACompleteLifetime by demoting consuming uses that appear within (that's to say _not_ on the boundary) of liveness to non-consuming uses. rdar://142846936
On main type annotations are not always required but on 6.1 they are.
|
@swift-ci please test |
tbkka
approved these changes
Jan 16, 2025
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.
Explanation: Fix a use-after-free on dead-end paths.
To ensure that it doesn't illegally hoist the destroys of any values across deinit barriers,
CopyPropagation'sOSSACanonicalizeOwnedutility must add to liveness instructions at the end of a value's lifetime on every path. Concretely, the final destroys of a value are added to liveness. Until values have complete lifetimes throughout the OSSA pipeline, however, a value need not have a final destroy on a dead-end path. The utility must still add something to liveness to avoid "hoisting" a destroy from "the end of the dead-end path" to somewhere above it. To find the appropriate instructions to add to liveness, the utility relies onOSSACompleteLifetimeto visit the availability boundary of the value; that boundary describes where the value's lifetime ends on dead-end paths (not always simply just beforeunreachableinstructions, say).OSSACompleteLifetimedepends on an instance of liveness for its work, andOSSACanonicalizeOwnedprovides it with a perturbed version of its own.OSSACanonicalizeOwnedoperates on a "copy-extended value".OSSACompleteLifetimeoperates on a standard OSSA value. The liveness produced byOSSACanonicalizeOwnedreflects that copy-extended-ness: it may contain consuming uses in the middle (corresponding to a consume of a copy of the base value in the middle).OSSACompleteLifetimerelies on its liveness being that for a standard OSSA value: it may not have consuming uses in the middle (that would correspond to a use-after-free). So there is a mismatch in the characteristics of the liveness produced by the former and used by the latter.Fix this by perturbing even more the copy of liveness passed from
OSSACanonicalizeOwnedtoOSSACompleteLifetime: demote consuming uses in the middle to non-consuming uses.Scope: Affects optimized code.
Issue: rdar://142846936
Original PR: #78624
Risk: Low.
Testing: Added tests.
Reviewer: Andrew Trick ( @atrick )