-
Notifications
You must be signed in to change notification settings - Fork 10.6k
[SemanticARCOpts] Fix miscompile at dead-ends. #83624
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
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
Put it alongsidde OperandToValue and friends.
For historical reasons, there was an API to check whether operands were within the boundary which just checked whether those operands' users were within the buondary. Make a copy of the method deal in instructions and factor the original API through it.
For historical reasons, the existing function (`areUsesWithinExtendedScope`) trafficked in operands rather than instructions. Now that PrunedLiveness has an API that deals with instructions, add a function (`areWithinExtendedScope`) which does as well. Factor the existing function through this new function.
The type is a union of an Operand (a real use) and a SILInstruction (an implicit use). Such a type is needed to reflect the fact that with incomplete lifetimes, values can be implicitly destroyed at the terminators of blocks in dead end regions (along the vaule's availability boundary).
Instead of tracking Operands, track UsePoints, i.e. `Either<Operand, SILInstruction>`s. For now, all use points wil remain operands. This change is necessary to allow tracking terminators in dead-end region as implicit uses.
It will be fixed in a subsequent commit.
With incomplete lifetimes, a value may have an "incomplete lifetime"--it may not be destroyed on paths into dead-end regions. When a value lacks a destroy on a path into a dead-end region, it is effectively destroyed at its availability boundary (e.g. an `unreachable` instruction). Indeed, lifetime completion amounts to making such implicit destroys explicit (with correct nesting). `SemanticARCOpts`' `performGuaranteedCopyValueOptimization` attempts to eliminate a `copy_value` of a guaranteed value. To do so, it computes the live range (`OwnershipLiveRange`) of the copy_value. Among other things, it checks that all destroys of the copy_value are within the scope of all guaranteed bases of the copied value. If any destroys are _not_ within any of those scopes, the copy cannot be eliminated (because the copy would have been originally live beyond the range which it would be live in after copy elimination). A value with an incomplete lifetime is implicitly destroyed at its availability boundary. So those implicit destroys along the availability boundary must _also_ be within the scopes of those guaranteed bases. Previously, implicit destroys along availability boundaries were not considered. This resulted in invalid shortening of lifetimes--before the transformation a value would be unconsumed up to its availability boundary; after the transformation it would be consumed somewhere before that. For example: ``` sil [ossa] @f : $@convention(thin) (@owned Outer) -> () { entry(%o : $*Outer): %o_borrow = begin_borrow %o : $Outer %i = struct_extract %o_borrow : $Outer, #Outer.inner %i_copy = copy_value %i : $Inner end_borrow %o_borrow : $Outer destroy_value %o : $Outer apply @g(%i_copy) : $@convention(thin) (@guaranteed Inner) -> () unreachable // %i_copy implicitly destroyed } ``` Here, `%i_copy` is implicitly destroyed at `unreachable` but `%i` is consumed at the scope end of `%o_borrow`. That means that an attempt to RAUW `%i_copy` with `%i` would be invalid because uses of `%i_copy` may be outside the live range of `%i`. (Note that this happens not to occur in the above example because there's a bailout in the case that there are no destroys, but perturbing the example to include a cond_br on one branch of which the value is destroyed results in the miscompile described above.) Here, this is fixed by adding the instructions on the availability boundary at which the value is implicitly destroyed to the live range. Do this by adding the instructions on the availability boundary of each def from which the live range is generated to the live range. One wrinkle is that the OwnershipLiveRange utility is used in one place by a utility when SIL is in an invalid state (phi uses have been replaced with undef so values leaks on those paths). Account for this by manually fixing up the liveness instance passed to the lifetime completion utility. rdar://157291161
7eb3759 to
c893c74
Compare
Contributor
Author
|
@swift-ci please test |
Contributor
Author
|
@swift-ci please apple silicon benchmark |
Contributor
Author
|
@swift-ci please 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.
Without complete OSSA lifetimes, a value may have an "incomplete lifetime"--it may not be destroyed on paths into dead-end regions. When a value lacks a destroy on a path into a dead-end region, it is effectively destroyed at its availability boundary (e.g. an
unreachableinstruction). Indeed, lifetime completion amounts to making such implicit destroys explicit (with correct nesting).SemanticARCOpts'performGuaranteedCopyValueOptimizationattempts to eliminate acopy_valueof a guaranteed value. To do so, it computes the live range (OwnershipLiveRange) of thecopy_value. Among other things, it checks that all destroys of thecopy_valueare within the scope of all guaranteed bases of the copied value. If any destroys are not within any of those scopes, the copy cannot be eliminated (because the copy would have been originally live beyond the range which it would be live in after copy elimination).A value with an incomplete lifetime is implicitly destroyed at its availability boundary. So those implicit destroys along the availability boundary must also be within the scopes of those guaranteed bases.
Previously, implicit destroys along availability boundaries were not considered. This resulted in invalid shortening of lifetimes--before the transformation a value would be unconsumed up to its availability boundary; after the transformation it would be consumed somewhere before that. For example:
Here,
%i_copyis implicitly destroyed atunreachablebut%iis consumed at the scope end of%o_borrow. That means that an attempt to RAUW%i_copywith%iwould be invalid because uses of%i_copymay be outside the live range of%i.(Note that this happens not to occur in the above example because there's a bailout in the case that there are no destroys, but perturbing the example to include a
cond_bron one branch of which the value is destroyed results in the miscompile described above.)Here, this is fixed by adding the instructions on the availability boundary at which the value is implicitly destroyed to the live range. Do this by adding the instructions on the availability boundary of each def from which the live range is generated to the live range.
One wrinkle is that the OwnershipLiveRange utility is used in one place by a utility when SIL is in an invalid state (phi uses have been replaced with undef so values leaks on those paths). Account for this by manually fixing up the liveness instance passed to the lifetime completion utility.
rdar://157291161