Skip to content
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

Migrate borrowck dataflow impls to new framework #68241

Merged
merged 11 commits into from
Feb 12, 2020

Conversation

ecstatic-morse
Copy link
Contributor

@ecstatic-morse ecstatic-morse commented Jan 15, 2020

This uses #65672 to implement the dataflow analyses needed by borrowck. These include all the InitializedPlaces analyses as well as Borrows. Additionally, this PR contains several independent changes around the dataflow API which improve performance and make it more ergonomic.

  • An optimization that inhibits the caching of block transfer functions for acyclic MIR (~0.3% I-CNT savings).
  • A ResultsVisitor for dataflow results that is more efficient than ResultsCursor when we have to visit every statement unconditionally (~0.3% I-CNT savings).
  • An into_engine method on Analysis that selects the appropriate Engine constructor.
  • A contains method for ResultsCursor as a shorthand for .get().contains().
  • A find_descendants helper on MovePath that replaces has_any_child_of on the old FlowsAtLocation

These changes made porting the dataflow analyses much easier. Finally, this PR removes some now-unused code in dataflow/at_location.rs and elsewhere.

You can view the perf results for the final version of this PR here. Here's an example of the graphviz diagrams that are generated for the MaybeInitializedPlaces analysis.

image

@ecstatic-morse
Copy link
Contributor Author

@bors try
@rust-timer queue

@rust-timer

This comment has been minimized.

@bors

This comment has been minimized.

@bors

This comment has been minimized.

@rust-timer
Copy link
Collaborator

Queued 31ca0fd7b109756a7c52baff29870d7831f3e723 with parent 4b172cc, future comparison URL.

@ecstatic-morse
Copy link
Contributor Author

Now without caching block transfer functions on acyclic MIR.

@bors try
@rust-timer queue

@rust-timer

This comment has been minimized.

@bors

This comment has been minimized.

@rust-highfive

This comment has been minimized.

@ecstatic-morse

This comment has been minimized.

@ecstatic-morse

This comment has been minimized.

@rust-timer

This comment has been minimized.

@bors

This comment has been minimized.

@rust-highfive

This comment has been minimized.

@bors

This comment has been minimized.

@rust-timer
Copy link
Collaborator

Queued b685c41b5ae4fe1476ec71023ad348b57781105c with parent 3291ae3, future comparison URL.

@rust-timer
Copy link
Collaborator

Finished benchmarking try commit b685c41b5ae4fe1476ec71023ad348b57781105c, comparison URL.

@ecstatic-morse
Copy link
Contributor Author

ecstatic-morse commented Jan 20, 2020

Once more but using a Visitor instead of a ResultsCursor. This should indicate whether ResultsCursor has meaningful overhead. I suspect it does not, but I'm happy to be proved wrong.

@bors try
@rust-timer queue

@rust-timer

This comment has been minimized.

@bors

This comment has been minimized.

@bors bors added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Jan 20, 2020
@rust-timer

This comment has been minimized.

@pnkfelix
Copy link
Member

Okay I'm done with the review.

I had some nits, but they are mostly about clarifying the commit messages or the commit history, along with a few small stylistic issues; so the fixes should be easy.

r=me after fixes are made.

@ecstatic-morse
Copy link
Contributor Author

@pnkfelix I believe all of your concerns have been addressed and I have put the mutate_place for TerminatorKind::Yield in the correct place. Feel free to look at the new comment on Analysis::into_engine, but in the meantime:

@bors r=pnkfelix

@bors
Copy link
Contributor

bors commented Feb 11, 2020

📌 Commit 5f40fe9 has been approved by pnkfelix

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Feb 11, 2020
@ecstatic-morse
Copy link
Contributor Author

@bors rollup=never

Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this pull request Feb 12, 2020
…ls, r=pnkfelix

Migrate borrowck dataflow impls to new framework

This uses rust-lang#65672 to implement the dataflow analyses needed by borrowck. These include all the `InitializedPlaces` analyses as well as `Borrows`. Additionally, this PR contains several independent changes around the dataflow API which improve performance and make it more ergonomic.

* An optimization that inhibits the caching of block transfer functions for acyclic MIR (~0.3% I-CNT savings).
* A `ResultsVisitor` for dataflow results that is more efficient than `ResultsCursor` when we have to visit every statement unconditionally (~0.3% I-CNT savings).
* An `into_engine` method on `Analysis` that selects the appropriate `Engine` constructor.
* A `contains` method for `ResultsCursor` as a shorthand for `.get().contains()`.
* A `find_descendants` helper on `MovePath` that replaces `has_any_child_of` on the old `FlowsAtLocation`

These changes made porting the dataflow analyses much easier. Finally, this PR removes some now-unused code in `dataflow/at_location.rs` and elsewhere.

You can view the perf results for the final version of this PR [here](https://perf.rust-lang.org/compare.html?start=29b854fb741809c29764e33fc17c32ba9c6523ba&end=6e516c1410c18cfe4eb6d030a39fdb73c8d8a4fe). Here's an example of the graphviz diagrams that are generated for the `MaybeInitializedPlaces` analysis.

![image](https://user-images.githubusercontent.com/29463364/72846117-c3e97d80-3c54-11ea-8171-3d48981c9ddd.png)
@Dylan-DPC-zz
Copy link

@bors p=1

@bors
Copy link
Contributor

bors commented Feb 12, 2020

⌛ Testing commit 5f40fe9 with merge 2ed25f0...

bors added a commit that referenced this pull request Feb 12, 2020
Migrate borrowck dataflow impls to new framework

This uses #65672 to implement the dataflow analyses needed by borrowck. These include all the `InitializedPlaces` analyses as well as `Borrows`. Additionally, this PR contains several independent changes around the dataflow API which improve performance and make it more ergonomic.

* An optimization that inhibits the caching of block transfer functions for acyclic MIR (~0.3% I-CNT savings).
* A `ResultsVisitor` for dataflow results that is more efficient than `ResultsCursor` when we have to visit every statement unconditionally (~0.3% I-CNT savings).
* An `into_engine` method on `Analysis` that selects the appropriate `Engine` constructor.
* A `contains` method for `ResultsCursor` as a shorthand for `.get().contains()`.
* A `find_descendants` helper on `MovePath` that replaces `has_any_child_of` on the old `FlowsAtLocation`

These changes made porting the dataflow analyses much easier. Finally, this PR removes some now-unused code in `dataflow/at_location.rs` and elsewhere.

You can view the perf results for the final version of this PR [here](https://perf.rust-lang.org/compare.html?start=29b854fb741809c29764e33fc17c32ba9c6523ba&end=6e516c1410c18cfe4eb6d030a39fdb73c8d8a4fe). Here's an example of the graphviz diagrams that are generated for the `MaybeInitializedPlaces` analysis.

![image](https://user-images.githubusercontent.com/29463364/72846117-c3e97d80-3c54-11ea-8171-3d48981c9ddd.png)
@bors
Copy link
Contributor

bors commented Feb 12, 2020

☀️ Test successful - checks-azure
Approved by: pnkfelix
Pushing 2ed25f0 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Feb 12, 2020
@bors bors merged commit 5f40fe9 into rust-lang:master Feb 12, 2020
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this pull request Feb 15, 2020
…ls2, r=eddyb

Use `ResultsCursor` for `elaborate_drops`

Some cleanup after rust-lang#68241. The old code was using a custom cursor-like struct called `InitializationData`.
bors added a commit that referenced this pull request Feb 16, 2020
Use `ResultsCursor` for `elaborate_drops`

Some cleanup after #68241. The old code was using a custom cursor-like struct called `InitializationData`.
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).
bors added a commit that referenced this pull request Mar 1, 2020
…tmandry

Use new dataflow framework for generators

#65672 introduced a new dataflow framework that can handle arbitrarily complex transfer functions as well as ones expressed as a series of gen/kill operations. This PR ports the analyses used to implement generators to the new framework so that we can remove the old one. See #68241 for a prior example of this. The new framework has some superficial API changes, but this shouldn't alter the generator passes in any way.

r? @tmandry
@ecstatic-morse ecstatic-morse deleted the unified-dataflow-impls branch October 6, 2020 01:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants