Do not remove trivial `SwitchInt` with mir-opt-level=0 #139042

compiler-errors · 2025-03-28T03:31:48Z

When mir-opt-level=0, do not optimize out SwitchInt terminators that all have the same terminator since it may remove a read which affects miri's ability to detect UB on that operand.

cc @RalfJung

Fixes rust-lang/miri#4237

This affects some tests... I guess I could mark them as mir-opt-level=1? Not sure.

rustbot · 2025-03-28T03:31:52Z

r? @saethlin

rustbot has assigned @saethlin.
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

rustbot · 2025-03-28T03:31:54Z

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

The Miri subtree was changed

cc @rust-lang/miri

compiler-errors · 2025-03-28T03:37:16Z

compiler/rustc_mir_transform/src/simplify.rs

-            _ => return false,
+        // Removing a `SwitchInt` terminator may remove reads that result in UB,
+        // so we must not apply this optimization when mir-opt-level = 0.
+        if self.mir_opt_level == 0 {


We could perhaps optimize trivial operands... Like plain locals. Maybe consts?

Well I guess even plain locals could have UB if they're uninitialized. But Rust would never produce that 🤔

SimplifyCfg runs before borrowck, so the following compiles:

fn main() { let bad_ref: &i32; let &(0 | _) = bad_ref; }

oh wonderful, so this is a load bearing optimization then

Ah, not just a bug for Miri then but a bigger problem. Certainly we shouldn't optimize before borrowck.

So instead of checking the mir_opt_level, this should be told which "phase" it is in (or it can maybe even get that info from the MIR body, not sure), and then not do this transformation in the "analysis" phase.

compiler-errors · 2025-03-28T05:39:47Z

well i do wonder if anything relies on this today

@bors try

…chint, r=<try> Do not remove trivial `SwitchInt` with mir-opt-level=0 When mir-opt-level=0, do not optimize out `SwitchInt` terminators that all have the same terminator since it may remove a read which affects miri's ability to detect UB on that operand. cc `@RalfJung` Fixes rust-lang/miri#4237 This affects some tests... I guess I could mark them as `mir-opt-level=1`? Not sure.

bors · 2025-03-28T05:40:59Z

⌛ Trying commit 6bad178 with merge 4dba42b...

compiler-errors · 2025-03-28T05:47:42Z

@rustbot author

RalfJung · 2025-03-28T07:14:39Z

src/tools/miri/tests/fail/read_from_trivial_switch.rs

@@ -0,0 +1,8 @@
+use std::mem::MaybeUninit;


Please add a top-level doc comment explaining what this is testing and referencing the issue.

bors · 2025-03-28T07:46:55Z

☀️ Try build successful - checks-actions
Build commit: 4dba42b (4dba42b29bacedb63812cd6f781e31ba4b782a95)

oli-obk · 2025-03-28T08:22:29Z

@craterbot check

craterbot · 2025-03-28T08:22:44Z

👌 Experiment pr-139042 created and queued.
🤖 Automatically detected try build 4dba42b
🔍 You can check out the queue and this experiment's details.

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

tmiasko · 2025-03-28T08:35:13Z

Pull request doesn't resolve #139042 (comment) yet and mir-opt-level=0 is never used by default. Are we getting ahead of ourselves with crater?

RalfJung · 2025-03-28T08:47:50Z

Yeah I think we do.
@craterbot cancel

The PR first needs to be changed to never do this SwitchInt removal in SimplifyCfg::Initial, SimplifyCfg::PromoteConsts, SimplifyCfg::RemoveFalseEdges, SimplifyCfg::PostAnalysis. And for SimplifyCfg::PreOptimizations it should likely be gated on mir_opt_level. Maintaining that list seems fragile so hopefully there is a better way... maybe it is checking for the phase and the opt_level.

Alternatively, instead of checking the opt level, we could have a separate flag. We already have a similar problem with RemovePlaceMention, which is now gated on the mir_keep_place_mention Z-flag. We could instead have a mir_preserve_ub Z-flag that controls both RemovePlaceMention and SimplifyCfg.

craterbot · 2025-03-28T08:50:37Z

🗑️ Experiment pr-139042 deleted!

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

compiler-errors · 2025-03-30T19:02:25Z

@bors try

bors · 2025-03-30T19:03:36Z

⌛ Trying commit 01ee75d with merge 5fee451...

…chint, r=<try> Do not remove trivial `SwitchInt` with mir-opt-level=0 When mir-opt-level=0, do not optimize out `SwitchInt` terminators that all have the same terminator since it may remove a read which affects miri's ability to detect UB on that operand. cc `@RalfJung` Fixes rust-lang/miri#4237 This affects some tests... I guess I could mark them as `mir-opt-level=1`? Not sure.

bors · 2025-03-30T21:09:56Z

☀️ Try build successful - checks-actions
Build commit: 5fee451 (5fee451ebd94296ea05214e819c311e164b30af6)

compiler-errors · 2025-03-30T23:11:37Z

@craterbot check

craterbot · 2025-03-30T23:11:54Z

👌 Experiment pr-139042 created and queued.
🤖 Automatically detected try build 5fee451
🔍 You can check out the queue and this experiment's details.

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

craterbot · 2025-03-30T23:13:34Z

🚧 Experiment pr-139042 is now running

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

craterbot · 2025-03-31T21:26:01Z

🎉 Experiment pr-139042 is completed!
📊 5 regressed and 5 fixed (606182 total)
📰 Open the full report.

⚠️ If you notice any spurious failure please add them to the denylist!
ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

compiler-errors · 2025-03-31T21:30:40Z

Oh good. Looks like nobody relies on this behavior.

@RalfJung: I took your suggested approach and generalized the flag into -Zmir-preserve-ub and then disabled SwitchInt optimization in the Built phase, since that's the pre-Analysis(Initial) state that we use for borrowck.

I can confirm that causes the example that @tmiasko shared to fail to compile (tho I'll add a test).

I'll write up a better description and then probably FCP this with lang?

RalfJung · 2025-04-01T06:36:58Z

compiler/rustc_mir_transform/src/simplify.rs

+        // Preserve `SwitchInt` reads on the phase transition from Built -> Analysis(Initial)
+        // or if `-Zmir-preserve-ub`.
+        let preserve_switch_reads =
+            matches!(body.phase, MirPhase::Built) | tcx.sess.opts.unstable_opts.mir_preserve_ub;


I think we should preserve SwitchInt for all "analysis" invocations of this pass. Otherwise, further analyses like const_check or... not sure what else we have... might start depending on this optimization, which we definitely do not want.

Also why are you using |, not ||?

typo re: |

RalfJung · 2025-04-01T06:38:07Z

compiler/rustc_mir_transform/src/simplify.rs

@@ -66,8 +66,8 @@ impl SimplifyCfg {
    }
 }

-pub(super) fn simplify_cfg(body: &mut Body<'_>) {
-    CfgSimplifier::new(body).simplify();
+pub(super) fn simplify_cfg<'tcx>(tcx: TyCtxt<'tcx>, body: &mut Body<'tcx>) {


Can you add a big fat warning somewhere prominently in this file that this optimization is one of the few that runs already on analysis MIR so we must be extremely careful since changes here can affect which programs even compile in an insta-stable way?

Do we need it to be running on analysis MIR?

it's really nice for real trivial terminators like gotos and empty bb's

rustbot assigned saethlin Mar 28, 2025

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Mar 28, 2025

compiler-errors commented Mar 28, 2025

View reviewed changes

This comment has been minimized.

Sign in to view

rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Mar 28, 2025

compiler-errors unassigned saethlin Mar 28, 2025

RalfJung reviewed Mar 28, 2025

View reviewed changes

craterbot added S-waiting-on-crater Status: Waiting on a crater run to be completed. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Mar 28, 2025

craterbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-crater Status: Waiting on a crater run to be completed. labels Mar 28, 2025

compiler-errors force-pushed the do-not-optimize-switchint branch from 6bad178 to 01ee75d Compare March 30, 2025 19:01

This comment has been minimized.

Sign in to view

craterbot added S-waiting-on-crater Status: Waiting on a crater run to be completed. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Mar 30, 2025

Do not optimize out SwitchInt before borrowck, or if Zmir-preserve-ub

6fa280b

compiler-errors force-pushed the do-not-optimize-switchint branch from 01ee75d to 6fa280b Compare March 30, 2025 23:21

craterbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-crater Status: Waiting on a crater run to be completed. labels Mar 31, 2025

RalfJung reviewed Apr 1, 2025

View reviewed changes

meithecatte mentioned this pull request Apr 1, 2025

Make closure capturing have consistent and correct behaviour around patterns #138961

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Do not remove trivial `SwitchInt` with mir-opt-level=0 #139042

Do not remove trivial `SwitchInt` with mir-opt-level=0 #139042

compiler-errors commented Mar 28, 2025

rustbot commented Mar 28, 2025

rustbot commented Mar 28, 2025

compiler-errors Mar 28, 2025

compiler-errors Mar 28, 2025

tmiasko Mar 28, 2025

compiler-errors Mar 28, 2025

RalfJung Mar 28, 2025

This comment has been minimized.

compiler-errors commented Mar 28, 2025

bors commented Mar 28, 2025

compiler-errors commented Mar 28, 2025

RalfJung Mar 28, 2025

bors commented Mar 28, 2025

oli-obk commented Mar 28, 2025

craterbot commented Mar 28, 2025

tmiasko commented Mar 28, 2025

RalfJung commented Mar 28, 2025

craterbot commented Mar 28, 2025

compiler-errors commented Mar 30, 2025

bors commented Mar 30, 2025

This comment has been minimized.

bors commented Mar 30, 2025

compiler-errors commented Mar 30, 2025

craterbot commented Mar 30, 2025

craterbot commented Mar 30, 2025

craterbot commented Mar 31, 2025

compiler-errors commented Mar 31, 2025

RalfJung Apr 1, 2025

RalfJung Apr 1, 2025

compiler-errors Apr 1, 2025

RalfJung Apr 1, 2025 •

edited

Loading

meithecatte Apr 1, 2025

compiler-errors Apr 1, 2025

Do not remove trivial SwitchInt with mir-opt-level=0 #139042

Are you sure you want to change the base?

Do not remove trivial SwitchInt with mir-opt-level=0 #139042

Conversation

compiler-errors commented Mar 28, 2025

rustbot commented Mar 28, 2025

rustbot commented Mar 28, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment has been minimized.

compiler-errors commented Mar 28, 2025

bors commented Mar 28, 2025

compiler-errors commented Mar 28, 2025

Choose a reason for hiding this comment

bors commented Mar 28, 2025

oli-obk commented Mar 28, 2025

craterbot commented Mar 28, 2025

tmiasko commented Mar 28, 2025

RalfJung commented Mar 28, 2025

craterbot commented Mar 28, 2025

compiler-errors commented Mar 30, 2025

bors commented Mar 30, 2025

This comment has been minimized.

bors commented Mar 30, 2025

compiler-errors commented Mar 30, 2025

craterbot commented Mar 30, 2025

craterbot commented Mar 30, 2025

craterbot commented Mar 31, 2025

compiler-errors commented Mar 31, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RalfJung Apr 1, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Do not remove trivial `SwitchInt` with mir-opt-level=0 #139042

Do not remove trivial `SwitchInt` with mir-opt-level=0 #139042

RalfJung Apr 1, 2025 •

edited

Loading