Tracking issue for MIR (RFC #1211)

[nikomatsakis]

Tracking issue for transition to MIR (rust-lang/rfcs#1211).

Relevant discuss thread: https://internals.rust-lang.org/t/transitioning-to-mir/2706

Regressions

These are cases where building MIR is causing existing crates to stop working.

• Stack overflow when compiling lots of macros #29466
• reduce memory usage for match Perf regression in nightly when compiling massive matches #29227

Refactorings

These are places where the design of MIR needs some tweaks. In some cases, the best course of action may be unclear or in active debate.

• Calls Mir: Refactor calls to permit calls that cannot panic and to better accommodate calls that always panic #29767
• Panics Mir: remove Panic terminator and instead just call suitable lang item #29573
• Scopes Mir: refactor how scopes are represented #29768
• Overflow Mir: introduce overflow operations #29769

Other work

• issues tagged with A-mir
• add overflow and division by zero checking
• constant propagation and hoisting
• port rvalue checking
• port dead-code pass
• port reachability
• port match checking to take place during MIR construction
• port effect checking
• port intrinsicck
• enable trans universally

[steveklabnik]

#27893

[nikomatsakis]

PR #28748 enables MIR construction unconditionally.

[bstrie]

@nikomatsakis I've heard reports that unconditional MIR construction is leading to OOM for certain people. Is MIR construction going to also be enabled for the stable and beta releases? I'd hate for compiler perf to regress for 1.5-stable...

[bstrie]

@nikomatsakis Not just memory regression, but perf regression too:

< Rym> Anyone know anything about the 'MIR Dump' pass and factors which might affect its speed?
<@bstrie> Rym: is it causing performance problems for you?
< Rym> bstrie: It's approximately 6 seconds of a 12second no-trans build (effectively doubling the time compared to a beta compiler, where it took <0.01s)

To reiterate my question, is it safe to assume that we won't be turning on MIR construction unconditionally for 1.5-beta, even if we leave it on for 1.6-nightly?

[Ryman]

crate/commit in question for the above: nickel-org/nickel.rs@197aac3
rustc: rustc 1.5.0-nightly (1210fb9bc 2015-10-23)

It's still repro on latest nightly: rustc 1.5.0-nightly (c3db627cb 2015-10-26).
Same compiler has <0.2s 'MIR dump' for hyper, so I'm wondering if we're doing something particularly bad within nickel? Seems to bounce around between 4-6seconds.

[nikomatsakis]

@bstrie Good question. I was actually just debating now whether to land a PR disabling construction without some sort of flag for that reason, since I haven't had time to investigate and fix those regressions (which was of course my preferred solution).

[nikomatsakis]

@Ryman the existing issue had to do with match translation, do you know if you have any match statements that have large sets of constants (or lots and lots of patterns)? For example:

match foo {
    1 | 2 | ... | 32767 => ...
    ...
}

[Ryman]

@nikomatsakis Ah, yeah we do, this expands into two decent sized matches. If I extracted that to another crate and re-exported the type I guess we'd lose the hit?

[nikomatsakis]

@Ryman well ideally we'll just fix the codegen issue :) I'm not sure if
exporting it to another crate would help, except that you would only do it
once I guess. Anyway, I've just about got a PR that disables MIR on
beta/stable for now.

On Mon, Oct 26, 2015 at 2:57 PM, Ryman notifications@github.com wrote:

@nikomatsakis https://github.com/nikomatsakis Ah, yeah we do
https://github.com/nickel-org/nickel.rs/blob/master/src/mimes.rs, this
expands into two decent sized matches. If I extracted that to another crate
and re-exported the type I guess we'd lose the hit?

—
Reply to this email directly or view it on GitHub
#27840 (comment).

[nikomatsakis]

@Ryman still that match doesn't quite fit the pattern I was expecting, I'll
have to investigate more deeply.

On Mon, Oct 26, 2015 at 3:07 PM, Nicholas Matsakis nmatsakis@mozilla.com
wrote:

@Ryman well ideally we'll just fix the codegen issue :) I'm not sure if
exporting it to another crate would help, except that you would only do it
once I guess. Anyway, I've just about got a PR that disables MIR on
beta/stable for now.

On Mon, Oct 26, 2015 at 2:57 PM, Ryman notifications@github.com wrote:

@nikomatsakis https://github.com/nikomatsakis Ah, yeah we do
https://github.com/nickel-org/nickel.rs/blob/master/src/mimes.rs, this
expands into two decent sized matches. If I extracted that to another crate
and re-exported the type I guess we'd lose the hit?

—
Reply to this email directly or view it on GitHub
#27840 (comment).

[Ryman]

@nikomatsakis it looks like each of the derivings for that enum also result in huge matches against single tuple refs, e.g. (&Foo,) => { .. }. Would that fit the pattern more?

[nikomatsakis]

@Ryman I was more thinking of matches against integer literals or other
non-enum types.

On Mon, Oct 26, 2015 at 4:58 PM, Ryman notifications@github.com wrote:

@nikomatsakis https://github.com/nikomatsakis it looks like each of the
derivings for that enum also result in huge matches against single tuple
refs, e.g. (&Foo,) => { .. }. Would that fit the pattern more?

—
Reply to this email directly or view it on GitHub
#27840 (comment).

[Aatch]

For overflow checking, there's a few options we go for handling it:

1. Do it all in LLVM code generation, similar to now. This isn't bad, but hides potential control-flow from the MIR.
2. Add a new terminator instruction that, like the Call terminator takes two branches, one for success and one for failure.
3. Add a new Rvalue that produces a (result, overflow) tuple then unpack it and generate a conditional panic.
4. Add a special "Overflow" operand that is implicitly set by the arithmetic operators and use it to generate a conditional panic. It can default to false.

I don't mind 3) as it means we can separate out the overflowing and non-overflowing cases. This is good if want to be able to turn it on and off "arbitrarily". It also makes the control flow explicit in the MIR.

[nikomatsakis]

@Aatch option 3 is what I had in mind. This maps precisely to what LLVM does, as well. I think that option 1 is a non-starter because translation no longer has knowledge of scopes or how to translate panics. Option 2 is probably overly limiting: option 3 gives us room to do optimizations like deferring the panic till after we collect various adjacent, pure operations.

[eddyb]

@nikomatsakis as an aside, deferring can be vectorized, reducing the slowdown on vectorized loops to around 2-3x (IIRC that's an order of magnitude better).

If a.add_with_overflow(b) is implemented as (a + b, a + b != a.saturating_add(b)), then multiple additions, saturated additions and comparisons can be executed in parallel, unlike the usual "do an add and check the overflow flag".

But LLVM can't do it by itself, since it requires vectorizing saturating arithmetic and LLVM doesn't have built-in saturating arithmetic, because if it did, it could vectorize it just like it can vectorize usual arithmetic.

[eddyb]

I think I know how I want to handle constants: have a mix between a Block-based Builder and a "constant builder" and make Rvalue translation generic over that.
Then, you can analyze temporaries and mark some of them as constant, even if you're translating a function and not a const item.

EDIT: That idea didn't pan out, see #33130 for the pre-miri implementation.

[eddyb]

Speaking of constants, this is a bit of a worst-case:

#![crate_type = "lib"]
#![feature(rustc_attrs)]

#[rustc_mir]
pub fn foo() -> [i32; 16] { [1, 0, 0, 0, 0, 1, 0, 0, 0, 0, 1, 0, 0, 0, 0, 1] }

Old trans does rvalue promotion everywhere which results in a beautiful single memcpy from constant global, whereas MIR trans generates one GEPi and one store for each element of the array.
One way to solve this is to run the const qualification pass from trans and translate statements based on whether they're considered const or not.
But it's also not that useful to do it everywhere, only for constructing non-immediate constant values (and [x; N] shouldn't ever get promoted this way, because doing it at runtime is always more efficient).

[nikomatsakis]

@eddyb it seems like this is something that the "constant promotion" code in MIR could handle, no? I'm a bit confused what distinguishes this particular case from other aggregates? (if anything)

[eddyb]

@nikomatsakis The promotion code is fine-tuned for enforcing semantics right now.
It only promotes references to a tree of temporary rvalues, so that the borrow-checker knows they're 'static.
I thought about running it after monomorphization but it occurs to me that constness is irrespective of type parameters, and if we promote immediates we can always translate them in a simple way in trans::mir::constant to not end up with silly memcpy's.
Have to be careful about it though, since it can make life hard for all other passes, I would run it in the "prepare for trans" pass group.
It should also only do promotion on Aggregate Rvalues that contain no nested Repeat Rvalues, to avoid any bloat. I hope the resulting MIR won't waste a lot of memory.

@Doener Was it you who found that optimization difference between MIR and old trans, where old trans would promote a None or some such?
Can you remember the precise details? IIRC it was something we should fix in the way we generate constant ADTs.

[solson]

Unless I misunderstand what it means, the item "enable trans universally" in the OP should be marked done now. Is anything else in the OP already done?

[nikomatsakis]

@solson I think we can close this tracking issue now. Doing so.

[Eh2406]

There is a FIXME related to this issue,
https://github.com/rust-lang/rust/blob/master/src/librustc_mir/build/expr/category.rs#L92
Now that the issue is closed can the FIXME be fix, or made more specific?