Skip to content
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.

Sign up for GitHub

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

[mir-opt] Fix mis-optimization and other issues with the SimplifyArmIdentity pass #73949

Merged
merged 4 commits into from
Jul 4, 2020
Merged

[mir-opt] Fix mis-optimization and other issues with the SimplifyArmIdentity pass #73949

Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
e3f599c
Add test for simplify-try misoptimization
wesleywiser Jun 22, 2020
9248d90
[mir-opt] Prevent mis-optimization when SimplifyArmIdentity runs
wesleywiser Jun 22, 2020
24bfdc9
Fix debuginfo so that it points to the correct local
wesleywiser Jun 21, 2020
e16d6a6
Fix missing return in `optimization_applies()`
wesleywiser Jun 21, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
[mir-opt] Prevent mis-optimization when SimplifyArmIdentity runs 
If temporaries are used beyond just the temporary chain, then we can't
optimize out the reads and writes.
  • Loading branch information
@wesleywiser
wesleywiser committed Jul 3, 2020
commit 9248d90d20601e9d489a4a1c21df9de686b9fd82
51 changes: 49 additions & 2 deletions src/librustc_mir/transform/simplify_try.rs
View file
Original file line number Diff line number Diff line change
@@ -12,6 +12,7 @@
use crate::transform::{simplify, MirPass, MirSource};
use itertools::Itertools as _;
use rustc_index::vec::IndexVec;
use rustc_middle::mir::visit::{PlaceContext, Visitor};
use rustc_middle::mir::*;
use rustc_middle::ty::{Ty, TyCtxt};
use rustc_target::abi::VariantIdx;
@@ -75,7 +76,9 @@ struct ArmIdentityInfo<'tcx> {
stmts_to_remove: Vec<usize>,
}

fn get_arm_identity_info<'a, 'tcx>(stmts: &'a [Statement<'tcx>]) -> Option<ArmIdentityInfo<'tcx>> {
fn get_arm_identity_info<'a, 'tcx>(
stmts: &'a [Statement<'tcx>],
) -> Option<ArmIdentityInfo<'tcx>> {
// This can't possibly match unless there are at least 3 statements in the block
// so fail fast on tiny blocks.
if stmts.len() < 3 {
@@ -249,6 +252,7 @@ fn get_arm_identity_info<'a, 'tcx>(stmts: &'a [Statement<'tcx>]) -> Option<ArmId
fn optimization_applies<'tcx>(
opt_info: &ArmIdentityInfo<'tcx>,
local_decls: &IndexVec<Local, LocalDecl<'tcx>>,
local_uses: &IndexVec<Local, usize>,
) -> bool {
trace!("testing if optimization applies...");

@@ -285,6 +289,26 @@ fn optimization_applies<'tcx>(
last_assigned_to = *l;
}

// Check that the first and last used locals are only used twice
// since they are of the form:
//
// ```
// _first = ((_x as Variant).n: ty);
// _n = _first;
// ...
// ((_y as Variant).n: ty) = _n;
// discriminant(_y) = z;
// ```
for (l, r) in &opt_info.field_tmp_assignments {
if local_uses[*l] != 2 {
warn!("NO: FAILED assignment chain local {:?} was used more than twice", l);
return false;
} else if local_uses[*r] != 2 {
warn!("NO: FAILED assignment chain local {:?} was used more than twice", r);
return false;
}
}

if source_local != opt_info.local_temp_0 {
trace!(
"NO: start of assignment chain does not match enum variant temp: {:?} != {:?}",
@@ -312,11 +336,12 @@ impl<'tcx> MirPass<'tcx> for SimplifyArmIdentity {
}

trace!("running SimplifyArmIdentity on {:?}", source);
let local_uses = LocalUseCounter::get_local_uses(body);
let (basic_blocks, local_decls) = body.basic_blocks_and_local_decls_mut();
for bb in basic_blocks {
if let Some(opt_info) = get_arm_identity_info(&bb.statements) {
trace!("got opt_info = {:#?}", opt_info);
if !optimization_applies(&opt_info, local_decls) {
if !optimization_applies(&opt_info, local_decls, &local_uses) {
debug!("optimization skipped for {:?}", source);
continue;
}
@@ -358,6 +383,28 @@ impl<'tcx> MirPass<'tcx> for SimplifyArmIdentity {
}
}

struct LocalUseCounter {
local_uses: IndexVec<Local, usize>,
}

impl LocalUseCounter {
fn get_local_uses<'tcx>(body: &Body<'tcx>) -> IndexVec<Local, usize> {
let mut counter = LocalUseCounter { local_uses: IndexVec::from_elem(0, &body.local_decls) };
counter.visit_body(body);
counter.local_uses
}
}

impl<'tcx> Visitor<'tcx> for LocalUseCounter {
fn visit_local(&mut self, local: &Local, context: PlaceContext, _location: Location) {
if context.is_storage_marker() {
return;
}

self.local_uses[*local] += 1;
}
}

/// Match on:
/// ```rust
/// _LOCAL_INTO = ((_LOCAL_FROM as Variant).FIELD: TY);
37 changes: 20 additions & 17 deletions src/test/mir-opt/issue-73223/32bit/rustc.main.PreCodegen.diff
View file
Original file line number Diff line number Diff line change
@@ -3,9 +3,9 @@

fn main() -> () {
let mut _0: (); // return place in scope 0 at $DIR/issue-73223.rs:1:11: 1:11
let _1: i32; // in scope 0 at $DIR/issue-73223.rs:2:9: 2:14
let mut _2: std::option::Option<i32>; // in scope 0 at $DIR/issue-73223.rs:2:23: 2:30
let _3: i32; // in scope 0 at $DIR/issue-73223.rs:3:14: 3:15
let mut _1: std::option::Option<i32>; // in scope 0 at $DIR/issue-73223.rs:2:23: 2:30
let _2: i32; // in scope 0 at $DIR/issue-73223.rs:3:14: 3:15
let mut _4: i32; // in scope 0 at $DIR/issue-73223.rs:7:22: 7:27
let mut _5: (&i32, &i32); // in scope 0 at $SRC_DIR/libcore/macros/mod.rs:LL:COL
let mut _6: &i32; // in scope 0 at $SRC_DIR/libcore/macros/mod.rs:LL:COL
let mut _9: bool; // in scope 0 at $SRC_DIR/libcore/macros/mod.rs:LL:COL
@@ -28,10 +28,10 @@
let mut _28: std::fmt::ArgumentV1; // in scope 0 at $SRC_DIR/libstd/macros.rs:LL:COL
let mut _29: for<'r, 's, 't0> fn(&'r &i32, &'s mut std::fmt::Formatter<'t0>) -> std::result::Result<(), std::fmt::Error>; // in scope 0 at $SRC_DIR/libcore/macros/mod.rs:LL:COL
scope 1 {
debug split => _1; // in scope 1 at $DIR/issue-73223.rs:2:9: 2:14
let _4: std::option::Option<i32>; // in scope 1 at $DIR/issue-73223.rs:7:9: 7:14
debug split => _2; // in scope 1 at $DIR/issue-73223.rs:2:9: 2:14
let _3: std::option::Option<i32>; // in scope 1 at $DIR/issue-73223.rs:7:9: 7:14
scope 3 {
debug _prev => _4; // in scope 3 at $DIR/issue-73223.rs:7:9: 7:14
debug _prev => _3; // in scope 3 at $DIR/issue-73223.rs:7:9: 7:14
let _7: &i32; // in scope 3 at $SRC_DIR/libcore/macros/mod.rs:LL:COL
let _8: &i32; // in scope 3 at $SRC_DIR/libcore/macros/mod.rs:LL:COL
scope 4 {
@@ -64,30 +64,34 @@
}
}
scope 2 {
debug v => _3; // in scope 2 at $DIR/issue-73223.rs:3:14: 3:15
debug v => _2; // in scope 2 at $DIR/issue-73223.rs:3:14: 3:15
}
scope 7 {
}
scope 9 {
}

bb0: {
StorageLive(_1); // scope 0 at $DIR/issue-73223.rs:2:9: 2:14
StorageLive(_2); // scope 0 at $DIR/issue-73223.rs:2:23: 2:30
((_2 as Some).0: i32) = const 1_i32; // scope 0 at $DIR/issue-73223.rs:2:23: 2:30
StorageLive(_1); // scope 0 at $DIR/issue-73223.rs:2:23: 2:30
((_1 as Some).0: i32) = const 1_i32; // scope 0 at $DIR/issue-73223.rs:2:23: 2:30
// ty::Const
// + ty: i32
// + val: Value(Scalar(0x00000001))
// mir::Constant
// + span: $DIR/issue-73223.rs:2:28: 2:29
// + literal: Const { ty: i32, val: Value(Scalar(0x00000001)) }
discriminant(_2) = 1; // scope 0 at $DIR/issue-73223.rs:2:23: 2:30
_4 = move _2; // scope 1 at $DIR/issue-73223.rs:7:17: 7:28
StorageDead(_2); // scope 0 at $DIR/issue-73223.rs:5:6: 5:7
StorageLive(_4); // scope 1 at $DIR/issue-73223.rs:7:9: 7:14
discriminant(_1) = 1; // scope 0 at $DIR/issue-73223.rs:2:23: 2:30
_2 = ((_1 as Some).0: i32); // scope 0 at $DIR/issue-73223.rs:3:14: 3:15
StorageDead(_1); // scope 0 at $DIR/issue-73223.rs:5:6: 5:7
StorageLive(_3); // scope 1 at $DIR/issue-73223.rs:7:9: 7:14
StorageLive(_4); // scope 1 at $DIR/issue-73223.rs:7:22: 7:27
_4 = _2; // scope 1 at $DIR/issue-73223.rs:7:22: 7:27
((_3 as Some).0: i32) = move _4; // scope 1 at $DIR/issue-73223.rs:7:17: 7:28
discriminant(_3) = 1; // scope 1 at $DIR/issue-73223.rs:7:17: 7:28
StorageDead(_4); // scope 1 at $DIR/issue-73223.rs:7:27: 7:28
StorageLive(_5); // scope 3 at $SRC_DIR/libcore/macros/mod.rs:LL:COL
StorageLive(_6); // scope 3 at $SRC_DIR/libcore/macros/mod.rs:LL:COL
_6 = &_1; // scope 3 at $SRC_DIR/libcore/macros/mod.rs:LL:COL
_6 = &_2; // scope 3 at $SRC_DIR/libcore/macros/mod.rs:LL:COL
(_5.0: &i32) = move _6; // scope 3 at $SRC_DIR/libcore/macros/mod.rs:LL:COL
(_5.1: &i32) = const main::promoted[1]; // scope 3 at $SRC_DIR/libcore/macros/mod.rs:LL:COL
// ty::Const
@@ -127,8 +131,7 @@
// mir::Constant
// + span: $DIR/issue-73223.rs:1:11: 9:2
// + literal: Const { ty: (), val: Value(Scalar(<ZST>)) }
StorageDead(_4); // scope 1 at $DIR/issue-73223.rs:9:1: 9:2
StorageDead(_1); // scope 0 at $DIR/issue-73223.rs:9:1: 9:2
StorageDead(_3); // scope 1 at $DIR/issue-73223.rs:9:1: 9:2
return; // scope 0 at $DIR/issue-73223.rs:9:2: 9:2
}

19 changes: 9 additions & 10 deletions src/test/mir-opt/issue-73223/32bit/rustc.main.SimplifyArmIdentity.diff
View file
Original file line number Diff line number Diff line change
@@ -134,18 +134,17 @@
}

bb2: {
- StorageLive(_4); // scope 0 at $DIR/issue-73223.rs:3:14: 3:15
- _4 = ((_2 as Some).0: i32); // scope 0 at $DIR/issue-73223.rs:3:14: 3:15
- _1 = _4; // scope 2 at $DIR/issue-73223.rs:3:20: 3:21
- StorageDead(_4); // scope 0 at $DIR/issue-73223.rs:3:21: 3:22
+ _6 = move _2; // scope 1 at $DIR/issue-73223.rs:7:17: 7:28
StorageLive(_4); // scope 0 at $DIR/issue-73223.rs:3:14: 3:15
_4 = ((_2 as Some).0: i32); // scope 0 at $DIR/issue-73223.rs:3:14: 3:15
_1 = _4; // scope 2 at $DIR/issue-73223.rs:3:20: 3:21
StorageDead(_4); // scope 0 at $DIR/issue-73223.rs:3:21: 3:22
StorageDead(_2); // scope 0 at $DIR/issue-73223.rs:5:6: 5:7
StorageLive(_6); // scope 1 at $DIR/issue-73223.rs:7:9: 7:14
- StorageLive(_7); // scope 1 at $DIR/issue-73223.rs:7:22: 7:27
- _7 = _1; // scope 1 at $DIR/issue-73223.rs:7:22: 7:27
- ((_6 as Some).0: i32) = move _7; // scope 1 at $DIR/issue-73223.rs:7:17: 7:28
- discriminant(_6) = 1; // scope 1 at $DIR/issue-73223.rs:7:17: 7:28
- StorageDead(_7); // scope 1 at $DIR/issue-73223.rs:7:27: 7:28
StorageLive(_7); // scope 1 at $DIR/issue-73223.rs:7:22: 7:27
_7 = _1; // scope 1 at $DIR/issue-73223.rs:7:22: 7:27
((_6 as Some).0: i32) = move _7; // scope 1 at $DIR/issue-73223.rs:7:17: 7:28
discriminant(_6) = 1; // scope 1 at $DIR/issue-73223.rs:7:17: 7:28
StorageDead(_7); // scope 1 at $DIR/issue-73223.rs:7:27: 7:28
StorageLive(_8); // scope 3 at $SRC_DIR/libcore/macros/mod.rs:LL:COL
StorageLive(_9); // scope 3 at $SRC_DIR/libcore/macros/mod.rs:LL:COL
StorageLive(_10); // scope 3 at $SRC_DIR/libcore/macros/mod.rs:LL:COL
37 changes: 20 additions & 17 deletions src/test/mir-opt/issue-73223/64bit/rustc.main.PreCodegen.diff
View file
Original file line number Diff line number Diff line change
@@ -3,9 +3,9 @@

fn main() -> () {
let mut _0: (); // return place in scope 0 at $DIR/issue-73223.rs:1:11: 1:11
let _1: i32; // in scope 0 at $DIR/issue-73223.rs:2:9: 2:14
let mut _2: std::option::Option<i32>; // in scope 0 at $DIR/issue-73223.rs:2:23: 2:30
let _3: i32; // in scope 0 at $DIR/issue-73223.rs:3:14: 3:15
let mut _1: std::option::Option<i32>; // in scope 0 at $DIR/issue-73223.rs:2:23: 2:30
let _2: i32; // in scope 0 at $DIR/issue-73223.rs:3:14: 3:15
let mut _4: i32; // in scope 0 at $DIR/issue-73223.rs:7:22: 7:27
let mut _5: (&i32, &i32); // in scope 0 at $SRC_DIR/libcore/macros/mod.rs:LL:COL
let mut _6: &i32; // in scope 0 at $SRC_DIR/libcore/macros/mod.rs:LL:COL
let mut _9: bool; // in scope 0 at $SRC_DIR/libcore/macros/mod.rs:LL:COL
@@ -28,10 +28,10 @@
let mut _28: std::fmt::ArgumentV1; // in scope 0 at $SRC_DIR/libstd/macros.rs:LL:COL
let mut _29: for<'r, 's, 't0> fn(&'r &i32, &'s mut std::fmt::Formatter<'t0>) -> std::result::Result<(), std::fmt::Error>; // in scope 0 at $SRC_DIR/libcore/macros/mod.rs:LL:COL
scope 1 {
debug split => _1; // in scope 1 at $DIR/issue-73223.rs:2:9: 2:14
let _4: std::option::Option<i32>; // in scope 1 at $DIR/issue-73223.rs:7:9: 7:14
debug split => _2; // in scope 1 at $DIR/issue-73223.rs:2:9: 2:14
let _3: std::option::Option<i32>; // in scope 1 at $DIR/issue-73223.rs:7:9: 7:14
scope 3 {
debug _prev => _4; // in scope 3 at $DIR/issue-73223.rs:7:9: 7:14
debug _prev => _3; // in scope 3 at $DIR/issue-73223.rs:7:9: 7:14
let _7: &i32; // in scope 3 at $SRC_DIR/libcore/macros/mod.rs:LL:COL
let _8: &i32; // in scope 3 at $SRC_DIR/libcore/macros/mod.rs:LL:COL
scope 4 {
@@ -64,30 +64,34 @@
}
}
scope 2 {
debug v => _3; // in scope 2 at $DIR/issue-73223.rs:3:14: 3:15
debug v => _2; // in scope 2 at $DIR/issue-73223.rs:3:14: 3:15
}
scope 7 {
}
scope 9 {
}

bb0: {
StorageLive(_1); // scope 0 at $DIR/issue-73223.rs:2:9: 2:14
StorageLive(_2); // scope 0 at $DIR/issue-73223.rs:2:23: 2:30
((_2 as Some).0: i32) = const 1_i32; // scope 0 at $DIR/issue-73223.rs:2:23: 2:30
StorageLive(_1); // scope 0 at $DIR/issue-73223.rs:2:23: 2:30
((_1 as Some).0: i32) = const 1_i32; // scope 0 at $DIR/issue-73223.rs:2:23: 2:30
// ty::Const
// + ty: i32
// + val: Value(Scalar(0x00000001))
// mir::Constant
// + span: $DIR/issue-73223.rs:2:28: 2:29
// + literal: Const { ty: i32, val: Value(Scalar(0x00000001)) }
discriminant(_2) = 1; // scope 0 at $DIR/issue-73223.rs:2:23: 2:30
_4 = move _2; // scope 1 at $DIR/issue-73223.rs:7:17: 7:28
StorageDead(_2); // scope 0 at $DIR/issue-73223.rs:5:6: 5:7
StorageLive(_4); // scope 1 at $DIR/issue-73223.rs:7:9: 7:14
discriminant(_1) = 1; // scope 0 at $DIR/issue-73223.rs:2:23: 2:30
_2 = ((_1 as Some).0: i32); // scope 0 at $DIR/issue-73223.rs:3:14: 3:15
StorageDead(_1); // scope 0 at $DIR/issue-73223.rs:5:6: 5:7
StorageLive(_3); // scope 1 at $DIR/issue-73223.rs:7:9: 7:14
StorageLive(_4); // scope 1 at $DIR/issue-73223.rs:7:22: 7:27
_4 = _2; // scope 1 at $DIR/issue-73223.rs:7:22: 7:27
((_3 as Some).0: i32) = move _4; // scope 1 at $DIR/issue-73223.rs:7:17: 7:28
discriminant(_3) = 1; // scope 1 at $DIR/issue-73223.rs:7:17: 7:28
StorageDead(_4); // scope 1 at $DIR/issue-73223.rs:7:27: 7:28
StorageLive(_5); // scope 3 at $SRC_DIR/libcore/macros/mod.rs:LL:COL
StorageLive(_6); // scope 3 at $SRC_DIR/libcore/macros/mod.rs:LL:COL
_6 = &_1; // scope 3 at $SRC_DIR/libcore/macros/mod.rs:LL:COL
_6 = &_2; // scope 3 at $SRC_DIR/libcore/macros/mod.rs:LL:COL
(_5.0: &i32) = move _6; // scope 3 at $SRC_DIR/libcore/macros/mod.rs:LL:COL
(_5.1: &i32) = const main::promoted[1]; // scope 3 at $SRC_DIR/libcore/macros/mod.rs:LL:COL
// ty::Const
@@ -127,8 +131,7 @@
// mir::Constant
// + span: $DIR/issue-73223.rs:1:11: 9:2
// + literal: Const { ty: (), val: Value(Scalar(<ZST>)) }
StorageDead(_4); // scope 1 at $DIR/issue-73223.rs:9:1: 9:2
StorageDead(_1); // scope 0 at $DIR/issue-73223.rs:9:1: 9:2
StorageDead(_3); // scope 1 at $DIR/issue-73223.rs:9:1: 9:2
return; // scope 0 at $DIR/issue-73223.rs:9:2: 9:2
}

19 changes: 9 additions & 10 deletions src/test/mir-opt/issue-73223/64bit/rustc.main.SimplifyArmIdentity.diff
View file
Original file line number Diff line number Diff line change
@@ -134,18 +134,17 @@
}

bb2: {
- StorageLive(_4); // scope 0 at $DIR/issue-73223.rs:3:14: 3:15
- _4 = ((_2 as Some).0: i32); // scope 0 at $DIR/issue-73223.rs:3:14: 3:15
- _1 = _4; // scope 2 at $DIR/issue-73223.rs:3:20: 3:21
- StorageDead(_4); // scope 0 at $DIR/issue-73223.rs:3:21: 3:22
+ _6 = move _2; // scope 1 at $DIR/issue-73223.rs:7:17: 7:28
StorageLive(_4); // scope 0 at $DIR/issue-73223.rs:3:14: 3:15
_4 = ((_2 as Some).0: i32); // scope 0 at $DIR/issue-73223.rs:3:14: 3:15
_1 = _4; // scope 2 at $DIR/issue-73223.rs:3:20: 3:21
StorageDead(_4); // scope 0 at $DIR/issue-73223.rs:3:21: 3:22
StorageDead(_2); // scope 0 at $DIR/issue-73223.rs:5:6: 5:7
StorageLive(_6); // scope 1 at $DIR/issue-73223.rs:7:9: 7:14
- StorageLive(_7); // scope 1 at $DIR/issue-73223.rs:7:22: 7:27
- _7 = _1; // scope 1 at $DIR/issue-73223.rs:7:22: 7:27
- ((_6 as Some).0: i32) = move _7; // scope 1 at $DIR/issue-73223.rs:7:17: 7:28
- discriminant(_6) = 1; // scope 1 at $DIR/issue-73223.rs:7:17: 7:28
- StorageDead(_7); // scope 1 at $DIR/issue-73223.rs:7:27: 7:28
StorageLive(_7); // scope 1 at $DIR/issue-73223.rs:7:22: 7:27
_7 = _1; // scope 1 at $DIR/issue-73223.rs:7:22: 7:27
((_6 as Some).0: i32) = move _7; // scope 1 at $DIR/issue-73223.rs:7:17: 7:28
discriminant(_6) = 1; // scope 1 at $DIR/issue-73223.rs:7:17: 7:28
StorageDead(_7); // scope 1 at $DIR/issue-73223.rs:7:27: 7:28
StorageLive(_8); // scope 3 at $SRC_DIR/libcore/macros/mod.rs:LL:COL
StorageLive(_9); // scope 3 at $SRC_DIR/libcore/macros/mod.rs:LL:COL
StorageLive(_10); // scope 3 at $SRC_DIR/libcore/macros/mod.rs:LL:COL
4 changes: 3 additions & 1 deletion ...opt/simplify-locals-removes-unused-discriminant-reads/32bit/rustc.map.SimplifyLocals.diff
View file
Original file line number Diff line number Diff line change
@@ -19,7 +19,9 @@
}

bb1: {
_0 = move _1; // scope 1 at $DIR/simplify-locals-removes-unused-discriminant-reads.rs:4:20: 4:27
_3 = move ((_1 as Some).0: std::boxed::Box<()>); // scope 0 at $DIR/simplify-locals-removes-unused-discriminant-reads.rs:4:14: 4:15
((_0 as Some).0: std::boxed::Box<()>) = move _3; // scope 1 at $DIR/simplify-locals-removes-unused-discriminant-reads.rs:4:20: 4:27
discriminant(_0) = 1; // scope 1 at $DIR/simplify-locals-removes-unused-discriminant-reads.rs:4:20: 4:27
goto -> bb3; // scope 0 at $DIR/simplify-locals-removes-unused-discriminant-reads.rs:2:5: 5:6
}

Loading