Skip to content

Commit

Permalink
[mir-opt] Prevent mis-optimization when SimplifyArmIdentity runs
Browse files Browse the repository at this point in the history 
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
1 parent e3f599c commit 9248d90
Show file tree
Hide file tree
Showing 10 changed files with 192 additions and 97 deletions.
51 changes: 49 additions & 2 deletions src/librustc_mir/transform/simplify_try.rs
View file
Expand Up @@ -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;
Expand Down Expand Up @@ -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 {
Expand Down Expand Up @@ -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...");

Expand Down Expand Up @@ -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: {:?} != {:?}",
Expand Down Expand Up @@ -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;
}
Expand Down Expand Up @@ -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);
Expand Down
37 changes: 20 additions & 17 deletions src/test/mir-opt/issue-73223/32bit/rustc.main.PreCodegen.diff
View file
Expand Up @@ -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
Expand All @@ -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 {
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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
}

Expand Down
19 changes: 9 additions & 10 deletions src/test/mir-opt/issue-73223/32bit/rustc.main.SimplifyArmIdentity.diff
View file
Expand Up @@ -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
Expand Down
37 changes: 20 additions & 17 deletions src/test/mir-opt/issue-73223/64bit/rustc.main.PreCodegen.diff
View file
Expand Up @@ -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
Expand All @@ -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 {
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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
}

Expand Down
19 changes: 9 additions & 10 deletions src/test/mir-opt/issue-73223/64bit/rustc.main.SimplifyArmIdentity.diff
View file
Expand Up @@ -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
Expand Down
4 changes: 3 additions & 1 deletion ...opt/simplify-locals-removes-unused-discriminant-reads/32bit/rustc.map.SimplifyLocals.diff
View file
Expand Up @@ -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
}

Expand Down

0 comments on commit 9248d90

Please sign in to comment.