Skip to content

Commit

Permalink
Introduce a temporary for discriminant value in MatchBranchSimplifica…
Browse files Browse the repository at this point in the history 
…tion

The optimization introduces additional uses of the discriminant operand, but
does not ensure that it is still valid to evaluate it or that it still
evaluates to the same value.

Evaluate it once at original position, and store the result in a new temporary.
  • Loading branch information
@tmiasko
tmiasko committed Oct 21, 2020
1 parent f965120 commit a4dc92b
Show file tree
Hide file tree
Showing 9 changed files with 309 additions and 48 deletions.
30 changes: 22 additions & 8 deletions compiler/rustc_mir/src/transform/match_branches.rs
View file
Expand Up @@ -38,19 +38,16 @@ pub struct MatchBranchSimplification;

impl<'tcx> MirPass<'tcx> for MatchBranchSimplification {
fn run_pass(&self, tcx: TyCtxt<'tcx>, body: &mut Body<'tcx>) {
// FIXME: This optimization can result in unsoundness, because it introduces
// additional uses of a place holding the discriminant value without ensuring that
// it is valid to do so.
if !tcx.sess.opts.debugging_opts.unsound_mir_opts {
if tcx.sess.opts.debugging_opts.mir_opt_level <= 1 {
return;
}

let param_env = tcx.param_env(body.source.def_id());
let bbs = body.basic_blocks_mut();
let (bbs, local_decls) = body.basic_blocks_and_local_decls_mut();
'outer: for bb_idx in bbs.indices() {
let (discr, val, switch_ty, first, second) = match bbs[bb_idx].terminator().kind {
TerminatorKind::SwitchInt {
discr: Operand::Copy(ref place) | Operand::Move(ref place),
discr: ref discr @ (Operand::Copy(_) | Operand::Move(_)),
switch_ty,
ref targets,
..
Expand All @@ -59,7 +56,7 @@ impl<'tcx> MirPass<'tcx> for MatchBranchSimplification {
if target == targets.otherwise() {
continue;
}
(place, value, switch_ty, target, targets.otherwise())
(discr, value, switch_ty, target, targets.otherwise())
}
// Only optimize switch int statements
_ => continue,
Expand Down Expand Up @@ -99,6 +96,10 @@ impl<'tcx> MirPass<'tcx> for MatchBranchSimplification {
// Take ownership of items now that we know we can optimize.
let discr = discr.clone();

// Introduce a temporary for the discriminant value.
let source_info = bbs[bb_idx].terminator().source_info;
let discr_local = local_decls.push(LocalDecl::new(switch_ty, source_info.span));

// We already checked that first and second are different blocks,
// and bb_idx has a different terminator from both of them.
let (from, first, second) = bbs.pick3_mut(bb_idx, first, second);
Expand Down Expand Up @@ -127,7 +128,11 @@ impl<'tcx> MirPass<'tcx> for MatchBranchSimplification {
rustc_span::DUMMY_SP,
);
let op = if f_b { BinOp::Eq } else { BinOp::Ne };
let rhs = Rvalue::BinaryOp(op, Operand::Copy(discr.clone()), const_cmp);
let rhs = Rvalue::BinaryOp(
op,
Operand::Copy(Place::from(discr_local)),
const_cmp,
);
Statement {
source_info: f.source_info,
kind: StatementKind::Assign(box (*lhs, rhs)),
Expand All @@ -138,7 +143,16 @@ impl<'tcx> MirPass<'tcx> for MatchBranchSimplification {
_ => unreachable!(),
}
});

from.statements
.push(Statement { source_info, kind: StatementKind::StorageLive(discr_local) });
from.statements.push(Statement {
source_info,
kind: StatementKind::Assign(box (Place::from(discr_local), Rvalue::Use(discr))),
});
from.statements.extend(new_stmts);
from.statements
.push(Statement { source_info, kind: StatementKind::StorageDead(discr_local) });
from.terminator_mut().kind = first.terminator().kind.clone();
}
}
Expand Down
8 changes: 6 additions & 2 deletions src/test/mir-opt/matches_reduce_branches.bar.MatchBranchSimplification.32bit.diff
View file
Expand Up @@ -10,6 +10,7 @@
let mut _8: bool; // in scope 0 at $DIR/matches_reduce_branches.rs:35:9: 35:10
let mut _9: bool; // in scope 0 at $DIR/matches_reduce_branches.rs:35:12: 35:13
let mut _10: bool; // in scope 0 at $DIR/matches_reduce_branches.rs:35:15: 35:16
+ let mut _11: i32; // in scope 0 at $DIR/matches_reduce_branches.rs:19:9: 19:10
scope 1 {
debug a => _2; // in scope 1 at $DIR/matches_reduce_branches.rs:13:9: 13:10
let _3: bool; // in scope 1 at $DIR/matches_reduce_branches.rs:14:9: 14:10
Expand All @@ -33,10 +34,13 @@
StorageLive(_5); // scope 3 at $DIR/matches_reduce_branches.rs:16:9: 16:10
StorageLive(_6); // scope 4 at $DIR/matches_reduce_branches.rs:18:5: 33:6
- switchInt(_1) -> [7_i32: bb2, otherwise: bb1]; // scope 4 at $DIR/matches_reduce_branches.rs:19:9: 19:10
+ _2 = Ne(_1, const 7_i32); // scope 4 at $DIR/matches_reduce_branches.rs:20:13: 20:22
+ _3 = Eq(_1, const 7_i32); // scope 4 at $DIR/matches_reduce_branches.rs:21:13: 21:21
+ StorageLive(_11); // scope 4 at $DIR/matches_reduce_branches.rs:19:9: 19:10
+ _11 = _1; // scope 4 at $DIR/matches_reduce_branches.rs:19:9: 19:10
+ _2 = Ne(_11, const 7_i32); // scope 4 at $DIR/matches_reduce_branches.rs:20:13: 20:22
+ _3 = Eq(_11, const 7_i32); // scope 4 at $DIR/matches_reduce_branches.rs:21:13: 21:21
+ _4 = const false; // scope 4 at $DIR/matches_reduce_branches.rs:22:13: 22:22
+ _5 = const true; // scope 4 at $DIR/matches_reduce_branches.rs:23:13: 23:21
+ StorageDead(_11); // scope 4 at $DIR/matches_reduce_branches.rs:19:9: 19:10
+ goto -> bb3; // scope 4 at $DIR/matches_reduce_branches.rs:19:9: 19:10
}

Expand Down
8 changes: 6 additions & 2 deletions src/test/mir-opt/matches_reduce_branches.bar.MatchBranchSimplification.64bit.diff
View file
Expand Up @@ -10,6 +10,7 @@
let mut _8: bool; // in scope 0 at $DIR/matches_reduce_branches.rs:35:9: 35:10
let mut _9: bool; // in scope 0 at $DIR/matches_reduce_branches.rs:35:12: 35:13
let mut _10: bool; // in scope 0 at $DIR/matches_reduce_branches.rs:35:15: 35:16
+ let mut _11: i32; // in scope 0 at $DIR/matches_reduce_branches.rs:19:9: 19:10
scope 1 {
debug a => _2; // in scope 1 at $DIR/matches_reduce_branches.rs:13:9: 13:10
let _3: bool; // in scope 1 at $DIR/matches_reduce_branches.rs:14:9: 14:10
Expand All @@ -33,10 +34,13 @@
StorageLive(_5); // scope 3 at $DIR/matches_reduce_branches.rs:16:9: 16:10
StorageLive(_6); // scope 4 at $DIR/matches_reduce_branches.rs:18:5: 33:6
- switchInt(_1) -> [7_i32: bb2, otherwise: bb1]; // scope 4 at $DIR/matches_reduce_branches.rs:19:9: 19:10
+ _2 = Ne(_1, const 7_i32); // scope 4 at $DIR/matches_reduce_branches.rs:20:13: 20:22
+ _3 = Eq(_1, const 7_i32); // scope 4 at $DIR/matches_reduce_branches.rs:21:13: 21:21
+ StorageLive(_11); // scope 4 at $DIR/matches_reduce_branches.rs:19:9: 19:10
+ _11 = _1; // scope 4 at $DIR/matches_reduce_branches.rs:19:9: 19:10
+ _2 = Ne(_11, const 7_i32); // scope 4 at $DIR/matches_reduce_branches.rs:20:13: 20:22
+ _3 = Eq(_11, const 7_i32); // scope 4 at $DIR/matches_reduce_branches.rs:21:13: 21:21
+ _4 = const false; // scope 4 at $DIR/matches_reduce_branches.rs:22:13: 22:22
+ _5 = const true; // scope 4 at $DIR/matches_reduce_branches.rs:23:13: 23:21
+ StorageDead(_11); // scope 4 at $DIR/matches_reduce_branches.rs:19:9: 19:10
+ goto -> bb3; // scope 4 at $DIR/matches_reduce_branches.rs:19:9: 19:10
}

Expand Down
6 changes: 5 additions & 1 deletion src/test/mir-opt/matches_reduce_branches.foo.MatchBranchSimplification.32bit.diff
View file
Expand Up @@ -6,12 +6,16 @@
let mut _0: (); // return place in scope 0 at $DIR/matches_reduce_branches.rs:6:25: 6:25
let mut _2: bool; // in scope 0 at $SRC_DIR/core/src/macros/mod.rs:LL:COL
let mut _3: isize; // in scope 0 at $DIR/matches_reduce_branches.rs:7:22: 7:26
+ let mut _4: isize; // in scope 0 at $DIR/matches_reduce_branches.rs:7:22: 7:26

bb0: {
StorageLive(_2); // scope 0 at $SRC_DIR/core/src/macros/mod.rs:LL:COL
_3 = discriminant(_1); // scope 0 at $DIR/matches_reduce_branches.rs:7:22: 7:26
- switchInt(move _3) -> [0_isize: bb2, otherwise: bb1]; // scope 0 at $DIR/matches_reduce_branches.rs:7:22: 7:26
+ _2 = Eq(_3, const 0_isize); // scope 0 at $SRC_DIR/core/src/macros/mod.rs:LL:COL
+ StorageLive(_4); // scope 0 at $DIR/matches_reduce_branches.rs:7:22: 7:26
+ _4 = move _3; // scope 0 at $DIR/matches_reduce_branches.rs:7:22: 7:26
+ _2 = Eq(_4, const 0_isize); // scope 0 at $SRC_DIR/core/src/macros/mod.rs:LL:COL
+ StorageDead(_4); // scope 0 at $DIR/matches_reduce_branches.rs:7:22: 7:26
+ goto -> bb3; // scope 0 at $DIR/matches_reduce_branches.rs:7:22: 7:26
}

Expand Down
6 changes: 5 additions & 1 deletion src/test/mir-opt/matches_reduce_branches.foo.MatchBranchSimplification.64bit.diff
View file
Expand Up @@ -6,12 +6,16 @@
let mut _0: (); // return place in scope 0 at $DIR/matches_reduce_branches.rs:6:25: 6:25
let mut _2: bool; // in scope 0 at $SRC_DIR/core/src/macros/mod.rs:LL:COL
let mut _3: isize; // in scope 0 at $DIR/matches_reduce_branches.rs:7:22: 7:26
+ let mut _4: isize; // in scope 0 at $DIR/matches_reduce_branches.rs:7:22: 7:26

bb0: {
StorageLive(_2); // scope 0 at $SRC_DIR/core/src/macros/mod.rs:LL:COL
_3 = discriminant(_1); // scope 0 at $DIR/matches_reduce_branches.rs:7:22: 7:26
- switchInt(move _3) -> [0_isize: bb2, otherwise: bb1]; // scope 0 at $DIR/matches_reduce_branches.rs:7:22: 7:26
+ _2 = Eq(_3, const 0_isize); // scope 0 at $SRC_DIR/core/src/macros/mod.rs:LL:COL
+ StorageLive(_4); // scope 0 at $DIR/matches_reduce_branches.rs:7:22: 7:26
+ _4 = move _3; // scope 0 at $DIR/matches_reduce_branches.rs:7:22: 7:26
+ _2 = Eq(_4, const 0_isize); // scope 0 at $SRC_DIR/core/src/macros/mod.rs:LL:COL
+ StorageDead(_4); // scope 0 at $DIR/matches_reduce_branches.rs:7:22: 7:26
+ goto -> bb3; // scope 0 at $DIR/matches_reduce_branches.rs:7:22: 7:26
}

Expand Down
116 changes: 116 additions & 0 deletions ...test/mir-opt/matches_reduce_branches.match_nested_if.MatchBranchSimplification.32bit.diff
View file
@@ -0,0 +1,116 @@
- // MIR for `match_nested_if` before MatchBranchSimplification
+ // MIR for `match_nested_if` after MatchBranchSimplification

fn match_nested_if() -> bool {
let mut _0: bool; // return place in scope 0 at $DIR/matches_reduce_branches.rs:38:25: 38:29
let _1: bool; // in scope 0 at $DIR/matches_reduce_branches.rs:39:9: 39:12
let mut _2: (); // in scope 0 at $DIR/matches_reduce_branches.rs:39:21: 39:23
let mut _3: bool; // in scope 0 at $DIR/matches_reduce_branches.rs:40:15: 40:88
let mut _4: bool; // in scope 0 at $DIR/matches_reduce_branches.rs:40:18: 40:68
let mut _5: bool; // in scope 0 at $DIR/matches_reduce_branches.rs:40:21: 40:48
let mut _6: bool; // in scope 0 at $DIR/matches_reduce_branches.rs:40:24: 40:28
+ let mut _7: bool; // in scope 0 at $DIR/matches_reduce_branches.rs:40:21: 40:48
+ let mut _8: bool; // in scope 0 at $DIR/matches_reduce_branches.rs:40:18: 40:68
+ let mut _9: bool; // in scope 0 at $DIR/matches_reduce_branches.rs:40:15: 40:88
+ let mut _10: bool; // in scope 0 at $DIR/matches_reduce_branches.rs:40:15: 40:88
scope 1 {
debug val => _1; // in scope 1 at $DIR/matches_reduce_branches.rs:39:9: 39:12
}

bb0: {
StorageLive(_1); // scope 0 at $DIR/matches_reduce_branches.rs:39:9: 39:12
StorageLive(_2); // scope 0 at $DIR/matches_reduce_branches.rs:39:21: 39:23
StorageLive(_3); // scope 0 at $DIR/matches_reduce_branches.rs:40:15: 40:88
StorageLive(_4); // scope 0 at $DIR/matches_reduce_branches.rs:40:18: 40:68
StorageLive(_5); // scope 0 at $DIR/matches_reduce_branches.rs:40:21: 40:48
StorageLive(_6); // scope 0 at $DIR/matches_reduce_branches.rs:40:24: 40:28
_6 = const true; // scope 0 at $DIR/matches_reduce_branches.rs:40:24: 40:28
- switchInt(_6) -> [false: bb2, otherwise: bb1]; // scope 0 at $DIR/matches_reduce_branches.rs:40:21: 40:48
+ StorageLive(_7); // scope 0 at $DIR/matches_reduce_branches.rs:40:21: 40:48
+ _7 = _6; // scope 0 at $DIR/matches_reduce_branches.rs:40:21: 40:48
+ _5 = Ne(_7, const false); // scope 0 at $DIR/matches_reduce_branches.rs:40:42: 40:47
+ StorageDead(_7); // scope 0 at $DIR/matches_reduce_branches.rs:40:21: 40:48
+ goto -> bb3; // scope 0 at $DIR/matches_reduce_branches.rs:40:21: 40:48
}

bb1: {
_5 = const true; // scope 0 at $DIR/matches_reduce_branches.rs:40:30: 40:34
goto -> bb3; // scope 0 at $DIR/matches_reduce_branches.rs:40:21: 40:48
}

bb2: {
_5 = const false; // scope 0 at $DIR/matches_reduce_branches.rs:40:42: 40:47
goto -> bb3; // scope 0 at $DIR/matches_reduce_branches.rs:40:21: 40:48
}

bb3: {
StorageDead(_6); // scope 0 at $DIR/matches_reduce_branches.rs:40:47: 40:48
- switchInt(_5) -> [false: bb4, otherwise: bb5]; // scope 0 at $DIR/matches_reduce_branches.rs:40:18: 40:68
+ StorageLive(_8); // scope 0 at $DIR/matches_reduce_branches.rs:40:18: 40:68
+ _8 = _5; // scope 0 at $DIR/matches_reduce_branches.rs:40:18: 40:68
+ _4 = Ne(_8, const false); // scope 0 at $DIR/matches_reduce_branches.rs:40:62: 40:67
+ StorageDead(_8); // scope 0 at $DIR/matches_reduce_branches.rs:40:18: 40:68
+ goto -> bb6; // scope 0 at $DIR/matches_reduce_branches.rs:40:18: 40:68
}

bb4: {
_4 = const false; // scope 0 at $DIR/matches_reduce_branches.rs:40:62: 40:67
goto -> bb6; // scope 0 at $DIR/matches_reduce_branches.rs:40:18: 40:68
}

bb5: {
_4 = const true; // scope 0 at $DIR/matches_reduce_branches.rs:40:50: 40:54
goto -> bb6; // scope 0 at $DIR/matches_reduce_branches.rs:40:18: 40:68
}

bb6: {
StorageDead(_5); // scope 0 at $DIR/matches_reduce_branches.rs:40:67: 40:68
- switchInt(_4) -> [false: bb7, otherwise: bb8]; // scope 0 at $DIR/matches_reduce_branches.rs:40:15: 40:88
+ StorageLive(_9); // scope 0 at $DIR/matches_reduce_branches.rs:40:15: 40:88
+ _9 = _4; // scope 0 at $DIR/matches_reduce_branches.rs:40:15: 40:88
+ _3 = Ne(_9, const false); // scope 0 at $DIR/matches_reduce_branches.rs:40:82: 40:87
+ StorageDead(_9); // scope 0 at $DIR/matches_reduce_branches.rs:40:15: 40:88
+ goto -> bb9; // scope 0 at $DIR/matches_reduce_branches.rs:40:15: 40:88
}

bb7: {
_3 = const false; // scope 0 at $DIR/matches_reduce_branches.rs:40:82: 40:87
goto -> bb9; // scope 0 at $DIR/matches_reduce_branches.rs:40:15: 40:88
}

bb8: {
_3 = const true; // scope 0 at $DIR/matches_reduce_branches.rs:40:70: 40:74
goto -> bb9; // scope 0 at $DIR/matches_reduce_branches.rs:40:15: 40:88
}

bb9: {
StorageDead(_4); // scope 0 at $DIR/matches_reduce_branches.rs:40:87: 40:88
- switchInt(move _3) -> [false: bb11, otherwise: bb10]; // scope 0 at $DIR/matches_reduce_branches.rs:40:15: 40:88
+ StorageLive(_10); // scope 0 at $DIR/matches_reduce_branches.rs:40:15: 40:88
+ _10 = move _3; // scope 0 at $DIR/matches_reduce_branches.rs:40:15: 40:88
+ StorageDead(_3); // scope 0 at $DIR/matches_reduce_branches.rs:40:95: 40:96
+ _1 = Ne(_10, const false); // scope 0 at $DIR/matches_reduce_branches.rs:41:14: 41:19
+ StorageDead(_10); // scope 0 at $DIR/matches_reduce_branches.rs:40:15: 40:88
+ goto -> bb12; // scope 0 at $DIR/matches_reduce_branches.rs:40:15: 40:88
}

bb10: {
StorageDead(_3); // scope 0 at $DIR/matches_reduce_branches.rs:40:95: 40:96
_1 = const true; // scope 0 at $DIR/matches_reduce_branches.rs:40:92: 40:96
goto -> bb12; // scope 0 at $DIR/matches_reduce_branches.rs:39:15: 42:6
}

bb11: {
StorageDead(_3); // scope 0 at $DIR/matches_reduce_branches.rs:40:95: 40:96
_1 = const false; // scope 0 at $DIR/matches_reduce_branches.rs:41:14: 41:19
goto -> bb12; // scope 0 at $DIR/matches_reduce_branches.rs:39:15: 42:6
}

bb12: {
StorageDead(_2); // scope 0 at $DIR/matches_reduce_branches.rs:42:6: 42:7
_0 = _1; // scope 1 at $DIR/matches_reduce_branches.rs:43:5: 43:8
StorageDead(_1); // scope 0 at $DIR/matches_reduce_branches.rs:44:1: 44:2
return; // scope 0 at $DIR/matches_reduce_branches.rs:44:2: 44:2
}
}

0 comments on commit a4dc92b

Please sign in to comment.