[WIP][MIR] Generic lattice-based DF framework #33628

nagisa · May 14, 2016

This PR at its current state has only the implementation of forward dataflow analysis and a accompanying alias-constant-simplify propagation pass.

The lattice-based dataflow framework is greatly inspired by the Hoopl and its paper. The framework besides just being yet another dataflow engine allows for interleaving transformations and analysis + composition of the passes. These are something the gen-kill-set-based dataflow engines aren’t quite suited for. Those features are showcased by the accompanying ACS pass and without them the ACS pass wouldn‘t be otherwise possible (at least not within 200 lines).

Some notes: this is not yet complete: I expect to expand the framework to gain some more useful combinators (or at least those described in the paper) as well as some more other niceties to simplify production of dataflow-reliant passes and backwards analysis capabilities and so on and on.

Some results:

For following pieces of code this MIR (with dead variables removed by yours truly) gets produced:

fn a(x: (bool, bool)) -> u32{
    let z = x.0;
    let v = x.1;
    let y = if z || v {
        10
    } else {
        20
    };
    y
}

fn a(arg0: (bool, bool)) -> u32 {
    let var0: (bool, bool); // x in Scope(1) at test.rs:1:6: 1:7
    let var1: bool; // z in Scope(3) at test.rs:2:9: 2:10
    let var2: bool; // v in Scope(7) at test.rs:3:9: 3:10
    let var3: u32; // y in Scope(11) at test.rs:4:9: 4:10
    let mut tmp0: bool;
    let mut tmp1: bool;
    let mut tmp2: bool;
    let mut tmp3: bool;
    let mut tmp4: bool;
    let mut tmp5: u32;

    bb0: {
        if((arg0.0: bool)) -> [true: bb1, false: bb3]; // Scope(14) at test.rs:4:16: 4:22
    }

    bb1: {
        goto -> bb4; // Scope(14) at test.rs:4:16: 4:22
    }

    bb2: {
        goto -> bb4; // Scope(14) at test.rs:4:16: 4:22
    }

    bb3: {
        if((arg0.1: bool)) -> [true: bb1, false: bb2]; // Scope(14) at test.rs:4:16: 4:22
    }

    bb4: {
        if(tmp2) -> [true: bb5, false: bb6]; // Scope(13) at test.rs:4:13: 8:6
    }

    bb5: {
        var3 = const 10u32; // Scope(19) at test.rs:5:9: 5:11
        goto -> bb7; // Scope(13) at test.rs:4:13: 8:6
    }

    bb6: {
        var3 = const 20u32; // Scope(23) at test.rs:7:9: 7:11
        goto -> bb7; // Scope(13) at test.rs:4:13: 8:6
    }

    bb7: {
        return = var3; // Scope(24) at test.rs:9:5: 9:6
        goto -> bb8; // Scope(0) at test.rs:1:1: 10:2
    }

    bb8: {
        return; // Scope(0) at test.rs:1:1: 10:2
    }

fn a() -> u32{
    let x = false;
    let z = x;
    let y = if z {
        10
    } else {
        10
    };
    y
}

fn a() -> u32 {
    let var0: bool; // x in Scope(3) at test.rs:2:9: 2:10
    let var1: bool; // z in Scope(6) at test.rs:3:9: 3:10
    let var2: u32; // y in Scope(9) at test.rs:4:9: 4:10
    let mut tmp0: bool;
    let mut tmp1: bool;
    let mut tmp2: u32;

    bb0: {
        goto -> bb2; // Scope(11) at test.rs:4:13: 8:6
    }

    bb1: {
        var2 = const 10u32; // Scope(14) at test.rs:5:9: 5:11
        goto -> bb3; // Scope(11) at test.rs:4:13: 8:6
    }

    bb2: {
        goto -> bb3; // Scope(11) at test.rs:4:13: 8:6
    }

    bb3: {
        return = const 10u32; // Scope(19) at test.rs:9:5: 9:6
        goto -> bb4; // Scope(0) at test.rs:1:1: 10:2
    }

    bb4: {
        return; // Scope(0) at test.rs:1:1: 10:2
    }
}

rust-highfive · May 14, 2016

r? @Aatch

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

bors · May 14, 2016

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

gereeter · May 14, 2016

gereeter reviewed May 14, 2016

View changes

gereeter · May 14, 2016

src/librustc/mir/transform/dataflow.rs

+                    StatementChange::None => StatementChange::Statement(ns),
+                    x => x
+                },
+            StatementChange::Statements(nss) => {


I think that, in the case of multiple statements, just transforming each statement doesn't work - the fact needs to be transferred across each statement. For example, an (admittedly silly) transformation that takes x = y to newtmp = y; x = newtmp should not mess up constant propogation - if y is constant, it should infer that newtmp is the same constant and fill in the value for x.

In Hoopl, this is solved by having rewrite functions be more like lists of rewrite functions - thenFwdRw doesn't actually run one rewrite after another, instead doing the first rewrite and then scheduling the second rewrite to occur next. Thus, the final transfer and rewrite engine is responsible for running rewrites in sequence, and can transfer facts across newly transformed statements.

I think that, in the case of multiple statements, just transforming each statement doesn't work - the fact needs to be transferred across each statement.

That’s right, I was thinking about how I would approach it and that’s exactly why I didn’t include a way to replace a statement with a (sub-)graph but replacement with a list of statements fell through the cracks.

gereeter · May 14, 2016

gereeter reviewed May 14, 2016

View changes

gereeter · May 14, 2016

src/librustc_mir/transform/acs_propagate.rs

+                        self.1 = true;
+                        *lvalue = nlval.clone();
+                    }
+                    _ => {}


Nit: if let?

gereeter · May 14, 2016

gereeter reviewed May 14, 2016

View changes

gereeter · May 14, 2016

src/librustc_mir/transform/acs_propagate.rs

+impl<'a, 'tcx> MutVisitor<'tcx> for RewriteConstVisitor<'a, 'tcx> {
+    fn visit_operand(&mut self, op: &mut Operand<'tcx>) {
+        let repl = if let Operand::Consume(ref lval) = *op {
+            if let Some(&Either::Const(ref c)) = self.0.get(lval) {


Nit: self.0.get(lval).map(|&Either::Const(ref c)| c.clone())?

Er... Scratch that. This doesn't work. I don't know what I was thinking.

gereeter · May 14, 2016

gereeter reviewed May 14, 2016

View changes

gereeter · May 14, 2016

src/librustc_mir/transform/acs_propagate.rs

+//! For all of them we will be using a lattice of Hashmap from Lvalue to
+//! WTop<Either<Lvalue, Constant>>
+//!
+//! My personal believ is that it should be possible to make a way to compose two hashmap lattices


Nit: belief

gereeter · May 14, 2016

gereeter reviewed May 14, 2016

View changes

gereeter · May 14, 2016

src/librustc_driver/driver.rs

@@ -939,7 +939,8 @@ pub fn phase_3_run_analysis_passes<'tcx, F, R>(sess: &'tcx Session,
            passes.push_pass(box mir::transform::remove_dead_blocks::RemoveDeadBlocks);
            passes.push_pass(box mir::transform::qualify_consts::QualifyAndPromoteConstants);
            passes.push_pass(box mir::transform::type_check::TypeckMir);
-            passes.push_pass(box mir::transform::simplify_cfg::SimplifyCfg);
+            passes.push_pass(box mir::transform::acs_propagate::ACSPropagate);
+            // passes.push_pass(box mir::transform::simplify_cfg::SimplifyCfg);


Note: this should be uncommented before merging.

gereeter · May 14, 2016

gereeter reviewed May 14, 2016

View changes

gereeter · May 14, 2016

src/librustc/mir/transform/dataflow.rs

+}
+
+/// Analyse and rewrite using dataflow in the forward direction
+pub fn ar_forward<'tcx, T, P>(cfg: &CFG<'tcx>, fs: Facts<P::Lattice>, mut queue: BitVector)


Could ar_forward just generate queue itself instead of taking the queue as an argument?

gereeter · May 14, 2016

gereeter reviewed May 14, 2016

View changes

gereeter · May 14, 2016

src/librustc/mir/transform/dataflow.rs

+
+impl<F: Lattice> ::std::ops::IndexMut<BasicBlock> for Facts<F> {
+    fn index_mut(&mut self, index: BasicBlock) -> &mut F {
+        if let None = self.0.get_mut(index.index()) {


Nit: self.0.get_mut(index.index()).is_none()

arielb1 · May 14, 2016

arielb1 reviewed May 14, 2016

View changes

arielb1 · May 14, 2016

src/librustc_mir/transform/acs_propagate.rs

+//! WTop<Either<Lvalue, Constant>>
+//!
+//! My personal believ is that it should be possible to make a way to compose two hashmap lattices
+//! into one, but I can’t seem to get it just right yet, so we do the composing and decomposing


Does this pass handle aliased memory writes? How?

I do not think this a problem for this pass as it is currently implemented. Most notably this pass does not propagate through Rvalue::Ref, only through Rvalue::Use nor does it replace the destination lvalues. These both being considered, the pass should not cause any problems related to aliased memory that do not already exist in the original MIR AFAICT.

Is there any particular examples you have in mind such pass should watch out for?

gereeter · May 15, 2016

gereeter reviewed May 15, 2016

View changes

gereeter · May 15, 2016

src/librustc/mir/transform/dataflow.rs

+use mir::transform::lattice::Lattice;
+
+
+pub trait DataflowPass<'tcx> {


I'm not sure that DataflowPass is pulling its weight. ar_forward already takes two parameters, one for the analysis and one for the pass. If DataflowPass was removed and ar_forward took two parameters, one for analysis, and one for rewriting, that would require less boilerplate while being just as ergonomic. Moreover, I don't think it really makes sense to tie the transfer function to the rewrite - as an example, no rewrite cares how alias results are generated, but there are many different alias analyses out there, so rewrites should not be tied to whatever alias analysis is used.

This would require that Transfer have its Lattice as an associated type instead of a parameter, but I can't see an example where it makes sense to implement Transfer for different lattices.

gereeter · May 15, 2016

gereeter reviewed May 15, 2016

View changes

gereeter · May 15, 2016

src/librustc/mir/transform/dataflow.rs

+    /// that is, given some fact `fact` true before both the statement and relacement graph, and
+    /// a fact `fact2` which is true after the statement, the same `fact2` must be true after the
+    /// replacement graph too.
+    fn stmt(&mir::Statement<'tcx>, &L, &mut CFG<'tcx>) -> StatementChange<'tcx>;


It might be nicer for these methods to take &self. This would allow runtime customization of what passes do. Additionally, it might make specifying compound rewrites more ergonomic by creating an and_then method on rewrite creating a RewriteAndThen. Thus,

impl<'tcx> DataflowPass<'tcx> for ACSPropagate { type Lattice = ACSLattice<'tcx>; type Rewrite = RewriteAndThen<'tcx, AliasRewrite, RewriteAndThen<'tcx, ConstRewrite, SimplifyRewrite>>; type Transfer = ACSPropagateTransfer; } ar_forward::<ACSPropagateTransfer, ACSPropagate>(&mut mir.cfg, Facts::new(), q);

would turn into

ar_forward( &mut mir.cfg, Facts::new(), q, ACSPropogateTransfer, AliasRewrite.and_then(ConstRewrite).and_then(SimplifyRewrite) );

This would require that ar_forward take its rewrite function and analysis by value instead of by type.

nagisa · May 15, 2016

Note for everybody looking at this: my exam session begins this week and lasts through the 2nd part of june. I’m pretty sure this PR won’t progress at all before the session ends.

All the reviews are appreciated and I’ll look at and try to respond to them in as timely manner as possible.

pnkfelix · May 16, 2016

Cc me

nagisa · May 16, 2016

Note to self: do not forget about nagisa#1

nikomatsakis · May 17, 2016

cc @rust-lang/compiler (I for one didn't see this due to lack of notification :)

nikomatsakis · May 17, 2016

@nagisa good luck on exams :)

dotdash · May 19, 2016

dotdash reviewed May 19, 2016

View changes

dotdash · May 19, 2016

src/librustc_mir/transform/simplify_cfg.rs

@@ -118,12 +83,11 @@ impl<'tcx> MirPass<'tcx> for SimplifyCfg {
        while changed {
            pretty::dump_mir(tcx, "simplify_cfg", &counter, src, mir, None);
            counter += 1;
-            changed = self.simplify_branches(mir);
            changed |= self.remove_goto_chains(mir);


This is an endless loop now, as changed is never set to false.

Yes that's why I commented the pass out.
On May 19, 2016 3:39 PM, "Björn Steinbrink" notifications@github.com
wrote:

In src/librustc_mir/transform/simplify_cfg.rs
#33628 (comment):

@@ -118,12 +83,11 @@ impl<'tcx> MirPass<'tcx> for SimplifyCfg {
while changed {
pretty::dump_mir(tcx, "simplify_cfg", &counter, src, mir, None);
counter += 1;

changed = self.simplify_branches(mir); changed |= self.remove_goto_chains(mir);

This is an endless loop now, as changed is never set to false.

—
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
https://github.com/rust-lang/rust/pull/33628/files/ee4a7c39e84237756fd453356eaec0921356873f#r63869926

rust-highfive assigned Aatch May 14, 2016

nagisa unassigned Aatch May 14, 2016

gereeter referenced this pull request Jun 8, 2016
Closed
[WIP] [MIR] Generic lattice-based dataflow framework, rebased #34164

nagisa closed this Jun 12, 2016

nagisa referenced this pull request Aug 11, 2016
Closed
[MIR] Dataflow framework, constant propagation and dead code elimination #35608

rust-lang/rust

Join GitHub today

[WIP][MIR] Generic lattice-based DF framework #33628

Conversation

nagisa commented May 14, 2016

nagisa added some commits May 14, 2016

rust-highfive assigned Aatch May 14, 2016

nagisa unassigned Aatch May 14, 2016

This comment has been minimized.

rust-highfive commented May 14, 2016

This comment has been minimized.

bors commented May 14, 2016

gereeter reviewed May 14, 2016 View changes

This comment has been minimized.

This comment has been minimized.

gereeter reviewed May 14, 2016 View changes

This comment has been minimized.

gereeter reviewed May 14, 2016 View changes

This comment has been minimized.

This comment has been minimized.

gereeter reviewed May 14, 2016 View changes

This comment has been minimized.

gereeter reviewed May 14, 2016 View changes

This comment has been minimized.

gereeter reviewed May 14, 2016 View changes

This comment has been minimized.

gereeter reviewed May 14, 2016 View changes

This comment has been minimized.

arielb1 reviewed May 14, 2016 View changes

This comment has been minimized.

arielb1 May 14, 2016 • edited

This comment has been minimized.

nagisa May 15, 2016 • edited

gereeter reviewed May 15, 2016 View changes

This comment has been minimized.

This comment has been minimized.

gereeter reviewed May 15, 2016 View changes

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

nagisa commented May 15, 2016

This comment has been minimized.

pnkfelix commented May 16, 2016

This comment has been minimized.

nagisa commented May 16, 2016

This comment has been minimized.

nikomatsakis commented May 17, 2016

This comment has been minimized.

nikomatsakis commented May 17, 2016

dotdash reviewed May 19, 2016 View changes

This comment has been minimized.

This comment has been minimized.

gereeter referenced this pull request Jun 8, 2016

nagisa closed this Jun 12, 2016

nagisa referenced this pull request Aug 11, 2016

gereeter reviewed May 14, 2016

View changes

gereeter reviewed May 14, 2016

View changes

gereeter reviewed May 14, 2016

View changes

gereeter reviewed May 14, 2016

View changes

gereeter reviewed May 14, 2016

View changes

gereeter reviewed May 14, 2016

View changes

gereeter reviewed May 14, 2016

View changes

arielb1 reviewed May 14, 2016

View changes

arielb1 May 14, 2016 •

edited

nagisa May 15, 2016 •

edited

gereeter reviewed May 15, 2016

View changes

gereeter reviewed May 15, 2016

View changes

dotdash reviewed May 19, 2016

View changes