[MIR] Implement Inlining #39648

Aatch · 2017-02-08T17:35:18Z

Fairly basic implementation of inlining for MIR. Uses conservative
heuristics for inlining.

Doesn't handle a number of cases, but can be extended later. This is basically the same as the previous inlining PR, but without the span-related changes (as the bugs it was dealing with have since been fixed).

/cc @rust-lang/compiler

rust-highfive · 2017-02-08T17:35:30Z

r? @pnkfelix

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

nagisa · 2017-02-08T18:05:00Z

src/librustc/ty/util.rs

@@ -647,11 +647,12 @@ impl<'a, 'tcx> ty::TyS<'tcx> {
        }

        tcx.layout_depth.set(depth+1);


Perhaps it would make sense to have some sort of guard for this instead?

mrhota · 2017-02-15T05:17:03Z

src/librustc/mir/mod.rs

@@ -63,8 +64,7 @@ macro_rules! newtype_index {
 }

 /// Lowered representation of a single function.
-// Do not implement clone for Mir, which can be accidently done and kind of expensive.
-#[derive(RustcEncodable, RustcDecodable, Debug)]
+#[derive(Clone, RustcEncodable, RustcDecodable, Debug)]


I thought 'Clone' was removed for a reason. Is there a good reason for bringing it back?

It's required for the TypeFoldable impl.

mrhota · 2017-02-15T05:19:58Z

src/librustc/mir/mod.rs

+
+        match self.kind {
+            Assign(ref lval, ref rval) => { lval.visit_with(visitor) || rval.visit_with(visitor) }
+            SetDiscriminant { ref lvalue, .. } |


according to comments from the previous PR, listing fields and using _ is preferable to ..

mrhota · 2017-02-15T05:20:21Z

src/librustc/mir/mod.rs

+        use mir::TerminatorKind::*;
+
+        match self.kind {
+            If { ref cond, .. } => cond.visit_with(visitor),


mrhota · 2017-02-15T05:23:53Z

src/librustc/ty/mod.rs

@@ -2322,6 +2322,20 @@ impl<'a, 'gcx, 'tcx> TyCtxt<'a, 'gcx, 'tcx> {
        }).borrow()
    }

+    /// Given the DefId of an item, returns its MIR, borrowed immutably.
+    /// Returns None if there is no MIR for the DefId
+    pub fn maybe_item_mir(self, did: DefId) -> Option<Ref<'gcx, Mir<'gcx>>> {


can't you use item_mir?

Not all functions have a MIR available and item_mir panics if there isn't a MIR available for that def id.

mrhota · 2017-02-15T05:25:54Z

src/librustc_mir/transform/inline.rs

+            let def_id = callgraph.def_id(node);
+
+            // Don't inspect functions from other crates
+            let id = if let Some(id) = self.tcx.hir.as_local_node_id(def_id) {


wouldn't if !def_id.is_local() {} do the trick?

I need the node id anyway, this combines that check.

mrhota · 2017-02-15T05:27:55Z

src/librustc_mir/transform/inline.rs

+            };
+            let src = MirSource::from_node(self.tcx, id);
+            if let MirSource::Fn(_) = src {
+                if let Some(mir) = self.tcx.maybe_item_mir(def_id) {


we know at least that def_id is local, maybe maybe_ isn't necessary?

Being a local doesn't mean a MIR is available. The obvious case is intrinsics, since you can define those locally.

mrhota

Several comments from the original PR should probably be addressed here, like these.

The code lacks descriptions of approach or rationale. I get the impression (from the previous PR) that you intend for this code to be a kind of first strike at this problem. There are lots of details here, and it's hard to work out what's going on when there's not even a high-level description of how this pass works (or should work).

Moreover, it'd be nice to have tests verifying the efficacy of this new pass. Performance comparisons would be nice, too, since you mentioned improvements in the previous PR.

mrhota · 2017-02-19T17:27:01Z

src/librustc_driver/driver.rs

@@ -1031,6 +1031,7 @@ pub fn phase_4_translate_to_llvm<'a, 'tcx>(tcx: TyCtxt<'a, 'tcx, 'tcx>,
        passes.push_pass(box mir::transform::simplify::SimplifyCfg::new("elaborate-drops"));

        // No lifetime analysis based on borrowing can be done from here on out.
+        passes.push_pass(box mir::transform::inline::Inline);


Doesn't this invalidate the comment in line 1033?

That comment is about line 1031, i.e. nothing below the comment can invalidate it.

ok, I thought it might refer to what is now line 1035

mrhota · 2017-02-19T17:29:28Z

src/librustc_mir/callgraph.rs

+ *
+ * [1]: https://en.wikipedia.org/wiki/Tarjan%27s_strongly_connected_components_algorithm
+ */
+pub struct SCCIterator<'g> {


From comments in the previous version of this PR, this should be generified and moved into the graph algos module. Obviously, the same goes for StackElement.

eddyb

r=me with comments addressed (mine and others')

eddyb · 2017-02-21T20:53:40Z

src/librustc_driver/driver.rs

@@ -1031,6 +1031,7 @@ pub fn phase_4_translate_to_llvm<'a, 'tcx>(tcx: TyCtxt<'a, 'tcx, 'tcx>,
        passes.push_pass(box mir::transform::simplify::SimplifyCfg::new("elaborate-drops"));

        // No lifetime analysis based on borrowing can be done from here on out.
+        passes.push_pass(box mir::transform::inline::Inline);


That comment is about line 1031, i.e. nothing below the comment can invalidate it.

eddyb · 2017-02-21T20:54:37Z

src/librustc_mir/callgraph.rs

+            graph: graph::Graph::new()
+        };
+
+        for def_id in def_ids {


Need to filter by .is_local(), cross-crate MIR should always be inspected on-demand (and not by accident).

eddyb · 2017-02-21T20:56:23Z

src/librustc_mir/transform/inline.rs

+const INSTR_COST : usize = 5;
+const CALL_PENALTY : usize = 25;
+
+const UNKNOWN_SIZE_COST : usize = 10;


Pretty sure the accepted style doesn't have a space before :.

eddyb · 2017-02-21T20:57:49Z

src/librustc_mir/transform/inline.rs

+            let mut is_drop = false;
+            match term.kind {
+                TerminatorKind::Drop { ref location, target, unwind } |
+                TerminatorKind::DropAndReplace { ref location, target, unwind, .. } => {


You shouldn't be seeing DropAndReplace anymore at this point.

eddyb · 2017-02-21T20:59:22Z

src/librustc_mir/transform/inline.rs

+                targets.1 = self.update_target(targets.1);
+            }
+            TerminatorKind::Switch { ref mut targets, .. } |
+            TerminatorKind::SwitchInt { ref mut targets, .. } => {


Need to rebase I suppose.

eddyb · 2017-02-21T20:59:38Z

src/librustc_mir/transform/inline.rs

+                }
+            }
+            TerminatorKind::Drop { ref mut target, ref mut unwind, .. } |
+            TerminatorKind::DropAndReplace { ref mut target, ref mut unwind, .. } => {


Again, DropAndReplace shouldn't show up anymore at this point.

nikomatsakis · 2017-02-21T21:38:04Z

@mrhota

The code lacks descriptions of approach or rationale.

I agree this would make the code stronger. At the same time, I don't want to raise the bar too high for this patch -- I'm afraid it will bitrot like last time, and I think I'd rather have something in-tree with which we can iterate than nothing (particularly since it is not enabled by default). It feels like we need to get a series of quality benchmarks and start tuning these MIR optimizations, and documenting the results of that tuning. Trying to divine rules in advance without benchmarks feels risky.

bors · 2017-02-28T10:04:36Z

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

mrhota · 2017-03-01T19:56:26Z

@Aatch ping?

arielb1 · 2017-03-01T20:30:20Z

src/librustc_mir/transform/inline.rs

+                // `i : &mut usize`, then just duplicating the `a[*i]`
+                // Lvalue could result in two different locations if `f`
+                // writes to `i`. To prevent this we need to create a temporary
+                // borrow of the lvalue and pass the destination as `*temp` instead.


I thought the semantics was that any such *i is "borrow-locked" for the duration of the call, so any unexpected assignments during function call evaluation result in UB (certainly, MIR construction always copies indexes to a temporary).

nikomatsakis · 2017-03-02T18:13:23Z

Somebody ought to rebase this =)

mrhota · 2017-03-09T02:36:54Z

@Aatch ping again... 😦

Adds `get`/`get_mut` accessors and `drain`/`drain_enumerated` iterators to IndexVec. Implements TypeFoldable for IndexVec.

Fairly basic implementation of inlining for MIR. Uses conservative heuristics for inlining.

eddyb · 2017-03-10T02:09:53Z

@bors r+

bors · 2017-03-10T02:09:54Z

📌 Commit 3eb26d1 has been approved by eddyb

bors · 2017-03-10T07:32:19Z

⌛ Testing commit 3eb26d1 with merge 3123793...

bors · 2017-03-10T07:48:54Z

💔 Test failed - status-appveyor

eddyb · 2017-03-10T10:48:51Z

@bors retry

appveyor llvm build failed

bors · 2017-03-10T13:06:16Z

⌛ Testing commit 3eb26d1 with merge 1b53cd3...

bors · 2017-03-10T13:22:06Z

💔 Test failed - status-travis

eddyb · 2017-03-10T13:43:33Z

@bors retry

linking LLVM failed (presumably Spurious failures in sccache #40240)

[MIR] Implement Inlining Fairly basic implementation of inlining for MIR. Uses conservative heuristics for inlining. Doesn't handle a number of cases, but can be extended later. This is basically the same as the previous inlining PR, but without the span-related changes (as the bugs it was dealing with have since been fixed). /cc @rust-lang/compiler

bors · 2017-03-10T15:39:32Z

⌛ Testing commit 3eb26d1 with merge 38aa079...

alexcrichton · 2017-03-10T15:43:45Z

@bors: retry

(prioritizing rollup)

bors · 2017-03-10T15:44:02Z

⌛ Testing commit 3eb26d1 with merge b37de9c...

alexcrichton · 2017-03-10T15:50:19Z

@bors: retry

(prioritizing rollup)

[MIR] Implement Inlining Fairly basic implementation of inlining for MIR. Uses conservative heuristics for inlining. Doesn't handle a number of cases, but can be extended later. This is basically the same as the previous inlining PR, but without the span-related changes (as the bugs it was dealing with have since been fixed). /cc @rust-lang/compiler

bors · 2017-03-11T08:25:48Z

⌛ Testing commit 3eb26d1 with merge 8c72b76...

[MIR] Implement Inlining Fairly basic implementation of inlining for MIR. Uses conservative heuristics for inlining. Doesn't handle a number of cases, but can be extended later. This is basically the same as the previous inlining PR, but without the span-related changes (as the bugs it was dealing with have since been fixed). /cc @rust-lang/compiler

bors · 2017-03-11T10:29:05Z

☀️ Test successful - status-appveyor, status-travis
Approved by: eddyb
Pushing 8c72b76 to master...

rust-highfive assigned pnkfelix Feb 8, 2017

nagisa reviewed Feb 8, 2017

View reviewed changes

nikomatsakis self-requested a review February 12, 2017 11:50

mrhota reviewed Feb 15, 2017

View reviewed changes

mrhota reviewed Feb 21, 2017

View reviewed changes

eddyb requested changes Feb 21, 2017

View reviewed changes

arielb1 reviewed Mar 1, 2017

View reviewed changes

Aatch added 5 commits March 10, 2017 02:55

Add extra methods to IndexVec and implement TypeFoldable for it

d708a40

Adds `get`/`get_mut` accessors and `drain`/`drain_enumerated` iterators to IndexVec. Implements TypeFoldable for IndexVec.

Fix recursion depth counting in layout

540b52e

Initial implementation of inlining for MIR

71d0d92

Fairly basic implementation of inlining for MIR. Uses conservative heuristics for inlining.

Add dep-graph tasks where needed

f55e92b

Only run inlining if mir opts are enabled

3eb26d1

eddyb force-pushed the mir-inlining-2 branch from d1c0666 to 3eb26d1 Compare March 10, 2017 02:07

eddyb approved these changes Mar 10, 2017

View reviewed changes

alexcrichton mentioned this pull request Mar 10, 2017

Rollup of 36 pull requests #40418

Closed

alexcrichton mentioned this pull request Mar 10, 2017

Rollup of 35 pull requests #40420

Closed

bors merged commit 3eb26d1 into rust-lang:master Mar 11, 2017

bors mentioned this pull request Mar 11, 2017

Translate shims using MIR #39628

Merged

		@@ -647,11 +647,12 @@ impl<'a, 'tcx> ty::TyS<'tcx> {
		}

		tcx.layout_depth.set(depth+1);

[MIR] Implement Inlining #39648

[MIR] Implement Inlining #39648

Conversation

Aatch commented Feb 8, 2017

rust-highfive commented Feb 8, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Aatch Feb 16, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mrhota left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eddyb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nikomatsakis commented Feb 21, 2017

bors commented Feb 28, 2017

mrhota commented Mar 1, 2017

Choose a reason for hiding this comment

nikomatsakis commented Mar 2, 2017

mrhota commented Mar 9, 2017

eddyb commented Mar 10, 2017

bors commented Mar 10, 2017

bors commented Mar 10, 2017

bors commented Mar 10, 2017

eddyb commented Mar 10, 2017

bors commented Mar 10, 2017

bors commented Mar 10, 2017

eddyb commented Mar 10, 2017 • edited by alexcrichton Loading

bors commented Mar 10, 2017

alexcrichton commented Mar 10, 2017

bors commented Mar 10, 2017

alexcrichton commented Mar 10, 2017

bors commented Mar 11, 2017

bors commented Mar 11, 2017

Aatch Feb 16, 2017 •

edited

Loading

eddyb commented Mar 10, 2017 •

edited by alexcrichton

Loading