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

A single framework for gen-kill and generic dataflow problems #65672

Merged
merged 11 commits into from
Jan 21, 2020

Conversation

ecstatic-morse
Copy link
Contributor

@ecstatic-morse ecstatic-morse commented Oct 21, 2019

This is the prototype implementation discussed in rust-lang/compiler-team#202. You can read a high-level description of it in the proposal for that design meeting. This would eventually supersede the existing BitDenotation interface.

r? @ghost

cc @rust-lang/compiler (esp. @eddyb and @pnkfelix)

@JohnCSimon JohnCSimon added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Oct 26, 2019
@bors
Copy link
Contributor

bors commented Oct 27, 2019

☔ The latest upstream changes (presumably #65869) made this pull request unmergeable. Please resolve the merge conflicts.

@hdhoang hdhoang added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Oct 31, 2019
@JohnCSimon
Copy link
Member

Ping from triage:
@ecstatic-morse Hi! is this PR still in progress?
thanks.

@JohnCSimon
Copy link
Member

Pinging from triage:
@ecstatic-morse Can you please provide status on this PR?
thanks.

@ecstatic-morse
Copy link
Contributor Author

ecstatic-morse commented Nov 11, 2019

This was discussed in the design meeting on Nov. 8 (rust-lang/compiler-team#202). The consensus was to move forward on a design similar to the prototype. I'll be actively working to finish this, hopefully with some input from the compiler team.

@ecstatic-morse ecstatic-morse marked this pull request as ready for review November 11, 2019 00:18
@ecstatic-morse ecstatic-morse force-pushed the unified-dataflow-proto branch 4 times, most recently from d6c3959 to df0d51b Compare November 13, 2019 01:35
@ecstatic-morse ecstatic-morse changed the title A single framework for gen-kill and generic dataflow problems [WIP] A single framework for gen-kill and generic dataflow problems Nov 13, 2019
@ecstatic-morse ecstatic-morse force-pushed the unified-dataflow-proto branch 2 times, most recently from 9ce9240 to 75775ba Compare November 14, 2019 04:19
@ecstatic-morse
Copy link
Contributor Author

@bors try @rust-timer queue

@rust-timer
Copy link
Collaborator

Awaiting bors try build completion

@bors
Copy link
Contributor

bors commented Nov 14, 2019

⌛ Trying commit 75775ba with merge 1e0b457...

bors added a commit that referenced this pull request Nov 14, 2019
[WIP] A single framework for gen-kill and generic dataflow problems

This is the prototype implementation discussed in rust-lang/compiler-team#202. You can read a high-level description of it in [the proposal](https://hackmd.io/@39Qr_z9cQhasi25sGjmFnA/Skvd9rztS) for that design meeting. This would eventually supersede the existing `BitDenotation` interface.

r? @ghost

cc @rust-lang/compiler (esp. @eddyb and @pnkfelix)
@bors
Copy link
Contributor

bors commented Nov 14, 2019

☀️ Try build successful - checks-azure
Build commit: 1e0b457 (1e0b457f588f21c0dfef20d076a5bc1371571aa1)

@rust-timer
Copy link
Collaborator

Queued 1e0b457 with parent 3f07f1c, future comparison URL.

@rust-timer
Copy link
Collaborator

Finished benchmarking try commit 1e0b457, comparison URL.

@ecstatic-morse
Copy link
Contributor Author

ecstatic-morse commented Nov 14, 2019

Okay, this now converts a commonly used dataflow analysis, MaybeInitializedLocals to the new framework to get an idea of usability/perf changes. The Engine implementation is simpler than the original since the choice of whether to cache block transfer functions is now dynamic. As a result, we can skip caching block transfer functions for acyclic MIR in the future.

I've triggered a perf run and the results look okay except for a slight regression in wg-grammar. I will try lifting the check for a cached block transfer function out of the loop to see if that helps. Regardless, I think this is ready for an initial round of review.

r? @pnkfelix cc @rust-lang/compiler (feel free to reassign)

Here are the unresolved "details" questions from the design meeting that are pertinent to this PR along with my thoughts on each. We should resolve these questions before merging.

  • How do we feel about the {before_,}*_effect naming convention? It takes a bit of getting used to, and doesn’t extend nicely to backwards dataflow.
    • OTOH, having a canonical statement_effect which most implementers want to use and a secondary, prefixed effect which is only rarely used is good.
  • Should we be passing a mir::Statement to the statement effect methods? What about mir::Body to bits_per_block?
    • I'd like to have a HasBody<'mir, 'tcx> trait that is a requirement for AnalysisDomain instead of passing in a separate body around everywhere, but this will increase the amount of boilerplate required.
  • Should the Analysis methods take &self or &mut self?
    • Some may depend on the results of an earlier dataflow analysis and want to hold a cursor.
    • However, transfer functions should be pure for a given MIR, so maybe forcing those analyses to use interior mutability is appropriate.

@ecstatic-morse
Copy link
Contributor Author

@bors try @rust-timer queue

@rust-timer
Copy link
Collaborator

Awaiting bors try build completion

@bors
Copy link
Contributor

bors commented Nov 15, 2019

⌛ Trying commit dceb654 with merge 43d649e...

bors added a commit that referenced this pull request Nov 15, 2019
[WIP] A single framework for gen-kill and generic dataflow problems

This is the prototype implementation discussed in rust-lang/compiler-team#202. You can read a high-level description of it in [the proposal](https://hackmd.io/@39Qr_z9cQhasi25sGjmFnA/Skvd9rztS) for that design meeting. This would eventually supersede the existing `BitDenotation` interface.

r? @ghost

cc @rust-lang/compiler (esp. @eddyb and @pnkfelix)
@bors
Copy link
Contributor

bors commented Nov 15, 2019

☀️ Try build successful - checks-azure
Build commit: 43d649e (43d649e19deb1597a734b28389fc636e8f1f2ad0)

@ecstatic-morse
Copy link
Contributor Author

ecstatic-morse commented Jan 15, 2020

I migrated the analyses used during borrow checking/drop elaboration to the new framework and perf seems good. I'm gonna try a few small optimizations I mentioned during the design meeting over on that PR as well.

While doing the conversion, it became apparent that code would much be nicer if we continued the "builder" style all the way back to Analysis by extending it with an into_engine method. For example, code like this:

let flow_inits = MaybeInitializedPlaces::new(tcx, &body, &mdpe);
let mut flow_inits = dataflow::generic::Engine::new_gen_kill(tcx, &body, def_id, flow_inits)
    .iterate_to_fixpoint()
    .into_cursor(&body);

could instead be written like this:

let mut flow_inits = MaybeInitializedPlaces::new(tcx, &body, &mdpe)
    .into_engine(tcx, &body, def_id)
    .iterate_to_fixpoint()
    .into_cursor(&body);

Another idea was to add HasBody as a supertrait of AnalysisDomain. This would obviate the need to pass body everywhere. With the current API, it's possible (albeit unlikely) for a user to pass a different mir::Body into, for example, into_engine than was used to create the analysis, which would cause subtle bugs. I believe this would only work if the Analysis methods continue to take &self, however.

I think it will take a few revisions to find the optimal API, and this PR has been sitting in the queue for a while, so I think we should not block it on making a decision re: the aforementioned changes. For now I'm focused on resolving any outstanding concerns of @pnkfelix.

@ecstatic-morse ecstatic-morse force-pushed the unified-dataflow-proto branch 2 times, most recently from 1b8a726 to d7a521d Compare January 20, 2020 03:07
@ecstatic-morse
Copy link
Contributor Author

@pnkfelix I addressed the motivation for the GenKill trait and added some documentation/renamed some methods in the last two commits.

bors added a commit that referenced this pull request Jan 20, 2020
[WIP] Migrate dataflow impls to new framework

This uses #65672 to implement the most commonly used dataflow analyses so that the performance of the new framework can be tuned. For now, it's just for perf runs.

r? @ghost
@pnkfelix
Copy link
Member

Thanks @ecstatic-morse !

@pnkfelix
Copy link
Member

@bors r+

@bors
Copy link
Contributor

bors commented Jan 21, 2020

📌 Commit 7b4dca2 has been approved by pnkfelix

@bors
Copy link
Contributor

bors commented Jan 21, 2020

⌛ Testing commit 7b4dca2 with merge 2cf24ab...

bors added a commit that referenced this pull request Jan 21, 2020
A single framework for gen-kill and generic dataflow problems

This is the prototype implementation discussed in rust-lang/compiler-team#202. You can read a high-level description of it in [the proposal](https://hackmd.io/@39Qr_z9cQhasi25sGjmFnA/Skvd9rztS) for that design meeting. This would eventually supersede the existing `BitDenotation` interface.

r? @ghost

cc @rust-lang/compiler (esp. @eddyb and @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 Jan 21, 2020
@bors
Copy link
Contributor

bors commented Jan 21, 2020

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

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Jan 21, 2020
@bors bors merged commit 7b4dca2 into rust-lang:master Jan 21, 2020
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)
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 added a commit to rust-lang/rust-clippy that referenced this pull request Feb 20, 2020
redundant_clone: Migrate to new dataflow framework

Migration to [the new dataflow framework](rust-lang/rust#65672) is ongoing in rustc. This PR updates the dataflow impl in `redundant_clone` lint.

---

changelog: none
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-proto 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. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants