-
Notifications
You must be signed in to change notification settings - Fork 12.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Mark simplify_aggregate_to_copy
mir-opt as unsound
#132356
Mark simplify_aggregate_to_copy
mir-opt as unsound
#132356
Conversation
r? @Nadrieril rustbot has assigned @Nadrieril. Use |
Some changes occurred to MIR optimizations cc @rust-lang/wg-mir-opt |
Nominating for beta-backport as this is a mir-opt miscompile. |
4010d46
to
e09ee93
Compare
This comment was marked as resolved.
This comment was marked as resolved.
e09ee93
to
981f9ec
Compare
981f9ec
to
51e1b28
Compare
Changes since last review:
|
Probably r? mir-opt |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indeed, we aren't allowed to introduce new dereferences without checking for aliasing.
r=me
// reproduce the miscompile. | ||
//@[release] compile-flags: -C opt-level=1 | ||
//@[debug] compile-flags: -C opt-level=0 | ||
//@ run-pass |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you copy this into mir-opt test which shows pop_min.GVN.diff
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To clarify, do you mean retain this ui test (for now), but also create a new mir-opt test which shows the GVN diff?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can use this for a mir-opt test if you like:
#![allow(internal_features)]
#![feature(rustc_attrs, core_intrinsics)]
#[no_mangle]
fn foo(v: &mut Option<i32>) -> Option<i32> {
if let &Some(col) = get(&v) {
*v = None;
return Some(col);
} else {
unsafe { std::intrinsics::unreachable() }
}
}
#[no_mangle]
#[inline(never)]
#[rustc_nounwind]
fn get(v: &Option<i32>) -> &Option<i32> {
v
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, I tried to add a GVN.diff
test w/ this snippet, lmw if the test needs adjustments.
51e1b28
to
29db55e
Compare
Added a GVN diff mir-opt test as requested. |
@bors r+ |
Thanks! |
@bors retry (msvc spurious linker failure) |
…e_to_copy, r=cjgillot,DianQK Mark `simplify_aggregate_to_copy` mir-opt as unsound Mark the `simplify_aggregate_to_copy` mir-opt added in rust-lang#128299 as unsound as it seems to miscompile the MCVE reported in rust-lang#132353. The mir-opt can be re-enabled once this case is fixed. ```rs fn pop_min(mut score2head: Vec<Option<usize>>) -> Option<usize> { loop { if let Some(col) = score2head[0] { score2head[0] = None; return Some(col); } } } fn main() { let min = pop_min(vec![Some(1)]); println!("min: {:?}", min); // panic happens here on beta in release mode // but not in debug mode min.unwrap(); } ``` This MCVE is included as a `run-pass` ui regression test in the first commit. I built the ui test with a nightly manually, and can reproduce the behavioral difference with `-C opt-level=0` and `-C opt-level=1`. Locally, this ui test will fail unless it was run on a compiler built with the second commit marking the mir-opt as unsound thus disabling it by default. This PR **partially reverts** commit e7386b3, reversing changes made to 02b1be1. The mir-opt implementation is just marked as unsound but **not** reverted to make reland reviews easier. The test changes are **fully** reverted. cc `@DianQK` `@cjgillot` (PR author and reviewer of rust-lang#128299)
Maybe don't rollup pr where optimization was disabled? |
|
ah, didn't see that this affected default mir opt levels |
Oh oops should have rollup=never yeah. |
☀️ Test successful - checks-actions |
Finished benchmarking commit (a0d98ff): comparison URL. Overall result: ❌✅ regressions and improvements - please read the text belowOur benchmarks found a performance regression caused by this PR. Next Steps:
@rustbot label: +perf-regression Instruction countThis is the most reliable metric that we have; it was used to determine the overall result at the top of this comment. However, even this metric can sometimes exhibit noise.
Max RSS (memory usage)Results (primary -2.2%, secondary 0.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.
CyclesResults (primary 0.9%)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.
Binary sizeResults (primary 0.1%, secondary 0.3%)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.
Bootstrap: 784.059s -> 782.659s (-0.18%) |
This deactivated an unsound MIR optimization until it is fixed. @rustbot label: +perf-regression-triaged |
[beta] backports - Bump libc to 0.2.161 rust-lang#131823 - Avoid use imports in `thread_local_inner!` rust-lang#131866 - Mark `simplify_aggregate_to_copy` mir-opt as unsound rust-lang#132356 r? cuviper
Mark the
simplify_aggregate_to_copy
mir-opt added in #128299 as unsound as it seems to miscompile the MCVE reported in #132353. The mir-opt can be re-enabled once this case is fixed.This MCVE is included as a
run-pass
ui regression test in the first commit. I built the ui test with a nightly manually, and can reproduce the behavioral difference with-C opt-level=0
and-C opt-level=1
. Locally, this ui test will fail unless it was run on a compiler built with the second commit marking the mir-opt as unsound thus disabling it by default.This PR partially reverts commit e7386b3, reversing changes made to 02b1be1. The mir-opt implementation is just marked as unsound but not reverted to make reland reviews easier. Test changes are reverted if they were not pure additions. Tests added by the original PR received
-Z unsound-mir-opts
compile-flags.cc @DianQK @cjgillot (PR author and reviewer of #128299)