From e7811f999f37ee1f1f71e18dedfbcf1dcf169f8f Mon Sep 17 00:00:00 2001 From: Simon Vandel Sillesen Date: Sat, 3 Oct 2020 01:04:53 +0200 Subject: [PATCH 1/8] Do not clone vec and bail early --- .../src/transform/early_otherwise_branch.rs | 16 +++++++--------- 1 file changed, 7 insertions(+), 9 deletions(-) diff --git a/compiler/rustc_mir/src/transform/early_otherwise_branch.rs b/compiler/rustc_mir/src/transform/early_otherwise_branch.rs index f97dcf4852df4..2443fb17cb9be 100644 --- a/compiler/rustc_mir/src/transform/early_otherwise_branch.rs +++ b/compiler/rustc_mir/src/transform/early_otherwise_branch.rs @@ -212,17 +212,15 @@ impl<'a, 'tcx> Helper<'a, 'tcx> { let discr = self.find_switch_discriminant_info(bb, switch)?; // go through each target, finding a discriminant read, and a switch - let results = discr.targets_with_values.iter().map(|(value, target)| { - self.find_discriminant_switch_pairing(&discr, target.clone(), value.clone()) - }); - - // if the optimization did not apply for one of the targets, then abort - if results.clone().any(|x| x.is_none()) || results.len() == 0 { - trace!("NO: not all of the targets matched the pattern for optimization"); - return None; + let mut results = vec![]; + + for (value, target) in discr.targets_with_values.iter() { + let info = + self.find_discriminant_switch_pairing(&discr, target.clone(), value.clone())?; + results.push(info); } - Some(results.flatten().collect()) + Some(results) } fn find_discriminant_switch_pairing( From 7a8c27775e513d3f4852b4f59764a6a37f9b81b5 Mon Sep 17 00:00:00 2001 From: Simon Vandel Sillesen Date: Sat, 3 Oct 2020 01:09:42 +0200 Subject: [PATCH 2/8] less clone --- compiler/rustc_mir/src/transform/early_otherwise_branch.rs | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/compiler/rustc_mir/src/transform/early_otherwise_branch.rs b/compiler/rustc_mir/src/transform/early_otherwise_branch.rs index 2443fb17cb9be..4a8005b42948e 100644 --- a/compiler/rustc_mir/src/transform/early_otherwise_branch.rs +++ b/compiler/rustc_mir/src/transform/early_otherwise_branch.rs @@ -215,8 +215,7 @@ impl<'a, 'tcx> Helper<'a, 'tcx> { let mut results = vec![]; for (value, target) in discr.targets_with_values.iter() { - let info = - self.find_discriminant_switch_pairing(&discr, target.clone(), value.clone())?; + let info = self.find_discriminant_switch_pairing(&discr, *target, *value)?; results.push(info); } @@ -300,7 +299,6 @@ impl<'a, 'tcx> Helper<'a, 'tcx> { // the declaration of the discriminant read. Place of this read is being used in the switch let discr_decl = &self.body.local_decls()[discr_local]; let discr_ty = discr_decl.ty; - // the otherwise target lies as the last element let otherwise_bb = targets.otherwise(); let targets_with_values = targets.iter().collect(); From 62b6896aa8381e0fe12f6a1957c27a8a724e405f Mon Sep 17 00:00:00 2001 From: Simon Vandel Sillesen Date: Sat, 3 Oct 2020 01:16:50 +0200 Subject: [PATCH 3/8] Remove duplicated check The same condition is checked just above --- compiler/rustc_mir/src/transform/early_otherwise_branch.rs | 6 ------ 1 file changed, 6 deletions(-) diff --git a/compiler/rustc_mir/src/transform/early_otherwise_branch.rs b/compiler/rustc_mir/src/transform/early_otherwise_branch.rs index 4a8005b42948e..a0fcb346a0d7d 100644 --- a/compiler/rustc_mir/src/transform/early_otherwise_branch.rs +++ b/compiler/rustc_mir/src/transform/early_otherwise_branch.rs @@ -250,12 +250,6 @@ impl<'a, 'tcx> Helper<'a, 'tcx> { 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.0 == 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 // ```rust From 4784a5ca5d0e84e36af9a39355dc4d9937e080ad Mon Sep 17 00:00:00 2001 From: Simon Vandel Sillesen Date: Sat, 3 Oct 2020 01:18:49 +0200 Subject: [PATCH 4/8] reorder checks from (intuitively) cheapest to most expensive --- .../src/transform/early_otherwise_branch.rs | 24 +++++++++---------- 1 file changed, 12 insertions(+), 12 deletions(-) diff --git a/compiler/rustc_mir/src/transform/early_otherwise_branch.rs b/compiler/rustc_mir/src/transform/early_otherwise_branch.rs index a0fcb346a0d7d..10679ecc32603 100644 --- a/compiler/rustc_mir/src/transform/early_otherwise_branch.rs +++ b/compiler/rustc_mir/src/transform/early_otherwise_branch.rs @@ -234,23 +234,13 @@ 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; } - // only allow optimization if the left and right of the tuple being matched are the same variants. + // 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<()>; @@ -270,6 +260,16 @@ impl<'a, 'tcx> Helper<'a, 'tcx> { return None; } + // The types of the two adts matched on have to be equal for 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; + } + // 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 From 586ba9d94530e47c579b07c90c1ca1613834512e Mon Sep 17 00:00:00 2001 From: Simon Vandel Sillesen Date: Sat, 3 Oct 2020 01:39:36 +0200 Subject: [PATCH 5/8] Restructure data such that data about the first switch is not duplicated --- .../src/transform/early_otherwise_branch.rs | 51 +++++++++---------- 1 file changed, 23 insertions(+), 28 deletions(-) diff --git a/compiler/rustc_mir/src/transform/early_otherwise_branch.rs b/compiler/rustc_mir/src/transform/early_otherwise_branch.rs index 10679ecc32603..36f4d714980ea 100644 --- a/compiler/rustc_mir/src/transform/early_otherwise_branch.rs +++ b/compiler/rustc_mir/src/transform/early_otherwise_branch.rs @@ -38,8 +38,8 @@ impl<'tcx> MirPass<'tcx> for EarlyOtherwiseBranch { .flat_map(|(bb_idx, bb)| { let switch = bb.terminator(); let helper = Helper { body, tcx }; - let infos = helper.go(bb, switch)?; - Some(OptimizationToApply { infos, basic_block_first_switch: bb_idx }) + let info = helper.go(bb, switch)?; + Some(OptimizationToApply { info, basic_block_first_switch: bb_idx }) }) .collect(); @@ -48,6 +48,8 @@ impl<'tcx> MirPass<'tcx> for EarlyOtherwiseBranch { for opt_to_apply in opts_to_apply { trace!("SUCCESS: found optimization possibility to apply: {:?}", &opt_to_apply); + let first_switch_info = opt_to_apply.info.first_switch_info; + let second_switch_info = &opt_to_apply.info.second_switch_infos[0]; let statements_before = body.basic_blocks()[opt_to_apply.basic_block_first_switch].statements.len(); let end_of_block_location = Location { @@ -58,8 +60,8 @@ impl<'tcx> MirPass<'tcx> for EarlyOtherwiseBranch { let mut patch = MirPatch::new(body); // create temp to store second discriminant in - let discr_type = opt_to_apply.infos[0].second_switch_info.discr_ty; - let discr_span = opt_to_apply.infos[0].second_switch_info.discr_source_info.span; + let discr_type = second_switch_info.discr_ty; + let discr_span = second_switch_info.discr_source_info.span; let second_discriminant_temp = patch.new_temp(discr_type, discr_span); patch.add_statement( @@ -68,8 +70,7 @@ impl<'tcx> MirPass<'tcx> for EarlyOtherwiseBranch { ); // create assignment of discriminant - let place_of_adt_to_get_discriminant_of = - opt_to_apply.infos[0].second_switch_info.place_of_adt_discr_read; + let place_of_adt_to_get_discriminant_of = second_switch_info.place_of_adt_discr_read; patch.add_assign( end_of_block_location, Place::from(second_discriminant_temp), @@ -83,8 +84,7 @@ impl<'tcx> MirPass<'tcx> for EarlyOtherwiseBranch { patch.add_statement(end_of_block_location, StatementKind::StorageLive(not_equal_temp)); // create NotEqual comparison between the two discriminants - let first_descriminant_place = - opt_to_apply.infos[0].first_switch_info.discr_used_in_switch; + let first_descriminant_place = first_switch_info.discr_used_in_switch; let not_equal_rvalue = Rvalue::BinaryOp( not_equal, Operand::Copy(Place::from(second_discriminant_temp)), @@ -96,19 +96,17 @@ impl<'tcx> MirPass<'tcx> for EarlyOtherwiseBranch { ); let new_targets = opt_to_apply - .infos + .info + .second_switch_infos .iter() - .flat_map(|x| x.second_switch_info.targets_with_values.iter()) + .flat_map(|x| x.targets_with_values.iter()) .cloned(); - let targets = SwitchTargets::new( - new_targets, - opt_to_apply.infos[0].first_switch_info.otherwise_bb, - ); + let targets = SwitchTargets::new(new_targets, 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 { - source_info: opt_to_apply.infos[0].second_switch_info.discr_source_info, + source_info: second_switch_info.discr_source_info, kind: TerminatorKind::SwitchInt { // the first and second discriminants are equal, so just pick one discr: Operand::Copy(first_descriminant_place), @@ -121,7 +119,7 @@ impl<'tcx> MirPass<'tcx> for EarlyOtherwiseBranch { // 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; + let true_case = first_switch_info.otherwise_bb; let false_case = new_switch_bb; patch.patch_terminator( opt_to_apply.basic_block_first_switch, @@ -189,7 +187,7 @@ struct SwitchDiscriminantInfo<'tcx> { #[derive(Debug)] struct OptimizationToApply<'tcx> { - infos: Vec>, + info: OptimizationInfo<'tcx>, /// Basic block of the original first switch basic_block_first_switch: BasicBlock, } @@ -198,8 +196,8 @@ struct OptimizationToApply<'tcx> { struct OptimizationInfo<'tcx> { /// Info about the first switch and discriminant first_switch_info: SwitchDiscriminantInfo<'tcx>, - /// Info about the second switch and discriminant - second_switch_info: SwitchDiscriminantInfo<'tcx>, + /// Info about all swtiches that are successors of the first switch + second_switch_infos: Vec>, } impl<'a, 'tcx> Helper<'a, 'tcx> { @@ -207,19 +205,19 @@ impl<'a, 'tcx> Helper<'a, 'tcx> { &self, bb: &BasicBlockData<'tcx>, switch: &Terminator<'tcx>, - ) -> Option>> { + ) -> Option> { // try to find the statement that defines the discriminant that is used for the switch let discr = self.find_switch_discriminant_info(bb, switch)?; // go through each target, finding a discriminant read, and a switch - let mut results = vec![]; + let mut second_switch_infos = vec![]; for (value, target) in discr.targets_with_values.iter() { let info = self.find_discriminant_switch_pairing(&discr, *target, *value)?; - results.push(info); + second_switch_infos.push(info); } - Some(results) + Some(OptimizationInfo { first_switch_info: discr, second_switch_infos }) } fn find_discriminant_switch_pairing( @@ -227,7 +225,7 @@ impl<'a, 'tcx> Helper<'a, 'tcx> { discr_info: &SwitchDiscriminantInfo<'tcx>, target: BasicBlock, value: u128, - ) -> Option> { + ) -> Option> { let bb = &self.body.basic_blocks()[target]; // find switch let terminator = bb.terminator(); @@ -273,10 +271,7 @@ impl<'a, 'tcx> Helper<'a, 'tcx> { // 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 { - first_switch_info: discr_info.clone(), - second_switch_info: this_bb_discr_info, - }) + Some(this_bb_discr_info) } else { None } From b4e144ebab3618c31f4dda0ebbaf5af8160cfc50 Mon Sep 17 00:00:00 2001 From: Simon Vandel Sillesen Date: Sat, 3 Oct 2020 16:21:44 +0200 Subject: [PATCH 6/8] Move helper creation out of loop --- compiler/rustc_mir/src/transform/early_otherwise_branch.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/compiler/rustc_mir/src/transform/early_otherwise_branch.rs b/compiler/rustc_mir/src/transform/early_otherwise_branch.rs index 36f4d714980ea..3ed0fd2fa4749 100644 --- a/compiler/rustc_mir/src/transform/early_otherwise_branch.rs +++ b/compiler/rustc_mir/src/transform/early_otherwise_branch.rs @@ -34,10 +34,10 @@ impl<'tcx> MirPass<'tcx> for EarlyOtherwiseBranch { let bbs_with_switch = body.basic_blocks().iter_enumerated().filter(|(_, bb)| is_switch(bb.terminator())); + let helper = Helper { body, tcx }; let opts_to_apply: Vec> = bbs_with_switch .flat_map(|(bb_idx, bb)| { let switch = bb.terminator(); - let helper = Helper { body, tcx }; let info = helper.go(bb, switch)?; Some(OptimizationToApply { info, basic_block_first_switch: bb_idx }) }) From bd56b11a8e2df0b2d0917feecaf1f2582f4d72f8 Mon Sep 17 00:00:00 2001 From: Simon Vandel Sillesen Date: Wed, 14 Oct 2020 17:19:59 +0200 Subject: [PATCH 7/8] Use SwitchTargets instead of storing in Vec --- .../src/transform/early_otherwise_branch.rs | 19 +++++++++---------- 1 file changed, 9 insertions(+), 10 deletions(-) diff --git a/compiler/rustc_mir/src/transform/early_otherwise_branch.rs b/compiler/rustc_mir/src/transform/early_otherwise_branch.rs index 3ed0fd2fa4749..567c911faf63c 100644 --- a/compiler/rustc_mir/src/transform/early_otherwise_branch.rs +++ b/compiler/rustc_mir/src/transform/early_otherwise_branch.rs @@ -99,8 +99,7 @@ impl<'tcx> MirPass<'tcx> for EarlyOtherwiseBranch { .info .second_switch_infos .iter() - .flat_map(|x| x.targets_with_values.iter()) - .cloned(); + .flat_map(|x| x.switch_targets.iter().map(|x| (x.0, x.1))); let targets = SwitchTargets::new(new_targets, first_switch_info.otherwise_bb); @@ -174,8 +173,8 @@ struct SwitchDiscriminantInfo<'tcx> { discr_ty: Ty<'tcx>, /// The basic block that the otherwise branch points to otherwise_bb: BasicBlock, - /// Target along with the value being branched from. Otherwise is not included - targets_with_values: Vec<(u128, BasicBlock)>, + /// Targets and values for the switch + switch_targets: SwitchTargets, discr_source_info: SourceInfo, /// The place of the discriminant used in the switch discr_used_in_switch: Place<'tcx>, @@ -212,8 +211,8 @@ impl<'a, 'tcx> Helper<'a, 'tcx> { // go through each target, finding a discriminant read, and a switch let mut second_switch_infos = vec![]; - for (value, target) in discr.targets_with_values.iter() { - let info = self.find_discriminant_switch_pairing(&discr, *target, *value)?; + for (value, target) in discr.switch_targets.iter() { + let info = self.find_discriminant_switch_pairing(&discr, target, value)?; second_switch_infos.push(info); } @@ -249,8 +248,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 - if !(this_bb_discr_info.targets_with_values.len() == 1 - && this_bb_discr_info.targets_with_values[0].0 == value) + if !(this_bb_discr_info.switch_targets.iter().len() == 1 + && this_bb_discr_info.switch_targets.iter().next()?.0 == value) { trace!( "NO: The second switch did not have only 1 target (besides otherwise) that had the same value as the value from the first switch that got us here" @@ -289,7 +288,7 @@ impl<'a, 'tcx> Helper<'a, 'tcx> { let discr_decl = &self.body.local_decls()[discr_local]; let discr_ty = discr_decl.ty; let otherwise_bb = targets.otherwise(); - let targets_with_values = targets.iter().collect(); + let targets_with_values = targets.clone(); // find the place of the adt where the discriminant is being read from // assume this is the last statement of the block @@ -306,7 +305,7 @@ impl<'a, 'tcx> Helper<'a, 'tcx> { discr_used_in_switch: discr.place()?, discr_ty, otherwise_bb, - targets_with_values, + switch_targets: targets_with_values, discr_source_info: discr_decl.source_info, place_of_adt_discr_read, type_adt_matched_on, From 1a8d28a7d616f90acbcbf549a482c6f0e1d47172 Mon Sep 17 00:00:00 2001 From: Simon Vandel Sillesen Date: Wed, 14 Oct 2020 17:25:13 +0200 Subject: [PATCH 8/8] Use otherwise from switchTargets --- .../src/transform/early_otherwise_branch.rs | 13 ++++++------- 1 file changed, 6 insertions(+), 7 deletions(-) diff --git a/compiler/rustc_mir/src/transform/early_otherwise_branch.rs b/compiler/rustc_mir/src/transform/early_otherwise_branch.rs index 567c911faf63c..fb11006274379 100644 --- a/compiler/rustc_mir/src/transform/early_otherwise_branch.rs +++ b/compiler/rustc_mir/src/transform/early_otherwise_branch.rs @@ -101,7 +101,8 @@ impl<'tcx> MirPass<'tcx> for EarlyOtherwiseBranch { .iter() .flat_map(|x| x.switch_targets.iter().map(|x| (x.0, x.1))); - let targets = SwitchTargets::new(new_targets, first_switch_info.otherwise_bb); + let targets = + SwitchTargets::new(new_targets, first_switch_info.switch_targets.otherwise()); // 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 { @@ -118,7 +119,7 @@ impl<'tcx> MirPass<'tcx> for EarlyOtherwiseBranch { // 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 = first_switch_info.otherwise_bb; + let true_case = first_switch_info.switch_targets.otherwise(); let false_case = new_switch_bb; patch.patch_terminator( opt_to_apply.basic_block_first_switch, @@ -171,8 +172,6 @@ struct Helper<'a, 'tcx> { struct SwitchDiscriminantInfo<'tcx> { /// Type of the discriminant being switched on discr_ty: Ty<'tcx>, - /// The basic block that the otherwise branch points to - otherwise_bb: BasicBlock, /// Targets and values for the switch switch_targets: SwitchTargets, discr_source_info: SourceInfo, @@ -232,7 +231,9 @@ impl<'a, 'tcx> Helper<'a, 'tcx> { let this_bb_discr_info = self.find_switch_discriminant_info(bb, terminator)?; // 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 { + if discr_info.switch_targets.otherwise() + != this_bb_discr_info.switch_targets.otherwise() + { trace!("NO: otherwise target is not the same"); return None; } @@ -287,7 +288,6 @@ impl<'a, 'tcx> Helper<'a, 'tcx> { // the declaration of the discriminant read. Place of this read is being used in the switch let discr_decl = &self.body.local_decls()[discr_local]; let discr_ty = discr_decl.ty; - let otherwise_bb = targets.otherwise(); let targets_with_values = targets.clone(); // find the place of the adt where the discriminant is being read from @@ -304,7 +304,6 @@ impl<'a, 'tcx> Helper<'a, 'tcx> { Some(SwitchDiscriminantInfo { discr_used_in_switch: discr.place()?, discr_ty, - otherwise_bb, switch_targets: targets_with_values, discr_source_info: discr_decl.source_info, place_of_adt_discr_read,