Don't rerun MIR passes when inlining #55244

wesleywiser · 2018-10-21T15:03:33Z

I updated your commit message with additional details. Let me know if any of that is incorrect. I also added the appropriate compile-flags directive to the test.

Thanks for you help on this!

cc @RalfJung related to your PR #55086

nikomatsakis · 2018-10-22T18:45:53Z

src/librustc_mir/transform/mod.rs

@@ -155,17 +155,30 @@ pub trait MirPass {
                          mir: &mut Mir<'tcx>);
 }

-pub macro run_passes($tcx:ident, $mir:ident, $def_id:ident, $suite_index:expr; $($pass:expr,)*) {{
-    let suite_index: usize = $suite_index;
+pub macro run_passes(


Unrelated: it'd be nice to move this out of a macro and into some kind of generic function. I don't really see any inherent difficulty in that. The functon could take a vec![&dyn MirPass] or else we could do some hancy h-list sort of thing (hint: seems unnecessary =)

src/librustc/mir/mod.rs

When inlining a function using the Mir inliner, we shouldn't rerun the various Mir passes on it because the Mir has already been lowered and that wil break various early Mir passes. The issue in rust-lang#50411 is that we've inlined a function with promotions whose Mir has already been lowered. The promotions are then copied into the local function and we begin to run passes on their lowered Mir which causes the ICE. Fixes rust-lang#50411

This can be obtained via the `$mir_phase` value.

As suggested in the feedback for rust-lang#55244. When I replaced the macro with a function, rustc started complaining that there were two unused functions so I also removed those.

wesleywiser · 2018-10-23T02:46:26Z

Good ideas! I've implemented the feedback.

nikomatsakis

r=me

nikomatsakis · 2018-10-23T17:30:08Z

src/librustc_mir/transform/mod.rs

+    mir: &mut Mir<'tcx>,
+    def_id: DefId,
+    mir_phase: MirPhase,
+    passes: &[&dyn MirPass]) {


Nit: rustfmt would put the ) { on the next line (with a trailing comma here)

Ah I was unsure of the convention here.

nikomatsakis · 2018-10-23T17:30:59Z

@bors r+

bors · 2018-10-23T17:31:01Z

📌 Commit c535147 has been approved by nikomatsakis

wesleywiser · 2018-10-23T18:34:51Z

Is it worth getting a perf run on these changes? I'm unsure of what the impact might be. It's probably small though...

wesleywiser · 2018-10-23T23:28:21Z

@bors try

For perf run, if we want that

bors · 2018-10-23T23:28:22Z

🙅 Please do not try after a pull request has been r+ed. If you need to try, unapprove (r-) it first.

nikomatsakis · 2018-10-24T19:58:59Z

@wesleywiser it is not worth it, in my opinion =)

bjorn3 · 2018-10-25T11:49:12Z

src/librustc/mir/mod.rs

+    /// Gets the index of the current MirPhase within the set of all MirPhases.
+    pub fn phase_index(&self) -> usize {
+        match self {
+            MirPhase::Build => 0,


Could you use self as usize instead of matching?

But then please assign the discriminants explicitly.

Sure, I can change that.

nikomatsakis · 2018-10-25T18:50:40Z

@bors r+

bors · 2018-10-25T18:50:41Z

📌 Commit 4655866 has been approved by nikomatsakis

@nikomatsakis

…akis Don't rerun MIR passes when inlining Fixes rust-lang#50411 r? @nikomatsakis I updated your commit message with additional details. Let me know if any of that is incorrect. I also added the appropriate `compile-flags` directive to the test. Thanks for you help on this! cc @RalfJung related to your PR rust-lang#55086

@nikomatsakis

…akis Don't rerun MIR passes when inlining Fixes rust-lang#50411 r? @nikomatsakis I updated your commit message with additional details. Let me know if any of that is incorrect. I also added the appropriate `compile-flags` directive to the test. Thanks for you help on this! cc @RalfJung related to your PR rust-lang#55086

Rollup of 11 pull requests Successful merges: - #55148 (Implement FromStr for PathBuf) - #55185 (path suggestions in Rust 2018 should point out the change in semantics) - #55191 (Fix sub-variant doc display) - #55199 (Impl items have generics) - #55244 (Don't rerun MIR passes when inlining) - #55252 (Add MaybeUninit::new) - #55257 (Allow extern statics with an extern type) - #55389 (Remove unnecessary mut in iterator.find_map documentation example, R…) - #55406 (Update string.rs) - #55412 (Fix an ICE in the min_const_fn analysis) - #55421 (Add ManuallyDrop::take)

rust-highfive assigned nikomatsakis Oct 21, 2018

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Oct 21, 2018

wesleywiser changed the title ~~Issue 50411~~ Don't rerun MIR passes when inlining Oct 21, 2018

This comment has been minimized.

Sign in to view

wesleywiser force-pushed the issue-50411 branch from 64e07d7 to 15afad1 Compare October 21, 2018 15:59

nikomatsakis approved these changes Oct 22, 2018

View reviewed changes

nikomatsakis requested changes Oct 22, 2018

View reviewed changes

src/librustc/mir/mod.rs Show resolved Hide resolved

nikomatsakis and others added 3 commits October 22, 2018 22:45

Remove the suite_index parameter of the run_passes!() macro

895a4b2

This can be obtained via the `$mir_phase` value.

Replace the run_passes! macro with a regular function

c535147

As suggested in the feedback for rust-lang#55244. When I replaced the macro with a function, rustc started complaining that there were two unused functions so I also removed those.

wesleywiser force-pushed the issue-50411 branch from 15afad1 to c535147 Compare October 23, 2018 02:45

nikomatsakis approved these changes Oct 23, 2018

View reviewed changes

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Oct 23, 2018

bjorn3 reviewed Oct 25, 2018

View reviewed changes

Fix CR feedback

4655866

pietroalbini mentioned this pull request Oct 25, 2018

Rollup of 15 pull requests #55357

Closed

kennytm mentioned this pull request Oct 28, 2018

Rollup of 11 pull requests #55433

Merged

bors merged commit 4655866 into rust-lang:master Oct 28, 2018

RalfJung mentioned this pull request Oct 29, 2018

MIR optimization level 3 works again :) rust-lang/miri#499

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Don't rerun MIR passes when inlining #55244

Don't rerun MIR passes when inlining #55244

wesleywiser commented Oct 21, 2018

This comment has been minimized.

nikomatsakis Oct 22, 2018

wesleywiser commented Oct 23, 2018

nikomatsakis left a comment

nikomatsakis Oct 23, 2018

wesleywiser Oct 23, 2018

nikomatsakis commented Oct 23, 2018

bors commented Oct 23, 2018

wesleywiser commented Oct 23, 2018

wesleywiser commented Oct 23, 2018

bors commented Oct 23, 2018

nikomatsakis commented Oct 24, 2018 •

edited

Loading

bjorn3 Oct 25, 2018

RalfJung Oct 25, 2018

wesleywiser Oct 25, 2018

nikomatsakis commented Oct 25, 2018

bors commented Oct 25, 2018

Don't rerun MIR passes when inlining #55244

Don't rerun MIR passes when inlining #55244

Conversation

wesleywiser commented Oct 21, 2018

This comment has been minimized.

nikomatsakis Oct 22, 2018

Choose a reason for hiding this comment

wesleywiser commented Oct 23, 2018

nikomatsakis left a comment

Choose a reason for hiding this comment

nikomatsakis Oct 23, 2018

Choose a reason for hiding this comment

wesleywiser Oct 23, 2018

Choose a reason for hiding this comment

nikomatsakis commented Oct 23, 2018

bors commented Oct 23, 2018

wesleywiser commented Oct 23, 2018

wesleywiser commented Oct 23, 2018

bors commented Oct 23, 2018

nikomatsakis commented Oct 24, 2018 • edited Loading

bjorn3 Oct 25, 2018

Choose a reason for hiding this comment

RalfJung Oct 25, 2018

Choose a reason for hiding this comment

wesleywiser Oct 25, 2018

Choose a reason for hiding this comment

nikomatsakis commented Oct 25, 2018

bors commented Oct 25, 2018

nikomatsakis commented Oct 24, 2018 •

edited

Loading