Do not remove trivial SwitchInt in analysis MIR

[compiler-errors]

This PR ensures that we don't prematurely remove trivial SwitchInt terminators which affects both the borrow-checking and runtime semantics (i.e. UB) of the code. Previously the SimplifyCfg optimization was removing SwitchInt terminators when they was "trivial", i.e. when all arms branched to the same basic block, even if that SwitchInt terminator had the side-effect of reading an operand which (for example) may not be initialized or may point to an invalid place in memory.

This behavior is unlike all other optimizations, which are only applied after "analysis" (i.e. borrow-checking) is finished, and which Miri disables to make sure the compiler doesn't silently remove UB.

Fixing this code "breaks" (i.e. unmasks) code that used to borrow-check but no longer does, like:

fn foo() {
    let x;
    let (0 | _) = x;
}

This match expression should perform a read because _ does not shadow the 0 literal pattern, and the compiler should have to read the match scrutinee to compare it to 0. I've checked that this behavior does not actually manifest in practice via a crater run which came back clean: #139042 (comment)

As a side-note, it may be tempting to suggest that this is actually a good thing or that we should preserve this behavior. If we wanted to make this work (i.e. trivially optimize out reads from matches that are redundant like 0 | _), then we should be enabling this behavior after fixing this. However, I think it's kinda unprincipled, and for example other variations of the code don't even work today, e.g.:

fn foo() {
    let x;
    let (0.. | _) = x;
}

[rustbot]

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]

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

The Miri subtree was changed

cc @rust-lang/miri

[compiler-errors]

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

[compiler-errors]

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

[tmiasko]

SimplifyCfg runs before borrowck, so the following compiles:

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

[compiler-errors]

oh wonderful, so this is a load bearing optimization then

[RalfJung]

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]

well i do wonder if anything relies on this today

@bors try

[bors]

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

[compiler-errors]

@rustbot author

[RalfJung]

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

[RalfJung]

This comment is still unresolved.

[bors]

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

[oli-obk]

@craterbot check

[craterbot]

👌 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]

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]

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]

🗑️ Experiment pr-139042 deleted!

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

[compiler-errors]

@bors try

[bors]

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

[compiler-errors]

Ah, on vacation 🌴
r? RalfJung or reassign

[traviscross]

I buy it.

@rfcbot fcp merge

[rfcbot]

Team member @traviscross has proposed to merge this. The next step is review by the rest of the tagged team members:

• @joshtriplett
• @nikomatsakis
• @scottmcm
• @tmandry
• @traviscross

No concerns currently listed.

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

cc @rust-lang/lang-advisors: FCP proposed for lang, please feel free to register concerns.
See this document for info about what commands tagged team members can give me.

[tmandry]

@rfcbot reviewed

[tmandry]

@bors rollup=never due to potential perf impacts

[rfcbot]

🔔 This is now entering its final comment period, as per the review above. 🔔

[compiler-errors]

There should be no perf impacts here, but we can just test that rather than guessing 😄

@bors try @rust-timer queue

[bors]

⌛ Trying commit 3ee62a9 with merge 2f722cba6738caa1d971f6ef4229d7d86891b5af...

[scottmcm]

Agreed this is something that should never have worked, and it's just a bug in mir transform.

[bors]

☀️ Try build successful - checks-actions
Build commit: 2f722cb (2f722cba6738caa1d971f6ef4229d7d86891b5af)

[rust-timer]

Finished benchmarking commit (2f722cb): comparison URL.

Overall result: no relevant changes - no action needed

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR may lead to changes in compiler perf.

@bors rollup=never
@rustbot label: -S-waiting-on-perf -perf-regression

Instruction count

This benchmark run did not return any relevant results for this metric.

Max RSS (memory usage)

Results (primary 0.7%, secondary -8.1%)

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	6.3%	[6.3%, 6.3%]	1
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-4.8%	[-4.8%, -4.8%]	1
Improvements ✅ (secondary)	-8.1%	[-8.1%, -8.1%]	1
All ❌✅ (primary)	0.7%	[-4.8%, 6.3%]	2

Cycles

Results (secondary 2.6%)

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	2.6%	[2.2%, 3.0%]	2
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-	-	0

Binary size

Results (primary -0.1%)

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-0.1%	[-0.1%, -0.1%]	1
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-0.1%	[-0.1%, -0.1%]	1

Bootstrap: 779.721s -> 780.808s (0.14%)
Artifact size: 366.13 MiB -> 366.15 MiB (0.01%)

[compiler-errors]

@bors rollup-

[saethlin]

My return from vacation hasn't processed yet but I'm quite interested in this PR so I've changed the assignment manually.