Skip to content

MatchBranchSimplification: fix equal const bool assignments #75537

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

Merged
merged 6 commits into from
Aug 15, 2020
Merged

MatchBranchSimplification: fix equal const bool assignments #75537

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
c0a811a
Add regression test for matching on u8
JulianKnodt Aug 13, 2020
fd74026
Add fix
JulianKnodt Aug 14, 2020
60d7d28
MatchBranchSimplification: optimize when switching on copy operand
tmiasko Aug 14, 2020
4fae049
MatchBranchSimplification: copy discriminant instead of moving it
tmiasko Aug 14, 2020
1fe2e29
MatchBranchSimplification: fix equal const bool assignments
tmiasko Aug 15, 2020
af9b9e4
MatchBranchSimplification: avoid intermediate vec allocation
tmiasko Aug 15, 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
11 changes: 11 additions & 0 deletions src/librustc_index/vec.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -669,6 +669,17 @@ impl<I: Idx, T> IndexVec<I, T> {
}
}

/// Returns mutable references to three distinct elements or panics otherwise.
#[inline]
pub fn pick3_mut(&mut self, a: I, b: I, c: I) -> (&mut T, &mut T, &mut T) {
let (ai, bi, ci) = (a.index(), b.index(), c.index());
assert!(ai != bi && bi != ci && ci != ai);
let len = self.raw.len();
assert!(ai < len && bi < len && ci < len);
let ptr = self.raw.as_mut_ptr();
unsafe { (&mut *ptr.add(ai), &mut *ptr.add(bi), &mut *ptr.add(ci)) }
}

pub fn convert_index_type<Ix: Idx>(self) -> IndexVec<Ix, T> {
IndexVec { raw: self.raw, _marker: PhantomData }
}
Expand Down
116 changes: 79 additions & 37 deletions src/librustc_mir/transform/match_branches.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,10 +4,37 @@ use rustc_middle::ty::TyCtxt;

pub struct MatchBranchSimplification;

// What's the intent of this pass?
// If one block is found that switches between blocks which both go to the same place
// AND both of these blocks set a similar const in their ->
// condense into 1 block based on discriminant AND goto the destination afterwards
/// If a source block is found that switches between two blocks that are exactly
/// the same modulo const bool assignments (e.g., one assigns true another false
/// to the same place), merge a target block statements into the source block,
/// using Eq / Ne comparison with switch value where const bools value differ.
///
/// For example:
///
/// ```rust
/// bb0: {
/// switchInt(move _3) -> [42_isize: bb1, otherwise: bb2];
/// }
///
/// bb1: {
/// _2 = const true;
/// goto -> bb3;
/// }
///
/// bb2: {
/// _2 = const false;
/// goto -> bb3;
/// }
/// ```
///
/// into:
///
/// ```rust
/// bb0: {
/// _2 = Eq(move _3, const 42_isize);
/// goto -> bb3;
/// }
/// ```

impl<'tcx> MirPass<'tcx> for MatchBranchSimplification {
fn run_pass(&self, tcx: TyCtxt<'tcx>, src: MirSource<'tcx>, body: &mut Body<'tcx>) {
Expand All @@ -16,12 +43,12 @@ impl<'tcx> MirPass<'tcx> for MatchBranchSimplification {
'outer: for bb_idx in bbs.indices() {
let (discr, val, switch_ty, first, second) = match bbs[bb_idx].terminator().kind {
TerminatorKind::SwitchInt {
discr: Operand::Move(ref place),
discr: Operand::Copy(ref place) | Operand::Move(ref place),
switch_ty,
ref targets,
ref values,
..
} if targets.len() == 2 && values.len() == 1 => {
} if targets.len() == 2 && values.len() == 1 && targets[0] != targets[1] => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just curious, is targets[0] == targets[1] ever hit? I imagine that would be a whole different opt pass

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know.

The CfgSimplifier optimizes switch with the same targets to a goto, and then collapses goto chains and merges blocks.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we can make this condition an assert after the beta branch, then we have 12 weeks to find out if it's a problem :D

(place, values[0], switch_ty, targets[0], targets[1])
}
// Only optimize switch int statements
Expand All @@ -42,49 +69,64 @@ impl<'tcx> MirPass<'tcx> for MatchBranchSimplification {
}
for (f, s) in first_stmts.iter().zip(scnd_stmts.iter()) {
match (&f.kind, &s.kind) {
// If two statements are exactly the same just ignore them.
(f_s, s_s) if f_s == s_s => (),
// If two statements are exactly the same, we can optimize.
(f_s, s_s) if f_s == s_s => {}

// If two statements are const bool assignments to the same place, we can optimize.
(
StatementKind::Assign(box (lhs_f, Rvalue::Use(Operand::Constant(f_c)))),
StatementKind::Assign(box (lhs_s, Rvalue::Use(Operand::Constant(s_c)))),
) if lhs_f == lhs_s => {
if let Some(f_c) = f_c.literal.try_eval_bool(tcx, param_env) {
// This should also be a bool because it's writing to the same place
let s_c = s_c.literal.try_eval_bool(tcx, param_env).unwrap();
if f_c != s_c {
// have to check this here because f_c & s_c might have
// different spans.
continue;
}
}
continue 'outer;
}
// If there are not exclusively assignments, then ignore this
) if lhs_f == lhs_s
&& f_c.literal.ty.is_bool()
&& s_c.literal.ty.is_bool()
&& f_c.literal.try_eval_bool(tcx, param_env).is_some()
&& s_c.literal.try_eval_bool(tcx, param_env).is_some() => {}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel like these two stmts will always evaluate to true if the previous two did, but better safe than sorry


// Otherwise we cannot optimize. Try another block.
_ => continue 'outer,
}
}
// Take owenership of items now that we know we can optimize.
// Take ownership of items now that we know we can optimize.
let discr = discr.clone();
let (from, first) = bbs.pick2_mut(bb_idx, first);

let new_stmts = first.statements.iter().cloned().map(|mut s| {
if let StatementKind::Assign(box (_, ref mut rhs)) = s.kind {
if let Rvalue::Use(Operand::Constant(c)) = rhs {
let size = tcx.layout_of(param_env.and(switch_ty)).unwrap().size;
let const_cmp = Operand::const_from_scalar(
tcx,
switch_ty,
crate::interpret::Scalar::from_uint(val, size),
rustc_span::DUMMY_SP,
);
if let Some(c) = c.literal.try_eval_bool(tcx, param_env) {
let op = if c { BinOp::Eq } else { BinOp::Ne };
*rhs = Rvalue::BinaryOp(op, Operand::Move(discr), const_cmp);
// 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);

let new_stmts = first.statements.iter().zip(second.statements.iter()).map(|(f, s)| {
match (&f.kind, &s.kind) {
(f_s, s_s) if f_s == s_s => (*f).clone(),

(
StatementKind::Assign(box (lhs, Rvalue::Use(Operand::Constant(f_c)))),
StatementKind::Assign(box (_, Rvalue::Use(Operand::Constant(s_c)))),
) => {
// From earlier loop we know that we are dealing with bool constants only:
let f_b = f_c.literal.try_eval_bool(tcx, param_env).unwrap();
let s_b = s_c.literal.try_eval_bool(tcx, param_env).unwrap();
if f_b == s_b {
// Same value in both blocks. Use statement as is.
(*f).clone()
} else {
// Different value between blocks. Make value conditional on switch condition.
let size = tcx.layout_of(param_env.and(switch_ty)).unwrap().size;
let const_cmp = Operand::const_from_scalar(
tcx,
switch_ty,
crate::interpret::Scalar::from_uint(val, size),
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);
Statement {
source_info: f.source_info,
kind: StatementKind::Assign(box (*lhs, rhs)),
}
}
}

_ => unreachable!(),
}
s
});
from.statements.extend(new_stmts);
from.terminator_mut().kind = first.terminator().kind.clone();
Expand Down
156 changes: 156 additions & 0 deletions src/test/mir-opt/matches_reduce_branches.bar.MatchBranchSimplification.diff.32bit
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,156 @@
- // MIR for `bar` before MatchBranchSimplification
+ // MIR for `bar` after MatchBranchSimplification

fn bar(_1: i32) -> (bool, bool, bool, bool) {
debug i => _1; // in scope 0 at $DIR/matches_reduce_branches.rs:11:8: 11:9
let mut _0: (bool, bool, bool, bool); // return place in scope 0 at $DIR/matches_reduce_branches.rs:11:19: 11:43
let _2: bool; // in scope 0 at $DIR/matches_reduce_branches.rs:12:9: 12:10
let _6: (); // in scope 0 at $DIR/matches_reduce_branches.rs:17:5: 32:6
let mut _7: bool; // in scope 0 at $DIR/matches_reduce_branches.rs:34:6: 34:7
let mut _8: bool; // in scope 0 at $DIR/matches_reduce_branches.rs:34:9: 34:10
let mut _9: bool; // in scope 0 at $DIR/matches_reduce_branches.rs:34:12: 34:13
let mut _10: bool; // in scope 0 at $DIR/matches_reduce_branches.rs:34:15: 34:16
scope 1 {
debug a => _2; // in scope 1 at $DIR/matches_reduce_branches.rs:12:9: 12:10
let _3: bool; // in scope 1 at $DIR/matches_reduce_branches.rs:13:9: 13:10
scope 2 {
debug b => _3; // in scope 2 at $DIR/matches_reduce_branches.rs:13:9: 13:10
let _4: bool; // in scope 2 at $DIR/matches_reduce_branches.rs:14:9: 14:10
scope 3 {
debug c => _4; // in scope 3 at $DIR/matches_reduce_branches.rs:14:9: 14:10
let _5: bool; // in scope 3 at $DIR/matches_reduce_branches.rs:15:9: 15:10
scope 4 {
debug d => _5; // in scope 4 at $DIR/matches_reduce_branches.rs:15:9: 15:10
}
}
}
}

bb0: {
StorageLive(_2); // scope 0 at $DIR/matches_reduce_branches.rs:12:9: 12:10
StorageLive(_3); // scope 1 at $DIR/matches_reduce_branches.rs:13:9: 13:10
StorageLive(_4); // scope 2 at $DIR/matches_reduce_branches.rs:14:9: 14:10
StorageLive(_5); // scope 3 at $DIR/matches_reduce_branches.rs:15:9: 15:10
StorageLive(_6); // scope 4 at $DIR/matches_reduce_branches.rs:17:5: 32:6
- switchInt(_1) -> [7_i32: bb2, otherwise: bb1]; // scope 4 at $DIR/matches_reduce_branches.rs:18:9: 18:10
+ _2 = Ne(_1, const 7_i32); // scope 4 at $DIR/matches_reduce_branches.rs:19:13: 19:22
+ // ty::Const
+ // + ty: i32
+ // + val: Value(Scalar(0x00000007))
+ // mir::Constant
+ // + span: $DIR/matches_reduce_branches.rs:1:1: 1:1
+ // + literal: Const { ty: i32, val: Value(Scalar(0x00000007)) }
+ _3 = Eq(_1, const 7_i32); // scope 4 at $DIR/matches_reduce_branches.rs:20:13: 20:21
+ // ty::Const
+ // + ty: i32
+ // + val: Value(Scalar(0x00000007))
+ // mir::Constant
+ // + span: $DIR/matches_reduce_branches.rs:1:1: 1:1
+ // + literal: Const { ty: i32, val: Value(Scalar(0x00000007)) }
+ _4 = const false; // scope 4 at $DIR/matches_reduce_branches.rs:21:13: 21:22
+ // ty::Const
+ // + ty: bool
+ // + val: Value(Scalar(0x00))
+ // mir::Constant
+ // + span: $DIR/matches_reduce_branches.rs:21:17: 21:22
+ // + literal: Const { ty: bool, val: Value(Scalar(0x00)) }
+ _5 = const true; // scope 4 at $DIR/matches_reduce_branches.rs:22:13: 22:21
+ // ty::Const
+ // + ty: bool
+ // + val: Value(Scalar(0x01))
+ // mir::Constant
+ // + span: $DIR/matches_reduce_branches.rs:22:17: 22:21
+ // + literal: Const { ty: bool, val: Value(Scalar(0x01)) }
+ goto -> bb3; // scope 4 at $DIR/matches_reduce_branches.rs:18:9: 18:10
}

bb1: {
_2 = const true; // scope 4 at $DIR/matches_reduce_branches.rs:26:13: 26:21
// ty::Const
// + ty: bool
// + val: Value(Scalar(0x01))
// mir::Constant
// + span: $DIR/matches_reduce_branches.rs:26:17: 26:21
// + literal: Const { ty: bool, val: Value(Scalar(0x01)) }
_3 = const false; // scope 4 at $DIR/matches_reduce_branches.rs:27:13: 27:22
// ty::Const
// + ty: bool
// + val: Value(Scalar(0x00))
// mir::Constant
// + span: $DIR/matches_reduce_branches.rs:27:17: 27:22
// + literal: Const { ty: bool, val: Value(Scalar(0x00)) }
_4 = const false; // scope 4 at $DIR/matches_reduce_branches.rs:28:13: 28:22
// ty::Const
// + ty: bool
// + val: Value(Scalar(0x00))
// mir::Constant
// + span: $DIR/matches_reduce_branches.rs:28:17: 28:22
// + literal: Const { ty: bool, val: Value(Scalar(0x00)) }
_5 = const true; // scope 4 at $DIR/matches_reduce_branches.rs:29:13: 29:21
// ty::Const
// + ty: bool
// + val: Value(Scalar(0x01))
// mir::Constant
// + span: $DIR/matches_reduce_branches.rs:29:17: 29:21
// + literal: Const { ty: bool, val: Value(Scalar(0x01)) }
goto -> bb3; // scope 4 at $DIR/matches_reduce_branches.rs:17:5: 32:6
}

bb2: {
_2 = const false; // scope 4 at $DIR/matches_reduce_branches.rs:19:13: 19:22
// ty::Const
// + ty: bool
// + val: Value(Scalar(0x00))
// mir::Constant
// + span: $DIR/matches_reduce_branches.rs:19:17: 19:22
// + literal: Const { ty: bool, val: Value(Scalar(0x00)) }
_3 = const true; // scope 4 at $DIR/matches_reduce_branches.rs:20:13: 20:21
// ty::Const
// + ty: bool
// + val: Value(Scalar(0x01))
// mir::Constant
// + span: $DIR/matches_reduce_branches.rs:20:17: 20:21
// + literal: Const { ty: bool, val: Value(Scalar(0x01)) }
_4 = const false; // scope 4 at $DIR/matches_reduce_branches.rs:21:13: 21:22
// ty::Const
// + ty: bool
// + val: Value(Scalar(0x00))
// mir::Constant
// + span: $DIR/matches_reduce_branches.rs:21:17: 21:22
// + literal: Const { ty: bool, val: Value(Scalar(0x00)) }
_5 = const true; // scope 4 at $DIR/matches_reduce_branches.rs:22:13: 22:21
// ty::Const
// + ty: bool
// + val: Value(Scalar(0x01))
// mir::Constant
// + span: $DIR/matches_reduce_branches.rs:22:17: 22:21
// + literal: Const { ty: bool, val: Value(Scalar(0x01)) }
goto -> bb3; // scope 4 at $DIR/matches_reduce_branches.rs:17:5: 32:6
}

bb3: {
StorageDead(_6); // scope 4 at $DIR/matches_reduce_branches.rs:32:6: 32:7
StorageLive(_7); // scope 4 at $DIR/matches_reduce_branches.rs:34:6: 34:7
_7 = _2; // scope 4 at $DIR/matches_reduce_branches.rs:34:6: 34:7
StorageLive(_8); // scope 4 at $DIR/matches_reduce_branches.rs:34:9: 34:10
_8 = _3; // scope 4 at $DIR/matches_reduce_branches.rs:34:9: 34:10
StorageLive(_9); // scope 4 at $DIR/matches_reduce_branches.rs:34:12: 34:13
_9 = _4; // scope 4 at $DIR/matches_reduce_branches.rs:34:12: 34:13
StorageLive(_10); // scope 4 at $DIR/matches_reduce_branches.rs:34:15: 34:16
_10 = _5; // scope 4 at $DIR/matches_reduce_branches.rs:34:15: 34:16
(_0.0: bool) = move _7; // scope 4 at $DIR/matches_reduce_branches.rs:34:5: 34:17
(_0.1: bool) = move _8; // scope 4 at $DIR/matches_reduce_branches.rs:34:5: 34:17
(_0.2: bool) = move _9; // scope 4 at $DIR/matches_reduce_branches.rs:34:5: 34:17
(_0.3: bool) = move _10; // scope 4 at $DIR/matches_reduce_branches.rs:34:5: 34:17
StorageDead(_10); // scope 4 at $DIR/matches_reduce_branches.rs:34:16: 34:17
StorageDead(_9); // scope 4 at $DIR/matches_reduce_branches.rs:34:16: 34:17
StorageDead(_8); // scope 4 at $DIR/matches_reduce_branches.rs:34:16: 34:17
StorageDead(_7); // scope 4 at $DIR/matches_reduce_branches.rs:34:16: 34:17
StorageDead(_5); // scope 3 at $DIR/matches_reduce_branches.rs:35:1: 35:2
StorageDead(_4); // scope 2 at $DIR/matches_reduce_branches.rs:35:1: 35:2
StorageDead(_3); // scope 1 at $DIR/matches_reduce_branches.rs:35:1: 35:2
StorageDead(_2); // scope 0 at $DIR/matches_reduce_branches.rs:35:1: 35:2
return; // scope 0 at $DIR/matches_reduce_branches.rs:35:2: 35:2
}
}

Loading