Skip to content

MIR-OPT: Less conservative EarlyOtherwiseBranch #77163

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

Closed
wants to merge 6 commits into from
Closed

MIR-OPT: Less conservative EarlyOtherwiseBranch #77163

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
081074b
Make the EarlyOtherwiseBranch optimization less conservative
simonvandel Sep 24, 2020
4a6b728
Recorder checks - from intuitively cheapest to most expensive
simonvandel Sep 24, 2020
3d52e54
Don't need to look at the layout of the types being matched on
simonvandel Sep 30, 2020
930898b
Do not invent new basic block - just jump to already existing bb
simonvandel Sep 30, 2020
03e1814
remove unused fields
simonvandel Sep 30, 2020
67a7013
Add back basic block to disambiguate where to go
simonvandel Oct 1, 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
91 changes: 51 additions & 40 deletions compiler/rustc_mir/src/transform/early_otherwise_branch.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ impl<'tcx> MirPass<'tcx> for EarlyOtherwiseBranch {
let opts_to_apply: Vec<OptimizationToApply<'tcx>> = bbs_with_switch
.flat_map(|(bb_idx, bb)| {
let switch = bb.terminator();
let helper = Helper { body, tcx };
let helper = Helper { body };
let infos = helper.go(bb, switch)?;
Some(OptimizationToApply { infos, basic_block_first_switch: bb_idx })
})
Expand Down Expand Up @@ -98,17 +98,47 @@ impl<'tcx> MirPass<'tcx> for EarlyOtherwiseBranch {
StatementKind::Assign(box (Place::from(not_equal_temp), not_equal_rvalue)),
);

let (mut targets_to_jump_to, values_to_jump_to): (Vec<_>, Vec<_>) = opt_to_apply
.infos
.iter()
.flat_map(|x| x.second_switch_info.targets_with_values.iter())
.cloned()
.unzip();
// Switch on the NotEqual. If true, then jump to the `otherwise` case,
// since we know the discriminant values are not the same.
let true_case = opt_to_apply.infos[0].first_switch_info.otherwise_bb;

// In the false case we know that the two discriminant values are equal,
// however we still need to account for the following scenario:
// ```rust
// match (x, y) {
// (Some(_), Some(_)) => 0,
// _ => 2
// }
// ```
//
// Here, the two match arms have the same discriminant values, but
// we need to make sure that we did not reach a `(None, None)` pattern.
// We therefore construct a new basic block that can disambiguate where to go.

let mut targets_and_values_to_jump_to = vec![];
for info in opt_to_apply.infos.iter() {
for (_, value) in info.first_switch_info.targets_with_values.iter() {
// Find corresponding value in second switch info -
// this is where we want to jump to
if let Some((target_to_jump_to, _)) = info
.second_switch_info
.targets_with_values
.iter()
.find(|(_, second_value)| value == second_value)
{
targets_and_values_to_jump_to.push((target_to_jump_to, value));
}
}
}

let (mut targets_to_jump_to, values_to_jump_to): (Vec<_>, Vec<_>) =
targets_and_values_to_jump_to.iter().cloned().unzip();

// add otherwise case in the end
targets_to_jump_to.push(opt_to_apply.infos[0].first_switch_info.otherwise_bb);

// new block that jumps to the correct discriminant case. This block is switched to if the discriminants are equal
let new_switch_data = BasicBlockData::new(Some(Terminator {
let mut new_switch_data = BasicBlockData::new(Some(Terminator {
source_info: opt_to_apply.infos[0].second_switch_info.discr_source_info,
kind: TerminatorKind::SwitchInt {
// the first and second discriminants are equal, so just pick one
Expand All @@ -119,14 +149,16 @@ impl<'tcx> MirPass<'tcx> for EarlyOtherwiseBranch {
},
}));

let new_switch_bb = patch.new_block(new_switch_data);
let basic_block_first_switch = opt_to_apply.basic_block_first_switch;

// switch on the NotEqual. If true, then jump to the `otherwise` case.
// If false, then jump to a basic block that then jumps to the correct disciminant case
let true_case = opt_to_apply.infos[0].first_switch_info.otherwise_bb;
// Inherit the is_cleanup from the bb we are jumping from, which is where the first switch is
new_switch_data.is_cleanup = body.basic_blocks()[basic_block_first_switch].is_cleanup;

let new_switch_bb = patch.new_block(new_switch_data);
let false_case = new_switch_bb;

patch.patch_terminator(
opt_to_apply.basic_block_first_switch,
basic_block_first_switch,
TerminatorKind::if_(
tcx,
Operand::Move(Place::from(not_equal_temp)),
Expand Down Expand Up @@ -169,7 +201,6 @@ fn is_switch<'tcx>(terminator: &Terminator<'tcx>) -> bool {

struct Helper<'a, 'tcx> {
body: &'a Body<'tcx>,
tcx: TyCtxt<'tcx>,
}

#[derive(Debug, Clone)]
Expand All @@ -185,8 +216,6 @@ struct SwitchDiscriminantInfo<'tcx> {
discr_used_in_switch: Place<'tcx>,
/// The place of the adt that has its discriminant read
place_of_adt_discr_read: Place<'tcx>,
/// The type of the adt that has its discriminant read
type_adt_matched_on: Ty<'tcx>,
}

#[derive(Debug)]
Expand Down Expand Up @@ -239,30 +268,14 @@ impl<'a, 'tcx> Helper<'a, 'tcx> {
if is_switch(terminator) {
let this_bb_discr_info = self.find_switch_discriminant_info(bb, terminator)?;

// the types of the two adts matched on have to be equalfor this optimization to apply
if discr_info.type_adt_matched_on != this_bb_discr_info.type_adt_matched_on {
trace!(
"NO: types do not match. LHS: {:?}, RHS: {:?}",
discr_info.type_adt_matched_on,
this_bb_discr_info.type_adt_matched_on
);
return None;
}

// the otherwise branch of the two switches have to point to the same bb
// The otherwise branch of the two switches have to point to the same bb
if discr_info.otherwise_bb != this_bb_discr_info.otherwise_bb {
trace!("NO: otherwise target is not the same");
return None;
}

// check that the value being matched on is the same. The
if this_bb_discr_info.targets_with_values.iter().find(|x| x.1 == value).is_none() {
trace!("NO: values being matched on are not the same");
return None;
}

// only allow optimization if the left and right of the tuple being matched are the same variants.
// so the following should not optimize
// Only allow optimization if the left and right of the tuple being matched
// are the same variants. So the following should not optimize
// ```rust
// let x: Option<()>;
// let y: Option<()>;
Expand All @@ -271,7 +284,8 @@ impl<'a, 'tcx> Helper<'a, 'tcx> {
// _ => {}
// }
// ```
// We check this by seeing that the value of the first discriminant is the only other discriminant value being used as a target in the second switch
// We check this by seeing that the value of the first discriminant is the only
// other discriminant value being used as a target in the second switch
if !(this_bb_discr_info.targets_with_values.len() == 1
&& this_bb_discr_info.targets_with_values[0].1 == value)
{
Expand All @@ -281,7 +295,7 @@ impl<'a, 'tcx> Helper<'a, 'tcx> {
return None;
}

// if we reach this point, the optimization applies, and we should be able to optimize this case
// If we reach this point, the optimization applies, and we should be able to optimize this case
// store the info that is needed to apply the optimization

Some(OptimizationInfo {
Expand Down Expand Up @@ -321,16 +335,13 @@ impl<'a, 'tcx> Helper<'a, 'tcx> {
_ => None,
}?;

let type_adt_matched_on = place_of_adt_discr_read.ty(self.body, self.tcx).ty;

Some(SwitchDiscriminantInfo {
discr_used_in_switch: discr.place()?,
discr_ty,
otherwise_bb,
targets_with_values,
discr_source_info: discr_decl.source_info,
place_of_adt_discr_read,
type_adt_matched_on,
})
}
_ => unreachable!("must only be passed terminator that is a switch"),
Expand Down
78 changes: 78 additions & 0 deletions src/test/mir-opt/early_otherwise_branch.opt3.EarlyOtherwiseBranch.diff
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,78 @@
- // MIR for `opt3` before EarlyOtherwiseBranch
+ // MIR for `opt3` after EarlyOtherwiseBranch

fn opt3(_1: MyOption1<u32>, _2: MyOption2<u32>) -> u32 {
debug x => _1; // in scope 0 at $DIR/early_otherwise_branch.rs:31:9: 31:10
debug y => _2; // in scope 0 at $DIR/early_otherwise_branch.rs:31:28: 31:29
let mut _0: u32; // return place in scope 0 at $DIR/early_otherwise_branch.rs:31:50: 31:53
let mut _3: (MyOption1<u32>, MyOption2<u32>); // in scope 0 at $DIR/early_otherwise_branch.rs:32:11: 32:17
let mut _4: MyOption1<u32>; // in scope 0 at $DIR/early_otherwise_branch.rs:32:12: 32:13
let mut _5: MyOption2<u32>; // in scope 0 at $DIR/early_otherwise_branch.rs:32:15: 32:16
let mut _6: isize; // in scope 0 at $DIR/early_otherwise_branch.rs:33:30: 33:48
let mut _7: isize; // in scope 0 at $DIR/early_otherwise_branch.rs:33:10: 33:28
let _8: u32; // in scope 0 at $DIR/early_otherwise_branch.rs:33:26: 33:27
let _9: u32; // in scope 0 at $DIR/early_otherwise_branch.rs:33:46: 33:47
+ let mut _10: isize; // in scope 0 at $DIR/early_otherwise_branch.rs:33:30: 33:48
+ let mut _11: bool; // in scope 0 at $DIR/early_otherwise_branch.rs:33:30: 33:48
scope 1 {
debug a => _8; // in scope 1 at $DIR/early_otherwise_branch.rs:33:26: 33:27
debug b => _9; // in scope 1 at $DIR/early_otherwise_branch.rs:33:46: 33:47
}

bb0: {
StorageLive(_3); // scope 0 at $DIR/early_otherwise_branch.rs:32:11: 32:17
StorageLive(_4); // scope 0 at $DIR/early_otherwise_branch.rs:32:12: 32:13
_4 = move _1; // scope 0 at $DIR/early_otherwise_branch.rs:32:12: 32:13
StorageLive(_5); // scope 0 at $DIR/early_otherwise_branch.rs:32:15: 32:16
_5 = move _2; // scope 0 at $DIR/early_otherwise_branch.rs:32:15: 32:16
(_3.0: MyOption1<u32>) = move _4; // scope 0 at $DIR/early_otherwise_branch.rs:32:11: 32:17
(_3.1: MyOption2<u32>) = move _5; // scope 0 at $DIR/early_otherwise_branch.rs:32:11: 32:17
StorageDead(_5); // scope 0 at $DIR/early_otherwise_branch.rs:32:16: 32:17
StorageDead(_4); // scope 0 at $DIR/early_otherwise_branch.rs:32:16: 32:17
_7 = discriminant((_3.0: MyOption1<u32>)); // scope 0 at $DIR/early_otherwise_branch.rs:33:10: 33:28
- switchInt(move _7) -> [0_isize: bb1, otherwise: bb2]; // scope 0 at $DIR/early_otherwise_branch.rs:33:10: 33:28
+ StorageLive(_10); // scope 0 at $DIR/early_otherwise_branch.rs:33:10: 33:28
+ _10 = discriminant((_3.1: MyOption2<u32>)); // scope 0 at $DIR/early_otherwise_branch.rs:33:10: 33:28
+ StorageLive(_11); // scope 0 at $DIR/early_otherwise_branch.rs:33:10: 33:28
+ _11 = Ne(_10, _7); // scope 0 at $DIR/early_otherwise_branch.rs:33:10: 33:28
+ StorageDead(_10); // scope 0 at $DIR/early_otherwise_branch.rs:33:10: 33:28
+ switchInt(move _11) -> [false: bb4, otherwise: bb1]; // scope 0 at $DIR/early_otherwise_branch.rs:33:10: 33:28
}

bb1: {
- _6 = discriminant((_3.1: MyOption2<u32>)); // scope 0 at $DIR/early_otherwise_branch.rs:33:30: 33:48
- switchInt(move _6) -> [0_isize: bb3, otherwise: bb2]; // scope 0 at $DIR/early_otherwise_branch.rs:33:30: 33:48
- }
-
- bb2: {
+ StorageDead(_11); // scope 0 at $DIR/early_otherwise_branch.rs:34:14: 34:15
_0 = const 1_u32; // scope 0 at $DIR/early_otherwise_branch.rs:34:14: 34:15
- goto -> bb4; // scope 0 at $DIR/early_otherwise_branch.rs:32:5: 35:6
+ goto -> bb3; // scope 0 at $DIR/early_otherwise_branch.rs:32:5: 35:6
}

- bb3: {
+ bb2: {
StorageLive(_8); // scope 0 at $DIR/early_otherwise_branch.rs:33:26: 33:27
_8 = (((_3.0: MyOption1<u32>) as Some).0: u32); // scope 0 at $DIR/early_otherwise_branch.rs:33:26: 33:27
StorageLive(_9); // scope 0 at $DIR/early_otherwise_branch.rs:33:46: 33:47
_9 = (((_3.1: MyOption2<u32>) as Some).0: u32); // scope 0 at $DIR/early_otherwise_branch.rs:33:46: 33:47
_0 = const 0_u32; // scope 1 at $DIR/early_otherwise_branch.rs:33:53: 33:54
StorageDead(_9); // scope 0 at $DIR/early_otherwise_branch.rs:33:53: 33:54
StorageDead(_8); // scope 0 at $DIR/early_otherwise_branch.rs:33:53: 33:54
- goto -> bb4; // scope 0 at $DIR/early_otherwise_branch.rs:32:5: 35:6
+ goto -> bb3; // scope 0 at $DIR/early_otherwise_branch.rs:32:5: 35:6
}

- bb4: {
+ bb3: {
StorageDead(_3); // scope 0 at $DIR/early_otherwise_branch.rs:36:1: 36:2
return; // scope 0 at $DIR/early_otherwise_branch.rs:36:2: 36:2
+ }
+
+ bb4: {
+ StorageDead(_11); // scope 0 at $DIR/early_otherwise_branch.rs:33:30: 33:48
+ switchInt(_7) -> [0_isize: bb2, otherwise: bb1]; // scope 0 at $DIR/early_otherwise_branch.rs:33:30: 33:48
}
}

64 changes: 64 additions & 0 deletions src/test/mir-opt/early_otherwise_branch.opt4.EarlyOtherwiseBranch.diff
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,64 @@
- // MIR for `opt4` before EarlyOtherwiseBranch
+ // MIR for `opt4` after EarlyOtherwiseBranch

fn opt4(_1: std::result::Result<u32, ()>, _2: Option<u32>) -> u32 {
debug x => _1; // in scope 0 at $DIR/early_otherwise_branch.rs:40:9: 40:10
debug y => _2; // in scope 0 at $DIR/early_otherwise_branch.rs:40:29: 40:30
let mut _0: u32; // return place in scope 0 at $DIR/early_otherwise_branch.rs:40:48: 40:51
let mut _3: (std::result::Result<u32, ()>, std::option::Option<u32>); // in scope 0 at $DIR/early_otherwise_branch.rs:41:11: 41:17
let mut _4: std::result::Result<u32, ()>; // in scope 0 at $DIR/early_otherwise_branch.rs:41:12: 41:13
let mut _5: std::option::Option<u32>; // in scope 0 at $DIR/early_otherwise_branch.rs:41:15: 41:16
let mut _6: isize; // in scope 0 at $DIR/early_otherwise_branch.rs:42:18: 42:25
let mut _7: isize; // in scope 0 at $DIR/early_otherwise_branch.rs:42:10: 42:16
+ let mut _8: isize; // in scope 0 at $DIR/early_otherwise_branch.rs:42:18: 42:25
+ let mut _9: bool; // in scope 0 at $DIR/early_otherwise_branch.rs:42:18: 42:25

bb0: {
StorageLive(_3); // scope 0 at $DIR/early_otherwise_branch.rs:41:11: 41:17
StorageLive(_4); // scope 0 at $DIR/early_otherwise_branch.rs:41:12: 41:13
_4 = _1; // scope 0 at $DIR/early_otherwise_branch.rs:41:12: 41:13
StorageLive(_5); // scope 0 at $DIR/early_otherwise_branch.rs:41:15: 41:16
_5 = _2; // scope 0 at $DIR/early_otherwise_branch.rs:41:15: 41:16
(_3.0: std::result::Result<u32, ()>) = move _4; // scope 0 at $DIR/early_otherwise_branch.rs:41:11: 41:17
(_3.1: std::option::Option<u32>) = move _5; // scope 0 at $DIR/early_otherwise_branch.rs:41:11: 41:17
StorageDead(_5); // scope 0 at $DIR/early_otherwise_branch.rs:41:16: 41:17
StorageDead(_4); // scope 0 at $DIR/early_otherwise_branch.rs:41:16: 41:17
_7 = discriminant((_3.0: std::result::Result<u32, ()>)); // scope 0 at $DIR/early_otherwise_branch.rs:42:10: 42:16
- switchInt(move _7) -> [1_isize: bb2, otherwise: bb1]; // scope 0 at $DIR/early_otherwise_branch.rs:42:10: 42:16
+ StorageLive(_8); // scope 0 at $DIR/early_otherwise_branch.rs:42:10: 42:16
+ _8 = discriminant((_3.1: std::option::Option<u32>)); // scope 0 at $DIR/early_otherwise_branch.rs:42:10: 42:16
+ StorageLive(_9); // scope 0 at $DIR/early_otherwise_branch.rs:42:10: 42:16
+ _9 = Ne(_8, _7); // scope 0 at $DIR/early_otherwise_branch.rs:42:10: 42:16
+ StorageDead(_8); // scope 0 at $DIR/early_otherwise_branch.rs:42:10: 42:16
+ switchInt(move _9) -> [false: bb4, otherwise: bb1]; // scope 0 at $DIR/early_otherwise_branch.rs:42:10: 42:16
}

bb1: {
+ StorageDead(_9); // scope 0 at $DIR/early_otherwise_branch.rs:43:14: 43:15
_0 = const 1_u32; // scope 0 at $DIR/early_otherwise_branch.rs:43:14: 43:15
- goto -> bb4; // scope 0 at $DIR/early_otherwise_branch.rs:41:5: 44:6
+ goto -> bb3; // scope 0 at $DIR/early_otherwise_branch.rs:41:5: 44:6
}

bb2: {
- _6 = discriminant((_3.1: std::option::Option<u32>)); // scope 0 at $DIR/early_otherwise_branch.rs:42:18: 42:25
- switchInt(move _6) -> [1_isize: bb3, otherwise: bb1]; // scope 0 at $DIR/early_otherwise_branch.rs:42:18: 42:25
+ _0 = const 0_u32; // scope 0 at $DIR/early_otherwise_branch.rs:42:30: 42:31
+ goto -> bb3; // scope 0 at $DIR/early_otherwise_branch.rs:41:5: 44:6
}

bb3: {
- _0 = const 0_u32; // scope 0 at $DIR/early_otherwise_branch.rs:42:30: 42:31
- goto -> bb4; // scope 0 at $DIR/early_otherwise_branch.rs:41:5: 44:6
+ StorageDead(_3); // scope 0 at $DIR/early_otherwise_branch.rs:45:1: 45:2
+ return; // scope 0 at $DIR/early_otherwise_branch.rs:45:2: 45:2
}

bb4: {
- StorageDead(_3); // scope 0 at $DIR/early_otherwise_branch.rs:45:1: 45:2
- return; // scope 0 at $DIR/early_otherwise_branch.rs:45:2: 45:2
+ StorageDead(_9); // scope 0 at $DIR/early_otherwise_branch.rs:42:18: 42:25
+ switchInt(_7) -> [1_isize: bb2, otherwise: bb1]; // scope 0 at $DIR/early_otherwise_branch.rs:42:18: 42:25
}
}

Loading