MIR graphviz prettification by solson · Pull Request #30602 · rust-lang/rust

solson · 2015-12-29T01:22:17Z

This PR changes most of the MIR graphviz debug output, making it smaller and more consistent. Also, it changes all fonts to monospace and adds a graph label containing the type of the fn the MIR is for and all the values (arguments, named bindings, and compiler temporaries).

I chose to re-write the graphviz output code instead of using the existing libgraphviz API because I found it much easier to prototype usage of various graphviz features when I had full control of the text output. It also makes the code simpler, I think.

Below are a bunch of example functions and links to their output images on the current nightly vs. this PR. File names starting with numbers (e.g. 80-factorial_fold-new.png) are for closures. There's still a bunch of low hanging fruit to make it even better, particularly around aggregates and references.

I also imagine the textual output for MIR will be able to closely match the graphviz output. The list of statements should look identical and the terminators will be the same except that the text form will have a list of target blocks (potentially using the same edge labels as the graphviz does). I can PR a simple text output right after this PR.

This is my first large change to the compiler, so if anything should be reorganized/renamed/etc, let me know! Also, feel free to bikeshed the details of the output, though any minor changes can come in future PRs.

#[rustc_mir(graphviz = "empty.dot")]fnempty(){}

http://vps.solson.me/mir-graphviz/empty-new.png
http://vps.solson.me/mir-graphviz/empty-old.png

#[rustc_mir(graphviz = "constant.dot")]fnconstant() -> i32{42}

http://vps.solson.me/mir-graphviz/constant-new.png
http://vps.solson.me/mir-graphviz/constant-old.png

#[rustc_mir(graphviz = "increment.dot")]fnincrement(x:i32) -> i32{
x + 1}

http://vps.solson.me/mir-graphviz/increment-new.png
http://vps.solson.me/mir-graphviz/increment-old.png

#[rustc_mir(graphviz = "factorial_recursive.dot")]fnfactorial_recursive(n:usize) -> usize{if n == 0{1}else{
n *factorial_recursive(n - 1)}}

http://vps.solson.me/mir-graphviz/factorial_recursive-new.png
http://vps.solson.me/mir-graphviz/factorial_recursive-old.png

#[rustc_mir(graphviz = "factorial_iterative.dot")]fnfactorial_iterative(n:usize) -> usize{letmut prod = 1;for x in1..n {
prod *= x;}
prod
}

http://vps.solson.me/mir-graphviz/factorial_iterative-new.png
http://vps.solson.me/mir-graphviz/factorial_iterative-old.png

#[rustc_mir(graphviz = "factorial_fold.dot")]fnfactorial_fold(n:usize) -> usize{(1..n).fold(1, |prod, x| prod * x)}

http://vps.solson.me/mir-graphviz/factorial_fold-new.png
http://vps.solson.me/mir-graphviz/factorial_fold-old.png
http://vps.solson.me/mir-graphviz/80-factorial_fold-new.png
http://vps.solson.me/mir-graphviz/80-factorial_fold-old.png

#[rustc_mir(graphviz = "collatz.dot")]fncollatz(mutn:usize){while n != 1{if n % 2 == 0{
n /= 2;}else{
n = 3* n + 1;}}}

http://vps.solson.me/mir-graphviz/collatz-new.png
http://vps.solson.me/mir-graphviz/collatz-old.png

#[rustc_mir(graphviz = "multi_switch.dot")]fnmulti_switch(n:usize) -> usize{match n {5 | 10 | 15 => 3,20 | 30 => 2,
_ => 1,}}

http://vps.solson.me/mir-graphviz/multi_switch-new.png
http://vps.solson.me/mir-graphviz/multi_switch-old.png

michaelwoerister · 2015-12-29T11:40:51Z

@tsion Very nice! I'd have to take a closer look if any vital information is missing now that was shown before, but from a first glance I like the new output a lot better. Much more readable :)

nagisa · 2015-12-29T16:10:40Z

Looks pretty good to me (both code and output). I have a minor concern that spelling out every branch (true/false/return/unwind) might introduce too much visual clutter, but I think it is fine to leave it as is for now and wait to see if it ends up being an issue.

I’d argue it still would be better to name Lvalues arg0, var0 and tmp0, so they are more self-documenting.

nagisa · 2015-12-29T16:15:17Z

Ah, also, I believe Adt, Tuple and Vec aggregates can easily be “prettified” as well. Adt is a struct or variant, and thus could be displayed as

Aggregate<StructPath { field1: value, field2: value2 }>,
Aggregate<EnumPath::Variant { field1: value, field2: value }>

and so on.

EDIT: note that it doesn’t really matter whether it is a struct or enum variant, what matters is the kind of Adt, i.e. is it struct-like (Path {…}), unit-like (Option::<T>::None) or tuple-like (Option::<T>::Some(t)). Tuple-like things cannot end up as Adt (currently) so you only need to handle struct-like and unit-like variants.

eddyb · 2015-12-29T17:16:01Z

The <> on Aggregate are a bit jarring, StructPath { field1: value, field2: value2 }, optionally prefixed by ADT or data would be fine, I think.

luqmana · 2015-12-29T21:53:00Z

👍 Nice! Looks great.

solson · 2015-12-30T01:38:01Z

@nagisa I think I agree about arg0, var0, and tmp0. I'm finding it hard to tell a0, v0, and t0 apart when quickly scanning some MIR, and the longer names are much more visually distinct.

For aggregates, I was planning to drop the whole Aggregate<> thing and pretty print every type of aggregate in its own way, but I left that for another PR.

nagisa · 2015-12-30T01:55:43Z

For aggregates, I was planning to drop the whole Aggregate<> thing and pretty print every type of aggregate in its own way, but I left that for another PR.

Sure, SGTM.

huonw · 2015-12-30T02:22:38Z

Maybe there should be (choosable) levels of verbosity in the output, e.g. I imagine the spans are usually not needed at all (except for closures, maybe).

eddyb · 2015-12-30T02:26:32Z

Ah, @huonw, that's a great idea - would be useful for types, i.e. being able to see them everywhere, like in LLVM IR, when you need that.
Even better if we could toggle the verbosity in SVG with CSS, but, alas, don't think graphviz is that sophisticated.

solson · 2015-12-30T02:28:10Z

@huonw Yeah, that's a good idea. I was mainly aiming to remove irrelevant details, e.g. between constant-old and constant-new, but we could have flags for adding spans and/or types back in.

solson · 2015-12-30T02:30:34Z

Another thought: we could show all integer constants with their type suffixes (e.g. 42i32), so showing all types would be necessary less often.

solson · 2015-12-30T02:34:16Z

I made the change from a/v/t to arg/var/tmp and updated the linked images.

Previously, all references to closure arguments went to the argument before the one they should (e.g. to `arg1` when it was supposed to go to `arg2`). This was because the MIR builder did not account for the implicit arguments that come before the explicit arguments, and closures have one implicit argument - the struct containing the captures. This is my test code and a diff of the MIR generated for the closure: ```rust let a = 2i32; let _f = |b: i32| -> i32 { a + b }: ``` ```diff --- old 2015-12-29 23:16:32.027926372 -0600 +++ new 2015-12-29 23:16:42.975400757 -0600 @@ -1,22 +1,22 @@ fn(arg0: &[closure@closure-args.rs:8:14: 8:39 a:&i32], arg1: i32) -> i32 { let var0: i32; // b let tmp0: (); let tmp1: i32; let tmp2: i32; bb0: { - var0 = arg0; + var0 = arg1; tmp1 = (*(*arg0).0); tmp2 = var0; ReturnPointer = Add(tmp1, tmp2); goto -> bb1; } bb1: { return; } bb2: { diverge; } } ``` (If you're wondering where this text MIR output comes from, it's from another branch of mine waiting on rust-lang#30602 to get merged.)

nikomatsakis · 2016-01-04T18:52:19Z

@bors r+

bors · 2016-01-04T18:52:20Z

📌 Commit 56343cd has been approved by nikomatsakis

nikomatsakis · 2016-01-04T18:52:26Z

pretty. :)

@nikomatsakis

r? @nikomatsakis cc @eddyb @nagisa This PR changes most of the MIR graphviz debug output, making it smaller and more consistent. Also, it changes all fonts to monospace and adds a graph label containing the type of the `fn` the MIR is for and all the values (arguments, named bindings, and compiler temporaries). I chose to re-write the graphviz output code instead of using the existing libgraphviz API because I found it much easier to prototype usage of various graphviz features when I had full control of the text output. It also makes the code simpler, I think. Below are a bunch of example functions and links to their output images on the current nightly vs. this PR. File names starting with numbers (e.g. `80-factorial_fold-new.png`) are for closures. There's still a bunch of low hanging fruit to make it even better, particularly around aggregates and references. I also imagine the textual output for MIR will be able to closely match the graphviz output. The list of statements should look identical and the terminators will be the same except that the text form will have a list of target blocks (potentially using the same edge labels as the graphviz does). I can PR a simple text output right after this PR. This is my first large change to the compiler, so if anything should be reorganized/renamed/etc, let me know! Also, feel free to bikeshed the details of the output, though any minor changes can come in future PRs. ```rust fn empty() {} ``` http://vps.solson.me/mir-graphviz/empty-new.png http://vps.solson.me/mir-graphviz/empty-old.png ```rust fn constant() -> i32 { 42 } ``` http://vps.solson.me/mir-graphviz/constant-new.png http://vps.solson.me/mir-graphviz/constant-old.png ```rust fn increment(x: i32) -> i32 { x + 1 } ``` http://vps.solson.me/mir-graphviz/increment-new.png http://vps.solson.me/mir-graphviz/increment-old.png ```rust fn factorial_recursive(n: usize) -> usize { if n == 0 { 1 } else { n * factorial_recursive(n - 1) } } ``` http://vps.solson.me/mir-graphviz/factorial_recursive-new.png http://vps.solson.me/mir-graphviz/factorial_recursive-old.png ```rust fn factorial_iterative(n: usize) -> usize { let mut prod = 1; for x in 1..n { prod *= x; } prod } ``` http://vps.solson.me/mir-graphviz/factorial_iterative-new.png http://vps.solson.me/mir-graphviz/factorial_iterative-old.png ```rust fn factorial_fold(n: usize) -> usize { (1..n).fold(1, |prod, x| prod * x) } ``` http://vps.solson.me/mir-graphviz/factorial_fold-new.png http://vps.solson.me/mir-graphviz/factorial_fold-old.png http://vps.solson.me/mir-graphviz/80-factorial_fold-new.png http://vps.solson.me/mir-graphviz/80-factorial_fold-old.png ```rust fn collatz(mut n: usize) { while n != 1 { if n % 2 == 0 { n /= 2; } else { n = 3 * n + 1; } } } ``` http://vps.solson.me/mir-graphviz/collatz-new.png http://vps.solson.me/mir-graphviz/collatz-old.png ```rust fn multi_switch(n: usize) -> usize { match n { 5 | 10 | 15 => 3, 20 | 30 => 2, _ => 1, } } ``` http://vps.solson.me/mir-graphviz/multi_switch-new.png http://vps.solson.me/mir-graphviz/multi_switch-old.png

bors · 2016-01-04T20:24:36Z

⌛ Testing commit 56343cd with merge badc23b...

bors · 2016-01-04T22:09:50Z

☀️ Test successful - auto-linux-32-nopt-t, auto-linux-32-opt, auto-linux-64-debug-opt, auto-linux-64-nopt-t, auto-linux-64-opt, auto-linux-64-x-android-t, auto-linux-cross-opt, auto-linux-musl-64-opt, auto-mac-32-opt, auto-mac-64-nopt-t, auto-mac-64-opt, auto-win-gnu-32-nopt-t, auto-win-gnu-32-opt, auto-win-gnu-64-nopt-t, auto-win-gnu-64-opt, auto-win-msvc-32-opt, auto-win-msvc-64-opt

solson added 2 commits December 28, 2015 17:17

Rewrite MIR graphviz printing and improve MIR debug printing.
9000ecf

Add comments and simplify MIR graphviz code.
e69713d

rust-highfive assigned nikomatsakis Dec 29, 2015

Use longer names for MIR variable names.
4b605e4

Fix MIR var names and keep them in sync.
31578f5

solson mentioned this pull request Dec 30, 2015
Fix argument indices in MIR for closures. #30630
Merged

Add 'mut' to temporary vars in MIR graphviz output.
56343cd

bors merged commit 56343cd into rust-lang:masterJan 4, 2016

bors mentioned this pull request Jan 4, 2016
[MIR] Refine representation and translation of calls #30481
Merged

solson deleted the mir-graphviz-display branch January 4, 2016 22:16

Uh oh!

Uh oh!

Conversation

solson commented Dec 29, 2015

Uh oh!

michaelwoerister commented Dec 29, 2015

Uh oh!

nagisa commented Dec 29, 2015

Uh oh!

nagisa commented Dec 29, 2015

Uh oh!

eddyb commented Dec 29, 2015

Uh oh!

luqmana commented Dec 29, 2015

Uh oh!

solson commented Dec 30, 2015

Uh oh!

nagisa commented Dec 30, 2015

Uh oh!

huonw commented Dec 30, 2015

Uh oh!

eddyb commented Dec 30, 2015

Uh oh!

solson commented Dec 30, 2015

Uh oh!

solson commented Dec 30, 2015

Uh oh!

solson commented Dec 30, 2015

Uh oh!

nikomatsakis commented Jan 4, 2016

Uh oh!

bors commented Jan 4, 2016

Uh oh!

nikomatsakis commented Jan 4, 2016

Uh oh!

bors commented Jan 4, 2016

Uh oh!

bors commented Jan 4, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants