[MIR] Implement Inlining by Aatch · Pull Request #39648 · rust-lang/rust

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

Need to rebase I suppose.

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

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

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 suggested 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 3eb26d1Compare 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:masterMar 11, 2017

bors mentioned this pull request Mar 11, 2017
Translate shims using MIR #39628
Merged

Uh oh!

Uh oh!

Conversation

Aatch commented Feb 8, 2017

Uh oh!

rust-highfive commented Feb 8, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AatchFeb 16, 2017• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mrhota left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eddyb left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nikomatsakis commented Feb 21, 2017

Uh oh!

bors commented Feb 28, 2017

Uh oh!

mrhota commented Mar 1, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nikomatsakis commented Mar 2, 2017

Uh oh!

mrhota commented Mar 9, 2017

Uh oh!

eddyb commented Mar 10, 2017

Uh oh!

bors commented Mar 10, 2017

Uh oh!

bors commented Mar 10, 2017

Uh oh!

bors commented Mar 10, 2017

AatchFeb 16, 2017•
edited
Loading

eddyb commented Mar 10, 2017•
edited by alexcrichton
Loading