Combine HaveBeenBorrowedLocals and IndirectlyMutableLocals into one dataflow analysis
#69113
Conversation
This file contains 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
This impl is temporary and will be removed along with the old dataflow framework. It allows us to reuse the transfer function of new dataflow analyses when defining old ones
`MaybeMutBorrowedLocals` serves the same purpose and has a better name.
This uses the new `MaybeMutBorrowedLocals` pass but we keep the `rustc_peek_indirectly_mutable` since the two are interchangable except when inspecting a local after it has been marked `StorageDead`.
It should have the same semantics as `HaveBeenBorrowedLocals`
|
r? @davidtwco (rust_highfive has picked a reviewer for you, use r? to override) |
rust-highfive
added
the
S-waiting-on-review
This comment has been minimized.
This comment has been minimized.
ecstatic-morse
force-pushed
the
unified-dataflow-borrowed
branch
from
e7d839d
to
0984639
Compare
This comment has been minimized.
This comment has been minimized.
|
r? @wesleywiser |
wesleywiser
reviewed
Feb 17, 2020
|
@ecstatic-morse Do you there there is the possibility for a compiler performance change here? Should we do a perf run before landing this? |
|
@wesleywiser It is unlikely. Switching from the old to the new framework for the "initialized places" analyses yielded very small gains, but the "borrowed" analyses are used only for const-checking and generators respectively. The perf queue is a bit full right now, otherwise I would do one just in case. Perhaps rollup=never is sufficient? |
|
@bors r+ rollup=never |
|
📌 Commit 077a93c has been approved by |
|
🌲 The tree is currently closed for pull requests below priority 100, this pull request will be tested once the tree is reopened |
bors
added
S-waiting-on-bors
|
@bors p=1 |
bors
added a commit
that referenced
this pull request
Feb 19, 2020
…sleywiser Combine `HaveBeenBorrowedLocals` and `IndirectlyMutableLocals` into one dataflow analysis This PR began as an attempt to port `HaveBeenBorrowedLocals` to the new dataflow framework (see #68241 for prior art). Along the way, I noticed that it could share most of its code with `IndirectlyMutableLocals` and then found a few bugs in the two analyses: - Neither one marked locals as borrowed after an `Rvalue::AddressOf`. - `IndirectlyMutableLocals` was missing a minor fix that `HaveBeenBorrowedLocals` got in #61069. This is not a problem today since it is only used during const-checking, where custom drop glue is forbidden. However, this may change some day. I decided to combine the two analyses so that they wouldn't diverge in the future while ensuring that they remain distinct types (called `MaybeBorrowedLocals` and `MaybeMutBorrowedLocals` to be consistent with the `Maybe{Un,}InitializedPlaces` naming scheme). I fixed the bugs and switched to exhaustive matching where possible to make them less likely in the future. Finally, I added comments explaining some of the finer points of the transfer function for these analyses (see #61069 and #65006).
|
☀️ Test successful - checks-azure |
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 PR began as an attempt to port
HaveBeenBorrowedLocalsto the new dataflow framework (see #68241 for prior art). Along the way, I noticed that it could share most of its code withIndirectlyMutableLocalsand then found a few bugs in the two analyses:Rvalue::AddressOf.IndirectlyMutableLocalswas missing a minor fix thatHaveBeenBorrowedLocalsgot in Make MIR drop terminators borrow the dropped location #61069. This is not a problem today since it is only used during const-checking, where custom drop glue is forbidden. However, this may change some day.I decided to combine the two analyses so that they wouldn't diverge in the future while ensuring that they remain distinct types (called
MaybeBorrowedLocalsandMaybeMutBorrowedLocalsto be consistent with theMaybe{Un,}InitializedPlacesnaming scheme). I fixed the bugs and switched to exhaustive matching where possible to make them less likely in the future. Finally, I added comments explaining some of the finer points of the transfer function for these analyses (see #61069 and #65006).