mir-transform: Treat optimize(none) the same as opt-level=0 - #160524
Conversation
354d5d9 to
e4742bc
Compare
|
Typo error caused an ICE in drop elaboration - optimizations.0 >= min_level
+ optimizations.0 < min_levelwhich seems a bit surprising EDIT: Seems like some other opts rely on |
This comment has been minimized.
This comment has been minimized.
|
Note that this now fails because #[optimize(none)]
pub fn const_branch() -> i32 {
if true { 1 } else { 0 }
}produces |
What do you mean by "rely" here and why does this become more of a problem with this PR? |
I didn't look into it deeply, but a few passes seemed to ICE if they ran with dead BBs.
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 |
|
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 |
|
Thanks! Could you file an issue? Then we can ping some folks to see what the expected contract for these passes is.
|
e4742bc to
963617f
Compare
This comment has been minimized.
This comment has been minimized.
963617f to
011d6ef
Compare
|
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 ; |
This comment has been minimized.
This comment has been minimized.
011d6ef to
7911d5f
Compare
This comment has been minimized.
This comment has been minimized.
|
I am not sure that |
7911d5f to
67ab36c
Compare
|
Some changes occurred in coverage instrumentation. cc @Zalathar Some changes occurred to MIR optimizations cc @rust-lang/wg-mir-opt |
|
r? @JohnTitor rustbot has assigned @JohnTitor. Use Why was this reviewer chosen?The reviewer was selected based on:
|
|
I dropped the |
efdd293 to
fcdb1a5
Compare
|
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. |
|
This is still "waiting for author"... what is left to do here? |
a825cd2 to
42853f2
Compare
|
I think |
42853f2 to
1076f97
Compare
1076f97 to
13b81b7
Compare
|
Thanks. :) @bors r+ |
This comment has been minimized.
This comment has been minimized.
What is this?This is an experimental post-merge analysis report that shows differences in test outcomes between the merged PR and its parent PR.Comparing f207aa3 (parent) -> f248f40 (this PR) Test differencesShow 10 test diffs10 doctest diffs were found. These are ignored, as they are noisy. Test dashboardRun cargo run --manifest-path src/ci/citool/Cargo.toml -- \
test-dashboard f248f4038796913873f11ca65b1b901e311c8dae --output-dir test-dashboardAnd then open Job duration changes
How to interpret the job duration changes?Job durations can vary a lot, based on the actual runner instance |
|
Finished benchmarking commit (f248f40): comparison URL. Overall result: no relevant changes - no action needed@rustbot label: -perf-regression Instruction countThis perf run didn't have relevant results for this metric. Max RSS (memory usage)Results (primary 2.7%, secondary -2.0%)A less reliable metric. May be of interest, but not used to determine the overall result above.
CyclesResults (primary -2.2%, secondary -2.1%)A less reliable metric. May be of interest, but not used to determine the overall result above.
Binary sizeThis perf run didn't have relevant results for this metric. Bootstrap: 476.209s -> 476.686s (0.10%) |
View all comments
cc @RalfJung