librustc_mir: Implement def-use chains and trivial copy propagation on MIR. #36388

pcwalton · 2016-09-10T19:46:16Z

This only supports trivial cases in which there is exactly one def and
one use.

Currently, some random unrelated MIR tests are failing, probably just because they haven't been updated.

eddyb · 2016-09-10T19:59:49Z

src/librustc/mir/repr.rs

@@ -699,6 +733,9 @@ pub enum StatementKind<'tcx> {

    /// End the current live range for the storage of the local.
    StorageDead(Lvalue<'tcx>),
+
+    /// No-op. Useful for deleting instructions without affecting statement indices.
+    Nop,


Potential point of disagreement (we already have at least one lazy patching mechanism which preserves indices).

What is that one?

It's in src/librustc_borrowck/borrowck/mir/patch.rs (because drop elaboration in there. IMO rustc_borrowck should be replaced with a MIR version that doesn't live in a rustc_borrowck crate but that's besides the point).

It doesn't have the ability to remove statements because it wasn't needed.
I'm not entirely clear on whether that functionality could be easily added. cc @arielb1

eddyb · 2016-09-10T20:18:04Z

LGTM except for the potentially contentious Nop introduction.

jonas-schievink · 2016-09-10T21:24:20Z

src/librustc_mir/transform/copy_prop.rs

+                        debug!("  Can't copy-propagate local: no uses");
+                        continue
+                    }
+                    if src_use_info.use_count() != 1 {


Putting the result of use_count() into a variable instead of calling it 3 times might be a good idea - it's O(n) to compute it

Maybe the candidate could be found with a single traversal?

I think the source local having exactly 1 mutation (i.e. your assignment) is more interesting than it having 1 use, because that implies the assignment dominates all uses (you might want to at least debug-assert that, because mem::uninitialized might violate it in the future).

BTW, you want to run this after drop elaboration, because that removes quite a few dominated drops (or at least add a dominated drop removal pass).

Also, I think that having the pivot/destination as the core of the iteration is more natural.

eddyb · 2016-09-10T21:29:59Z

cc @rust-lang/compiler

arielb1 · 2016-09-11T21:27:27Z

src/librustc/mir/visit.rs

+    }
+
+    /// Returns true if this lvalue context represents a mutation (def).
+    pub fn is_def(&self) -> bool {


I would prefer is_mutation rather than is_def for mutations. BTW, why isn't drop considered? Sounds like it could create some trouble.

Agreed, the name def really confused me (though I get why you chose it now). My preference would be is_mut(). And yes Drop seems like a mutation to me.

See e.g. http://www.cs.cmu.edu/afs/cs/academic/class/15745-s05/www/lectures/lect2.pdf — "def" seems to be the right term. @nikomatsakis, want to weigh in?

Sure. In SSA, mutations are defs ("FRU" updates, if needed). Tho drop is a mutation too.

I think the term "def" for a mutation is confusing in a non-SSA context, but whatever.

@pcwalton my feeling is that def is the right term, it's just overloaded. Without context, I interpreted it first as "definition" as in def_id, versus definition as in "assignment to a variable".

nikomatsakis

Overall, this looks great to me! It's simple, but we gotta start somewhere. =) I left a few nits with suggestions on comments and naming.

nikomatsakis · 2016-09-15T16:56:54Z

src/librustc/mir/visit.rs

    // Used as base for another lvalue, e.g. `x` in `x.y`
-    Projection,
+    Projection(Mutability),


A comment might be nice here -- what is this Mutability referring to exactly? Ideally a snippet of Rust code to make the distinction clear.

nikomatsakis · 2016-09-15T17:09:25Z

src/librustc_mir/def_use.rs

+        }
+    }
+
+    fn lvalue_mut_info<'a>(&'a mut self, lvalue: &Lvalue<'tcx>) -> Option<&'a mut Info<'tcx>> {


Nit: In these cases, I'd normally let elision do its thing instead of writing out 'a. But whatever.

nikomatsakis · 2016-09-15T17:15:22Z

src/librustc_mir/def_use.rs

+        &mut self.info[local]
+    }
+
+    fn mutate_uses<F>(&self, local: Local, mir: &mut Mir<'tcx>, mut callback: F)


This function could use a comment explaining what it does. Also, the term use seems to be somewhat overloaded, no? This doesn't just iterate over "uses" in the "def-use" sense but really any "mention" (to use @arielb1's term).

nikomatsakis · 2016-09-15T17:21:18Z

src/librustc_mir/def_use.rs

+    }
+}
+
+impl<'tcx> Visitor<'tcx> for DefUseAnalysis<'tcx> {


Nit: Personally, I would probably not make DefUseAnalysis implement Visitor directly, but rather have some wrapper that does it, and make the a method like record_uses_from_mir() that invokes the visitor. This just makes other code a bit more explicit vs seeing def_use.visit_mir(mir), which isn't so clear.

nikomatsakis · 2016-09-15T17:32:32Z

src/librustc_mir/transform/copy_prop.rs

+                let src_location;
+                let dest_local;
+                {
+                    // There must be exactly one use of the source used in a statement (not in a


Nit: It'd be great to have a little example of what pattern you are matching here, in terms of MIR. I am thinking of some comments like:

Look for patterns like

X = Y ... USE(X)

and replace with:

... USE(Y)

Here, the src_local would be Y. So we look for a variable Y that is used at most once, and that use is an assignment. (etc)

I find it particularly useful to link the names in the MIR examples to the variables in the Rust source doing the transform.

bors · 2016-09-16T17:46:57Z

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

nikomatsakis · 2016-09-17T01:51:34Z

BTW, r=me modulo nits. (In case that wasn't clear.)

(It doesn't seem like there have been any major objections raised, right?)

nikic · 2016-09-19T14:42:28Z

src/librustc/mir/repr.rs

+impl Location {
+    pub fn dominates(&self, other: &Location, dominators: &Dominators<BasicBlock>) -> bool {
+        if self.block == other.block {
+            self.statement_index < other.statement_index


As the method name is dominates, should this be <=?

Seems correct, yeah.

This is useful when passes want to remove statements without affecting `Location`s.

This only supports trivial cases in which there is exactly one def and one use.

pcwalton · 2016-09-20T02:19:40Z

Updated. All tests pass locally. This should be ready to go.

Please see the fix at 480287e#diff-033ea10cb9e9d0a0b2eb95445ea25502R125.

r? @nikomatsakis

nikomatsakis · 2016-09-20T13:27:09Z

@bors r+

bors · 2016-09-20T13:27:10Z

📌 Commit 480287e has been approved by nikomatsakis

bors · 2016-09-20T15:01:02Z

⌛ Testing commit 480287e with merge c772948...

@eddyb

librustc_mir: Implement def-use chains and trivial copy propagation on MIR. This only supports trivial cases in which there is exactly one def and one use. Currently, some random unrelated MIR tests are failing, probably just because they haven't been updated. r? @eddyb

bors · 2016-09-20T21:41:29Z

rust-highfive assigned eddyb Sep 10, 2016

pcwalton force-pushed the copy-propagation branch 2 times, most recently from 156deff to 4cd0bc9 Compare September 10, 2016 19:49

eddyb reviewed Sep 10, 2016
View reviewed changes

jonas-schievink reviewed Sep 10, 2016
View reviewed changes

arielb1 reviewed Sep 11, 2016
View reviewed changes

pcwalton changed the title ~~Implement def-use chains and trivial copy propagation on MIR.~~ librustc_mir: Implement def-use chains and trivial copy propagation on MIR. Sep 15, 2016

pcwalton force-pushed the copy-propagation branch 2 times, most recently from b489231 to 9173e9e Compare September 16, 2016 01:20

nikomatsakis approved these changes Sep 16, 2016

View reviewed changes

pcwalton force-pushed the copy-propagation branch from 9173e9e to bfb6a04 Compare September 17, 2016 01:35

nikic reviewed Sep 19, 2016

View reviewed changes

librustc: Add a new nop statement to the MIR.

2e6a918

This is useful when passes want to remove statements without affecting `Location`s.

pcwalton force-pushed the copy-propagation branch 2 times, most recently from 2eec6e1 to fb2fb68 Compare September 20, 2016 02:16

librustc: Implement def-use chains and trivial copy propagation on MIR.

480287e

This only supports trivial cases in which there is exactly one def and one use.

pcwalton force-pushed the copy-propagation branch from fb2fb68 to 480287e Compare September 20, 2016 02:18

rust-highfive assigned nikomatsakis and unassigned eddyb Sep 20, 2016

bors merged commit 480287e into rust-lang:master Sep 20, 2016

This was referenced Sep 20, 2016

Rollup of 5 pull requests #36552

Closed

[MIR] Initial implementation of inlining #36593

Closed

pcwalton deleted the copy-propagation branch September 20, 2016 22:08

nagisa mentioned this pull request Mar 15, 2017

LLVM memcpyopt #40553

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

librustc_mir: Implement def-use chains and trivial copy propagation on MIR. #36388

librustc_mir: Implement def-use chains and trivial copy propagation on MIR. #36388

pcwalton commented Sep 10, 2016

eddyb Sep 10, 2016

pcwalton Sep 10, 2016

eddyb Sep 10, 2016

eddyb commented Sep 10, 2016

jonas-schievink Sep 10, 2016

eddyb Sep 10, 2016

arielb1 Sep 11, 2016 •

edited

Loading

eddyb commented Sep 10, 2016

arielb1 Sep 11, 2016 •

edited

Loading

nikomatsakis Sep 15, 2016 •

edited

Loading

pcwalton Sep 16, 2016

arielb1 Sep 16, 2016 •

edited

Loading

nikomatsakis Sep 16, 2016

nikomatsakis left a comment

nikomatsakis Sep 15, 2016 •

edited

Loading

nikomatsakis Sep 15, 2016

nikomatsakis Sep 15, 2016 •

edited

Loading

nikomatsakis Sep 15, 2016

nikomatsakis Sep 15, 2016 •

edited

Loading

bors commented Sep 16, 2016

nikomatsakis commented Sep 17, 2016

nikic Sep 19, 2016

nikomatsakis Sep 19, 2016

pcwalton commented Sep 20, 2016

nikomatsakis commented Sep 20, 2016

bors commented Sep 20, 2016

bors commented Sep 20, 2016

bors commented Sep 20, 2016

librustc_mir: Implement def-use chains and trivial copy propagation on MIR. #36388

librustc_mir: Implement def-use chains and trivial copy propagation on MIR. #36388

Conversation

pcwalton commented Sep 10, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eddyb commented Sep 10, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arielb1 Sep 11, 2016 • edited Loading

Choose a reason for hiding this comment

eddyb commented Sep 10, 2016

arielb1 Sep 11, 2016 • edited Loading

Choose a reason for hiding this comment

nikomatsakis Sep 15, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arielb1 Sep 16, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nikomatsakis left a comment

Choose a reason for hiding this comment

nikomatsakis Sep 15, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nikomatsakis Sep 15, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nikomatsakis Sep 15, 2016 • edited Loading

Choose a reason for hiding this comment

bors commented Sep 16, 2016

nikomatsakis commented Sep 17, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pcwalton commented Sep 20, 2016

nikomatsakis commented Sep 20, 2016

bors commented Sep 20, 2016

bors commented Sep 20, 2016

bors commented Sep 20, 2016

arielb1 Sep 11, 2016 •

edited

Loading

arielb1 Sep 11, 2016 •

edited

Loading

nikomatsakis Sep 15, 2016 •

edited

Loading

arielb1 Sep 16, 2016 •

edited

Loading

nikomatsakis Sep 15, 2016 •

edited

Loading

nikomatsakis Sep 15, 2016 •

edited

Loading

nikomatsakis Sep 15, 2016 •

edited

Loading