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

Graphviz debug output for generic dataflow analysis #64828

Merged
merged 6 commits into from
Oct 1, 2019

Conversation

ecstatic-morse
Copy link
Contributor

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

A follow up to #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, 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).

@rust-highfive
Copy link
Collaborator

r? @zackmdavis

(rust_highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Sep 27, 2019
@ecstatic-morse
Copy link
Contributor Author

r? @oli-obk

@rust-highfive rust-highfive assigned oli-obk and unassigned zackmdavis Sep 27, 2019
@ecstatic-morse
Copy link
Contributor Author

ecstatic-morse commented Sep 27, 2019

Oh, this should only get merged after #64470 does, since this changes the signature of dataflow::generic::Engine::new.

@eddyb
Copy link
Member

eddyb commented Sep 27, 2019

I think it would be neat to use debug_set for more things, including all of our set abstractions, and this.

Instead of (_1,_2), you'd get {_1, _2} which looks more like a set to me, at least.

@ecstatic-morse
Copy link
Contributor Author

ecstatic-morse commented Sep 27, 2019

The only reason I didn't go with debug_set was to get rid of the space after the comma. I'm considering changing to the hex representation (14-ef) when the cardinality crosses a certain threshold, which would make this less of an issue. I also thought about printing the full set on entry and exit but only printing the diff for each statement, but this is kind of a pain if you want to know the state in the middle of a longer basic block.

@ecstatic-morse
Copy link
Contributor Author

I switched to only rendering the diff for each statement but printing the full dataset at block entry and exit. This makes it a lot easier to track changes to the dataflow state when there are a lot of elements, but makes it more difficult to quickly see the whole state at a given statement. I also improved the formatting a bit. Here's the new on the same input file:

prettier

This particular analysis never clears dataflow bits, but those would appear in red prefixed with a minus.

@bors
Copy link
Contributor

bors commented Sep 30, 2019

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

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.

just some nits

src/librustc_mir/dataflow/generic.rs Show resolved Hide resolved
src/librustc_mir/dataflow/generic.rs Show resolved Hide resolved
@ecstatic-morse
Copy link
Contributor Author

ecstatic-morse commented Sep 30, 2019

I've fixed merge conflicts and cleaned up the code a bit. I also added an extra row to the table for each basic block, so that both the diff for the terminator and the full state after the terminator's transfer function is applied are displayed. I think this is ready to go.

#![feature(rustc_attrs)]
#![feature(const_if_match)]

#[rustc_mir(borrowck_graphviz_postflow="test.dot")]
const _: Option<&std::cell::Cell<i32>> = if false {
    Some(&std::cell::Cell::new(5))
} else {
    None
};

Old table:
diagram

New table:
prettiest

@oli-obk
Copy link
Contributor

oli-obk commented Sep 30, 2019

@bors r+

@bors
Copy link
Contributor

bors commented Sep 30, 2019

📌 Commit 2b8e023 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 30, 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`).
bors added a commit that referenced this pull request Oct 1, 2019
Rollup of 9 pull requests

Successful merges:

 - #64377 (Add long error explanation for E0493)
 - #64786 (Use https for curl when building for linux)
 - #64828 (Graphviz debug output for generic dataflow analysis)
 - #64838 (Add long error explanation for E0550)
 - #64891 (Fix `vec![x; n]` with null raw fat pointer zeroing the pointer metadata)
 - #64893 (Zero-initialize `vec![None; n]` for `Option<&T>`, `Option<&mut T>` and `Option<Box<T>>`)
 - #64911 (Fixed a misleading documentation issue #64844)
 - #64921 (Add test for issue-64662)
 - #64923 (Add missing links for mem::needs_drop)

Failed merges:

 - #64918 (Add long error explanation for E0551)

r? @ghost
@bors bors merged commit 2b8e023 into rust-lang:master Oct 1, 2019
tmandry added a commit to tmandry/rust that referenced this pull request Oct 2, 2019
…ra, r=petrochenkov

Fix zebra-striping in generic dataflow visualization

A small formatting improvement to rust-lang#64828.

Prior to this, the background color of the first row of the table for each basic block changed seemingly at random. You can see this in [basic block rust-lang#5](rust-lang#64828 (comment)) under "New table". Now it is always light.

This also updates the example table to match the current output.
@ecstatic-morse ecstatic-morse deleted the generic-dataflow-graphviz 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