5.9: [MoveOnlyAddressChecker] Fixed two use-before-def handlings. #66905
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
Ported versions of the unit tests that Andy added to pruned liveness to FieldSensitivePL. rdar://110862719
rdar://111118843
The multi-def algorithm is based off the single-def algorithm. However, it differs from it in needing to handle "liveness holes"--uses before defs. When identifying blocks which are originally live, the algorithm starts from consuming blocks and walks backwards until a condition is met. Previously, that condition was finding blocks which were originally live. That makes sense in the single-def case: if a block is originally live, either it was already walked through or else it was one of the blocks discovered by liveness. In either case, its predecessors have already been visited if appropriate. However, in the multi-def case, this condition is too stringent. It fails to account for the possibility of a block with has a "liveness hole". Only stop the backwards walk if the predecessor not only (1) was in originally live but additionally (2) "kills liveness"--doesn't have a use before a def. rdar://111221183
During initializeLiveness, all blocks which contain destroys are to be recorded in consuming blocks. Previously, however, certain reinits were not being added. Here, all reinits are correctly added.
Contributor
Author
|
@swift-ci please test |
tbkka
approved these changes
Jun 26, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
Description: Fix two classes of miscompiles in the move-only address checker.
Back in #65000, Andrew added support for addreses (by way of handling for uses before defs) to PrunedLiveness. Here, the unit tests that Andrew wrote to verify that support is ported to FieldSensitivePrunedLiveness.
One of those unit tests revealed a bug in FieldSensitivePrunedLiveness's handling of uses before defs. That issue resulted in miscompiles for simple Swift programs featuring move-only values and loops; see the new test
Interpreter/moveonly_loop.swift. Here, that is fixed by replacing FieldSensitivePrunedLiveness' own attempt to handle uses-before-defs with a copy of the implementation from PrunedLiveness and a straightforward vectorization.Finally, fixing that bug in FieldSensitivePrunedLiveness revealed a bug in the multi-def lifetime extension for move-only values stored in addresses. The important difference between the algorithm used by the address checker and the algorithm used by the value checker is that the former is multi-def and so must handle uses-before-defs. The discovery of consumed blocks was properly handling that case, but the discovery of original live blocks was not. Here, that is fixed by walking through already discovered blocks if those blocks do not kill liveness.
Risk: Low. The first fix straightforwardly vectorizes a copy of the scalar code that determines whether there's a use before a def. The second fix amounts to using a variation of the normal way to compute liveness.
Scope: Narrow. This only affects move-only types.
Original PR: #66892
Reviewed By: Andrew Trick ( @atrick )
Testing: Added test that the move-only address checker previously miscompiled.
Resolves: rdar://110862719&111118843&111221183