mir-transform: Treat optimize(none) the same as opt-level=0

[clubby789]

View all comments

cc @RalfJung

[clubby789]

Typo error caused an ICE in drop elaboration

- optimizations.0 >= min_level
+ optimizations.0 < min_level

which seems a bit surprising

EDIT: Seems like some other opts rely on SimplifyCfg removing dead BBs. Not sure if we need to factor that into the pass handling? Probably fine to ignore in this PR

[clubby789]

Note that this now fails because

#[optimize(none)]
pub fn const_branch() -> i32 {
    if true { 1 } else { 0 }
}

produces return 1

[RalfJung]

EDIT: Seems like some other opts rely on SimplifyCfg removing dead BBs. Not sure if we need to factor that into the pass handling? Probably fine to ignore in this PR

What do you mean by "rely" here and why does this become more of a problem with this PR?

[clubby789]

What do you mean by "rely" here

I didn't look into it deeply, but a few passes seemed to ICE if they ran with dead BBs.

and why does this become more of a problem with this PR?

It doesn't; I just made a typo during implementation which exposed this and I was unsure if that was intentional/known.

[RalfJung]

It doesn't; I just made a typo during implementation which exposed this and I was unsure if that was intentional/known.

We'd have to ask the people that wrote the passes, so it'd help if you had a concrete example and backtrace. I presume this can be reproduced with -Zmir-enable-passes already on nightly?

[clubby789]

The ICE is from a debug assertion; with the typo reverted, and debug-assertions enabled, the ICE is as follows (when building std) rustc-ice-2026-08-10T06_58_41-132370.txt

Minimised to a baseline rustc with debug assertions:

// rustc ice.rs -Zmir-enable-passes=-SimplifyCfg-initial,-SimplifyCfg-promote-consts,-SimplifyCfg-post-analysis
fn mir_drop<T>(_place: T) {
    panic!()
}

fn main() {
    mir_drop(());
}

Not sure if this is a problem at all in practice? If you think it is I can split out a new issue since it's not too relevant to this PR specifically

[RalfJung]

Thanks! Could you file an issue? Then we can ping some folks to see what the expected contract for these passes is.

[clubby789]

Refactored around this API:

impl PassPolicy {
    /// Create a [`PassPolicy::Optional`] that is not an optimization,
    /// enabled by default under the given condition.
    pub(crate) fn optional_non_optimization(enabled_by_default: bool) -> Self;

    /// Create a [`PassPolicy::Optional`] optimization enabled at the given MIR optimization level.
    pub(crate) fn optimization(ctx: &PassCtx<'_>, min_mir_opt_level: usize) -> Self;

    /// Add another condition to an optional pass's default enablement.
    pub(crate) fn and_enabled(self, enabled: bool) -> Self;

    /// Add a minimum mir-opt-level to an optional pass.
    /// Will panic if used on a required pass.
    fn with_min_mir_opt_level(self, ctx: &PassCtx<'_>, min_mir_opt_level: usize) -> Self ;

[RalfJung]

I am not sure that PassPolicy::optimization(ctx, 2) is really more readable than PassPolicy::optimization(ctx.mir_opt_level() >= 2). The meaning of the "2" becomes rather opaque.

[rustbot]

Some changes occurred in coverage instrumentation.

cc @Zalathar

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

[rustbot]

r? @JohnTitor

rustbot has assigned @JohnTitor.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

Why was this reviewer chosen?

The reviewer was selected based on:

• Owners of files modified in this PR: compiler, mir, mir-opt
• compiler, mir, mir-opt expanded to 75 candidates
• Random selection from 16 candidates

[clubby789]

I dropped the optimization method, since it's generally easier to just have an optional constructor which can take a ctx.mir_opt_level() comparison. I also updated the opt-none test to something that is no longer optimised at level 0.

[clubby789]

rust/tests/coverage/async.rs

Line 6 in 8925ea3

//@ compile-flags: -Copt-level=1

We dropped the -Copt-level checks in inlining; as this test runs at -Copt-level=1, inlining improves reachability checks and eliminates the branch calling d(), making it unused

- Function name: async::j::d
- Raw bytes (19): 0x[01, 01, 00, 03, 01, 40, 05, 00, 11, 01, 00, 14, 00, 15, 01, 00, 16, 00, 17]
+ Function name: async::j::d (unused)
+ Raw bytes (19): 0x[01, 01, 00, 03, 00, 40, 05, 00, 11, 00, 00, 14, 00, 15, 00, 00, 16, 00, 17]

[rustbot]

Some changes occurred in coverage tests.

cc @Zalathar

[clubby789]

Would be good to know if this change (just --bless'd it) is a good fix or if the test needs to be updated otherwise, since I don't have context for these tests

@bors try jobs=i686-msvc-1

[Zalathar]

Would be good to know if this change (just --bless'd it) is a good fix or if the test needs to be updated otherwise, since I don't have context for these tests

Yep, blessing is the correct fix here.

(Being known-unused affects some internal details which is why it shows up in the snapshots, but it's not something the test deeply cares about.)

[RalfJung]

We dropped the -Copt-level checks in inlining

I thought we droppe dthem in a way that would be equivalent... but I see now, we map -Copt-level=1 and -Copt-level=2 to the same mir-opt-level but we actually treated them different for inlining. What an odd choice.

So with this PR, non-incremental -Copt-level=1 builds will now do MIR inlining, which they did not do before.
Cargo by default only uses ``-Copt-level=0and-Copt-level=3`.

I guess we could just keep the original logic, just with ctx.mir_opt_level() rather than sess.mir_opt_level()?

[RalfJung]

Would it make sense to store an OptLevel here instead, so that the inliner can query it?

View changes since the review

[clubby789]

That would mean mir_opt_level 0/3/4 wouldn't apply (if manually overridden), since OptLevel translates to only MIR opt-level 1/2

As an aside, I was wondering about introducing an MIR opt-level newtype/enum rather than passing around usize, does that make any sense to do here?

[RalfJung]

That would mean mir_opt_level 0/3/4 wouldn't apply (if manually overridden), since OptLevel translates to only MIR opt-level 1/2

Ah, right.

As an aside, I was wondering about introducing an MIR opt-level newtype/enum rather than passing around usize, does that make any sense to do here?

Not in this PR please.

[RalfJung]

With this, we won't do MIR inlining with optimize(speed) if the global opt level is 0 or 1 which seems wrong. The inliner kind of introduces a mir-opt-level 1.5 here, it's the only MIR opt that dintinguishes -Copt-level=1 from -Copt-level=2. It seems like it has done this ever since #91743. @cjgillot @wesleywiser @oli-obk what is the reason this was done and do you have ideas for how it could be handled with the per-function attribute? We use ctx to communicate the intended opt level for the function this pass runs on, but that needs to reflected -Zmir-opt-level so it is expressed as a MIR opt level and therefore can't capture the logic here that depends on both mir-opt-level and -Copt-level. The easiest fix it to stop doing this odd special case but that would mean the MIR inliner would kick in at -Copt-level=1. IMO if we don't want that we shouldn't map -Copt-level=1 and -Copt-level=2 to the same MIR opt level.

View changes since the review

[clubby789]

Pushed an approach which records the source for the body-opt-level to at least retain the behaviour; this is kind of overkill (we could just have a local_override: bool flag or something on PassCtx), but having this information around might be useful.

[RalfJung]

Urgh :/

I'd rather "free up" mir-opt-level 3 so that we can map -Copt-level=1 to a different mir-opt-level. Or decide that the inliner doesn't need to be that special. The -Copt-levels other than 0 and 3 are not widely used anyway I assume...

[clubby789]

While we figure out the semantics, just going to try a perf run of mapping

- 0 => 1
- 1 | 2 | S | Z => 2
- 3 => 3

@bors try @rust-timer queue

[RalfJung]

FWIW the inliner distinguishes -Copt-level=1 from 2, not 2 from 3 as your new classification.

[clubby789]

IIRC the perf suite only records numbers for -C0 and -C3, so this should be fine for the perf run. Will adjust if we do make this change (but I'm assuming it's large enough to maybe worth doing in another PR)

[RalfJung]

I would assume if we do this we'd also check all the conditions for MIR opts to get a very good idea of which new opts we are enabling in -O now.

[rust-bors]

☀️ Try build successful (CI)
Build commit: 9e33c30 (9e33c30b55097ca78bb4329d3b2c242a33bb22b3)
Base parent: 526c36b (526c36b4c477994b7905ec6a084d6150e49076f5)

[rustbot]

This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.