Uh oh!
There was an error while loading. Please reload this page.
Don't rerun MIR passes when inlining - #55244
Conversation
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
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 =)
Uh oh!
There was an error while loading. Please reload this page.
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. Fixesrust-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
commented
Oct 23, 2018
Good ideas! I've implemented the feedback. |
| mir: &mut Mir<'tcx>, | ||
| def_id: DefId, | ||
| mir_phase: MirPhase, | ||
| passes: &[&dyn MirPass]) { |
There was a problem hiding this comment.
Nit: rustfmt would put the ) { on the next line (with a trailing comma here)
There was a problem hiding this comment.
Ah I was unsure of the convention here.
nikomatsakis
commented
Oct 23, 2018
@bors r+ |
bors
commented
Oct 23, 2018
📌 Commit c535147 has been approved by |
wesleywiser
commented
Oct 23, 2018
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
commented
Oct 23, 2018
@bors try For perf run, if we want that |
bors
commented
Oct 23, 2018
🙅 Please do not |
@wesleywiser it is not worth it, in my opinion =) |
| /// Gets the index of the current MirPhase within the set of all MirPhases. | ||
| pub fn phase_index(&self) -> usize { | ||
| match self { | ||
| MirPhase::Build => 0, |
There was a problem hiding this comment.
Could you use self as usize instead of matching?
There was a problem hiding this comment.
But then please assign the discriminants explicitly.
nikomatsakis
commented
Oct 25, 2018
@bors r+ |
bors
commented
Oct 25, 2018
📌 Commit 4655866 has been approved by |
…akis Don't rerun MIR passes when inlining Fixesrust-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
…akis Don't rerun MIR passes when inlining Fixesrust-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)
Fixes#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-flagsdirective to the test.Thanks for you help on this!
cc @RalfJung related to your PR #55086