From 9d34c280d92e1bc443c9ae10744e5b73a71e8915 Mon Sep 17 00:00:00 2001 From: Scott A Carr Date: Tue, 31 May 2016 17:08:00 -0700 Subject: [PATCH 1/3] generate fewer basic blocks for variant switches --- src/librustc_mir/build/matches/mod.rs | 16 +++++++- src/librustc_mir/build/matches/test.rs | 57 ++++++++++++++++++++++---- 2 files changed, 65 insertions(+), 8 deletions(-) diff --git a/src/librustc_mir/build/matches/mod.rs b/src/librustc_mir/build/matches/mod.rs index c1a0e1f9a69..bdaaafc1d8f 100644 --- a/src/librustc_mir/build/matches/mod.rs +++ b/src/librustc_mir/build/matches/mod.rs @@ -266,6 +266,7 @@ enum TestKind<'tcx> { // test the branches of enum Switch { adt_def: AdtDef<'tcx>, + variants: Vec, }, // test the branches of enum @@ -391,9 +392,11 @@ impl<'a, 'gcx, 'tcx> Builder<'a, 'gcx, 'tcx> { fn join_otherwise_blocks(&mut self, span: Span, - otherwise: Vec) + mut otherwise: Vec) -> BasicBlock { + otherwise.sort(); + otherwise.dedup(); // variant switches can introduce duplicate target blocks let scope_id = self.innermost_scope_id(); if otherwise.len() == 1 { otherwise[0] @@ -502,6 +505,15 @@ impl<'a, 'gcx, 'tcx> Builder<'a, 'gcx, 'tcx> { } } } + TestKind::Switch { adt_def: _, ref mut variants} => { + for candidate in candidates.iter() { + if !self.add_variants_to_switch(&match_pair.lvalue, + candidate, + variants) { + break; + } + } + } _ => { } } @@ -525,6 +537,8 @@ impl<'a, 'gcx, 'tcx> Builder<'a, 'gcx, 'tcx> { &mut target_candidates)) .count(); assert!(tested_candidates > 0); // at least the last candidate ought to be tested + debug!("tested_candidates: {}", tested_candidates); + debug!("untested_candidates: {}", candidates.len() - tested_candidates); // For each outcome of test, process the candidates that still // apply. Collect a list of blocks where control flow will diff --git a/src/librustc_mir/build/matches/test.rs b/src/librustc_mir/build/matches/test.rs index e53584a3f8b..307c8ecd832 100644 --- a/src/librustc_mir/build/matches/test.rs +++ b/src/librustc_mir/build/matches/test.rs @@ -33,7 +33,10 @@ impl<'a, 'gcx, 'tcx> Builder<'a, 'gcx, 'tcx> { PatternKind::Variant { ref adt_def, variant_index: _, subpatterns: _ } => { Test { span: match_pair.pattern.span, - kind: TestKind::Switch { adt_def: adt_def.clone() }, + kind: TestKind::Switch { + adt_def: adt_def.clone(), + variants: vec![false; self.hir.num_variants(adt_def)], + }, } } @@ -125,9 +128,10 @@ impl<'a, 'gcx, 'tcx> Builder<'a, 'gcx, 'tcx> { }); true } - + PatternKind::Variant { .. } => { + panic!("you should have called add_cases_to_switch_switch instead!"); + } PatternKind::Range { .. } | - PatternKind::Variant { .. } | PatternKind::Slice { .. } | PatternKind::Array { .. } | PatternKind::Wild | @@ -140,6 +144,30 @@ impl<'a, 'gcx, 'tcx> Builder<'a, 'gcx, 'tcx> { } } + pub fn add_variants_to_switch<'pat>(&mut self, + test_lvalue: &Lvalue<'tcx>, + candidate: &Candidate<'pat, 'tcx>, + variants: &mut Vec) + -> bool + { + let match_pair = match candidate.match_pairs.iter().find(|mp| mp.lvalue == *test_lvalue) { + Some(match_pair) => match_pair, + _ => { return false; } + }; + + match *match_pair.pattern.kind { + PatternKind::Variant { adt_def: _ , variant_index, .. } => { + // Do I need to look at the PatternKind::Variant subpatterns? + variants[variant_index] |= true; + true + } + _ => { + // don't know how to add these patterns to a switch + false + } + } + } + /// Generates the code to perform a test. pub fn perform_test(&mut self, block: BasicBlock, @@ -148,11 +176,26 @@ impl<'a, 'gcx, 'tcx> Builder<'a, 'gcx, 'tcx> { -> Vec { let scope_id = self.innermost_scope_id(); match test.kind { - TestKind::Switch { adt_def } => { + TestKind::Switch { adt_def, ref variants } => { let num_enum_variants = self.hir.num_variants(adt_def); - let target_blocks: Vec<_> = + debug!("num_enum_variants: {}", num_enum_variants); + debug!("variants.len(): {}", variants.len()); + debug!("variants: {:?}", variants); + let target_blocks: Vec<_> = if variants.into_iter().any(|b| {!b}) { + let otherwise_block = self.cfg.start_new_block(); + debug!("basic block: {:?} is an otherwise block!", otherwise_block); + (0..num_enum_variants).map(|i| + if variants[i] { + self.cfg.start_new_block() + } else { + otherwise_block + } + ) + .collect() + } else { (0..num_enum_variants).map(|_| self.cfg.start_new_block()) - .collect(); + .collect() + }; self.cfg.terminate(block, scope_id, test.span, TerminatorKind::Switch { discr: lvalue.clone(), adt_def: adt_def, @@ -383,7 +426,7 @@ impl<'a, 'gcx, 'tcx> Builder<'a, 'gcx, 'tcx> { match test.kind { // If we are performing a variant switch, then this // informs variant patterns, but nothing else. - TestKind::Switch { adt_def: tested_adt_def } => { + TestKind::Switch { adt_def: tested_adt_def , .. } => { match *match_pair.pattern.kind { PatternKind::Variant { adt_def, variant_index, ref subpatterns } => { assert_eq!(adt_def, tested_adt_def); From 79bf586d4b12456817578d75b305b3027e96d44e Mon Sep 17 00:00:00 2001 From: Scott A Carr Date: Wed, 1 Jun 2016 10:23:56 -0700 Subject: [PATCH 2/3] switch to BitVector, simplify target_block logic clarify comments and panic message --- src/librustc_data_structures/bitvec.rs | 2 +- src/librustc_mir/build/matches/mod.rs | 3 +- src/librustc_mir/build/matches/test.rs | 47 ++++++++++++-------------- 3 files changed, 25 insertions(+), 27 deletions(-) diff --git a/src/librustc_data_structures/bitvec.rs b/src/librustc_data_structures/bitvec.rs index cb648038c34..79d3f0cf688 100644 --- a/src/librustc_data_structures/bitvec.rs +++ b/src/librustc_data_structures/bitvec.rs @@ -11,7 +11,7 @@ use std::iter::FromIterator; /// A very simple BitVector type. -#[derive(Clone)] +#[derive(Clone, Debug, PartialEq)] pub struct BitVector { data: Vec, } diff --git a/src/librustc_mir/build/matches/mod.rs b/src/librustc_mir/build/matches/mod.rs index bdaaafc1d8f..88d7e41bc61 100644 --- a/src/librustc_mir/build/matches/mod.rs +++ b/src/librustc_mir/build/matches/mod.rs @@ -15,6 +15,7 @@ use build::{BlockAnd, BlockAndExtension, Builder}; use rustc_data_structures::fnv::FnvHashMap; +use rustc_data_structures::bitvec::BitVector; use rustc::middle::const_val::ConstVal; use rustc::ty::{AdtDef, Ty}; use rustc::mir::repr::*; @@ -266,7 +267,7 @@ enum TestKind<'tcx> { // test the branches of enum Switch { adt_def: AdtDef<'tcx>, - variants: Vec, + variants: BitVector, }, // test the branches of enum diff --git a/src/librustc_mir/build/matches/test.rs b/src/librustc_mir/build/matches/test.rs index 307c8ecd832..838597d1a17 100644 --- a/src/librustc_mir/build/matches/test.rs +++ b/src/librustc_mir/build/matches/test.rs @@ -19,6 +19,7 @@ use build::Builder; use build::matches::{Candidate, MatchPair, Test, TestKind}; use hair::*; use rustc_data_structures::fnv::FnvHashMap; +use rustc_data_structures::bitvec::BitVector; use rustc::middle::const_val::ConstVal; use rustc::ty::{self, Ty}; use rustc::mir::repr::*; @@ -35,7 +36,7 @@ impl<'a, 'gcx, 'tcx> Builder<'a, 'gcx, 'tcx> { span: match_pair.pattern.span, kind: TestKind::Switch { adt_def: adt_def.clone(), - variants: vec![false; self.hir.num_variants(adt_def)], + variants: BitVector::new(self.hir.num_variants(adt_def)), }, } } @@ -129,7 +130,7 @@ impl<'a, 'gcx, 'tcx> Builder<'a, 'gcx, 'tcx> { true } PatternKind::Variant { .. } => { - panic!("you should have called add_cases_to_switch_switch instead!"); + panic!("you should have called add_variants_to_switch instead!"); } PatternKind::Range { .. } | PatternKind::Slice { .. } | @@ -145,10 +146,10 @@ impl<'a, 'gcx, 'tcx> Builder<'a, 'gcx, 'tcx> { } pub fn add_variants_to_switch<'pat>(&mut self, - test_lvalue: &Lvalue<'tcx>, - candidate: &Candidate<'pat, 'tcx>, - variants: &mut Vec) - -> bool + test_lvalue: &Lvalue<'tcx>, + candidate: &Candidate<'pat, 'tcx>, + variants: &mut BitVector) + -> bool { let match_pair = match candidate.match_pairs.iter().find(|mp| mp.lvalue == *test_lvalue) { Some(match_pair) => match_pair, @@ -157,8 +158,9 @@ impl<'a, 'gcx, 'tcx> Builder<'a, 'gcx, 'tcx> { match *match_pair.pattern.kind { PatternKind::Variant { adt_def: _ , variant_index, .. } => { - // Do I need to look at the PatternKind::Variant subpatterns? - variants[variant_index] |= true; + // We have a pattern testing for variant `variant_index` + // set the corresponding index to true + variants.insert(variant_index); true } _ => { @@ -178,24 +180,19 @@ impl<'a, 'gcx, 'tcx> Builder<'a, 'gcx, 'tcx> { match test.kind { TestKind::Switch { adt_def, ref variants } => { let num_enum_variants = self.hir.num_variants(adt_def); - debug!("num_enum_variants: {}", num_enum_variants); - debug!("variants.len(): {}", variants.len()); - debug!("variants: {:?}", variants); - let target_blocks: Vec<_> = if variants.into_iter().any(|b| {!b}) { - let otherwise_block = self.cfg.start_new_block(); - debug!("basic block: {:?} is an otherwise block!", otherwise_block); - (0..num_enum_variants).map(|i| - if variants[i] { - self.cfg.start_new_block() - } else { - otherwise_block + let mut otherwise_block = None; + let target_blocks: Vec<_> = (0..num_enum_variants).map(|i| { + if variants.contains(i) { + self.cfg.start_new_block() + } else { + if otherwise_block.is_none() { + otherwise_block = Some(self.cfg.start_new_block()); } - ) - .collect() - } else { - (0..num_enum_variants).map(|_| self.cfg.start_new_block()) - .collect() - }; + otherwise_block.unwrap() + } + }).collect(); + debug!("num_enum_variants: {}, num tested variants: {}, variants: {:?}", + num_enum_variants, variants.iter().count(), variants); self.cfg.terminate(block, scope_id, test.span, TerminatorKind::Switch { discr: lvalue.clone(), adt_def: adt_def, From d4551ece5f5a318f24c16368fcea9d67b64c41ff Mon Sep 17 00:00:00 2001 From: Scott A Carr Date: Thu, 2 Jun 2016 15:40:03 -0700 Subject: [PATCH 3/3] remove trailing whitespace --- src/librustc_mir/build/matches/test.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/librustc_mir/build/matches/test.rs b/src/librustc_mir/build/matches/test.rs index 838597d1a17..43ec638eeb0 100644 --- a/src/librustc_mir/build/matches/test.rs +++ b/src/librustc_mir/build/matches/test.rs @@ -34,8 +34,8 @@ impl<'a, 'gcx, 'tcx> Builder<'a, 'gcx, 'tcx> { PatternKind::Variant { ref adt_def, variant_index: _, subpatterns: _ } => { Test { span: match_pair.pattern.span, - kind: TestKind::Switch { - adt_def: adt_def.clone(), + kind: TestKind::Switch { + adt_def: adt_def.clone(), variants: BitVector::new(self.hir.num_variants(adt_def)), }, }