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 more generic interface for dataflow analysis #64566

Merged
merged 10 commits into from
Sep 19, 2019

Conversation

ecstatic-morse
Copy link
Contributor

@ecstatic-morse ecstatic-morse commented Sep 18, 2019

#64470 requires a transfer function that is slightly more complex than the typical gen/kill one. Namely, it must copy state bits between locals when assignments occur (see #62547 for an attempt to make this fit into the existing framework). This PR contains a dataflow interface that allows for arbitrary transfer functions. The trade-off is efficiency: we can no longer coalesce transfer functions for blocks and must visit each statement individually while iterating to fixpoint.

Another issue is that poorly behaved transfer functions can result in an analysis that fails to converge. gen/kill sets do not have this problem. I believe that, in order to guarantee convergence, flipping a bit from false to true in the entry set cannot cause an output bit to go from true to false (negate all preceding booleans when true is the bottom value). Perhaps someone with a more formal background can confirm and we can add a section to the docs?

This approach is not maximally generic: it still requires that the lattice used for analysis is the powerset of values of Analysis::Idx for the mir::Body of interest. This can be done at a later date. Also, this is the bare minimum to get #64470 working. I've not adapted the existing debug framework to work with the new analysis, so there are no rustc_peek tests either. I'm planning to do this after #64470 is merged.

Finally, my ultimate plan is to make the existing, gen/kill-based BitDenotation a special case of generic::Analysis. Currently they share a ton of code. I should be able to do this without changing any implementers of BitDenotation. Something like:

struct GenKillAnalysis<A: BitDenotation> {
    trans_for_block: IndexVec<BasicBlock, GenKillSet<A::Idx>>,
    analysis: A,
}

impl<A> generic::Analysis for GenKillAnalysis<A> {
    // specializations of `apply_{partial,whole}_block_effect`...
}

r? @pnkfelix

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Sep 18, 2019
@@ -30,6 +30,7 @@ use self::move_paths::MoveData;

Copy link
Contributor

@Centril Centril Sep 18, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

btw, as drive-by improvement potential, it would be great if this module had a brief explanation of what dataflow is and if it pointed to some good beginner->less-beginner resources for understanding dataflow.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ecstatic-morse Since @oli-obk r+ed, maybe this can be done as a follow up?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good. I tentatively plan to do this in the rustc guide, then link to it from dataflow/mod.rs.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's currently a "to be written" entry in the appendix

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds great. 👍

Copy link
Contributor

@oli-obk oli-obk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like it. This is much clearer (to me) than the existing dataflow framework.

src/librustc_mir/dataflow/generic.rs Outdated Show resolved Hide resolved
src/librustc_mir/dataflow/generic.rs Show resolved Hide resolved
src/librustc_mir/dataflow/generic.rs Outdated Show resolved Hide resolved
src/librustc_mir/dataflow/generic.rs Outdated Show resolved Hide resolved
src/librustc_mir/dataflow/generic.rs Show resolved Hide resolved
src/librustc_mir/dataflow/generic.rs Outdated Show resolved Hide resolved
@ecstatic-morse ecstatic-morse changed the title Add a more generic interface for dataflow analysis A more generic interface for dataflow analysis Sep 18, 2019
@oli-obk
Copy link
Contributor

oli-obk commented Sep 18, 2019

@bors r+

@bors
Copy link
Contributor

bors commented Sep 18, 2019

📌 Commit a7f5252 has been approved by oli-obk

@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 Sep 18, 2019
@ecstatic-morse
Copy link
Contributor Author

@oli-obk Can you r- and pick up the latest commit. There was a bug in the logic of seek_after_assume_call_return.

@ecstatic-morse
Copy link
Contributor Author

@bors r-

(worth a shot)

@bors bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Sep 18, 2019
@oli-obk
Copy link
Contributor

oli-obk commented Sep 18, 2019

@bors r+

@bors
Copy link
Contributor

bors commented Sep 18, 2019

📌 Commit b4e94d9 has been approved by oli-obk

@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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Sep 18, 2019
Centril added a commit to Centril/rust that referenced this pull request Sep 18, 2019
…oli-obk

A more generic interface for dataflow analysis

rust-lang#64470 requires a transfer function that is slightly more complex than the typical `gen`/`kill` one. Namely, it must copy state bits between locals when assignments occur (see rust-lang#62547 for an attempt to make this fit into the existing framework). This PR contains a dataflow interface that allows for arbitrary transfer functions. The trade-off is efficiency: we can no longer coalesce transfer functions for blocks and must visit each statement individually while iterating to fixpoint.

Another issue is that poorly behaved transfer functions can result in an analysis that fails to converge. `gen`/`kill` sets do not have this problem. I believe that, in order to guarantee convergence, flipping a bit from `false` to `true` in the entry set cannot cause an output bit to go from `true` to `false` (negate all preceding booleans when `true` is the bottom value). Perhaps someone with a more formal background can confirm and we can add a section to the docs?

This approach is not maximally generic: it still requires that the lattice used for analysis is the powerset of values of `Analysis::Idx` for the `mir::Body` of interest. This can be done at a later date. Also, this is the bare minimum to get rust-lang#64470 working. I've not adapted the existing debug framework to work with the new analysis, so there are no `rustc_peek` tests either. I'm planning to do this after rust-lang#64470 is merged.

Finally, my ultimate plan is to make the existing, `gen`/`kill`-based `BitDenotation` a special case of `generic::Analysis`. Currently they share a ton of code. I should be able to do this without changing any implementers of `BitDenotation`. Something like:

```rust
struct GenKillAnalysis<A: BitDenotation> {
    trans_for_block: IndexVec<BasicBlock, GenKillSet<A::Idx>>,
    analysis: A,
}

impl<A> generic::Analysis for GenKillAnalysis<A> {
    // specializations of `apply_{partial,whole}_block_effect`...
}
```

r? @pnkfelix
Centril added a commit to Centril/rust that referenced this pull request Sep 19, 2019
…oli-obk

A more generic interface for dataflow analysis

rust-lang#64470 requires a transfer function that is slightly more complex than the typical `gen`/`kill` one. Namely, it must copy state bits between locals when assignments occur (see rust-lang#62547 for an attempt to make this fit into the existing framework). This PR contains a dataflow interface that allows for arbitrary transfer functions. The trade-off is efficiency: we can no longer coalesce transfer functions for blocks and must visit each statement individually while iterating to fixpoint.

Another issue is that poorly behaved transfer functions can result in an analysis that fails to converge. `gen`/`kill` sets do not have this problem. I believe that, in order to guarantee convergence, flipping a bit from `false` to `true` in the entry set cannot cause an output bit to go from `true` to `false` (negate all preceding booleans when `true` is the bottom value). Perhaps someone with a more formal background can confirm and we can add a section to the docs?

This approach is not maximally generic: it still requires that the lattice used for analysis is the powerset of values of `Analysis::Idx` for the `mir::Body` of interest. This can be done at a later date. Also, this is the bare minimum to get rust-lang#64470 working. I've not adapted the existing debug framework to work with the new analysis, so there are no `rustc_peek` tests either. I'm planning to do this after rust-lang#64470 is merged.

Finally, my ultimate plan is to make the existing, `gen`/`kill`-based `BitDenotation` a special case of `generic::Analysis`. Currently they share a ton of code. I should be able to do this without changing any implementers of `BitDenotation`. Something like:

```rust
struct GenKillAnalysis<A: BitDenotation> {
    trans_for_block: IndexVec<BasicBlock, GenKillSet<A::Idx>>,
    analysis: A,
}

impl<A> generic::Analysis for GenKillAnalysis<A> {
    // specializations of `apply_{partial,whole}_block_effect`...
}
```

r? @pnkfelix
Centril added a commit to Centril/rust that referenced this pull request Sep 19, 2019
…oli-obk

A more generic interface for dataflow analysis

rust-lang#64470 requires a transfer function that is slightly more complex than the typical `gen`/`kill` one. Namely, it must copy state bits between locals when assignments occur (see rust-lang#62547 for an attempt to make this fit into the existing framework). This PR contains a dataflow interface that allows for arbitrary transfer functions. The trade-off is efficiency: we can no longer coalesce transfer functions for blocks and must visit each statement individually while iterating to fixpoint.

Another issue is that poorly behaved transfer functions can result in an analysis that fails to converge. `gen`/`kill` sets do not have this problem. I believe that, in order to guarantee convergence, flipping a bit from `false` to `true` in the entry set cannot cause an output bit to go from `true` to `false` (negate all preceding booleans when `true` is the bottom value). Perhaps someone with a more formal background can confirm and we can add a section to the docs?

This approach is not maximally generic: it still requires that the lattice used for analysis is the powerset of values of `Analysis::Idx` for the `mir::Body` of interest. This can be done at a later date. Also, this is the bare minimum to get rust-lang#64470 working. I've not adapted the existing debug framework to work with the new analysis, so there are no `rustc_peek` tests either. I'm planning to do this after rust-lang#64470 is merged.

Finally, my ultimate plan is to make the existing, `gen`/`kill`-based `BitDenotation` a special case of `generic::Analysis`. Currently they share a ton of code. I should be able to do this without changing any implementers of `BitDenotation`. Something like:

```rust
struct GenKillAnalysis<A: BitDenotation> {
    trans_for_block: IndexVec<BasicBlock, GenKillSet<A::Idx>>,
    analysis: A,
}

impl<A> generic::Analysis for GenKillAnalysis<A> {
    // specializations of `apply_{partial,whole}_block_effect`...
}
```

r? @pnkfelix
bors added a commit that referenced this pull request Sep 19, 2019
Rollup of 5 pull requests

Successful merges:

 - #63630 (Update installed compiler dependencies)
 - #64536 (Update Cargo)
 - #64554 (Polonius: more `ui` test suite fixes)
 - #64566 (A more generic interface for dataflow analysis)
 - #64591 (Fix a minor grammar nit, update UI tests)

Failed merges:

r? @ghost
@bors bors merged commit b4e94d9 into rust-lang:master Sep 19, 2019
tmandry added a commit to tmandry/rust that referenced this pull request Sep 30, 2019
…phviz, r=oli-obk

Graphviz debug output for generic dataflow analysis

A follow up to rust-lang#64566.

This outputs graphviz diagrams in the generic dataflow engine when `#[rustc_mir(borrowck_graphviz_postflow="suffix.dot")]` is set on an item. This code is based on [`dataflow/graphviz.rs`](https://github.com/rust-lang/rust/blob/master/src/librustc_mir/dataflow/graphviz.rs), but displays different information since there are no "gen"/"kill" sets and transfer functions cannot be coalesced in the generic analysis. As a result, we show the dataflow state at the start and end of each block, as well as the changes resulting from each statement. I also render the state bitset in full  (`{_1,_2}`) instead of hex-encoded as the current renderer does (`06`).
@ecstatic-morse ecstatic-morse deleted the generic-dataflow 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
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