Skip to content

Commit

Permalink
Rollup merge of #121750 - Nadrieril:switchkind-if, r=matthewjasper
Browse files Browse the repository at this point in the history 
match lowering: Separate the `bool` case from other integers in `TestKind`

`TestKind::SwitchInt` had a special case for `bool` essentially everywhere it's used, so I made `TestKind::If` to handle the bool case on its own.

r? `@matthewjasper`
  • Loading branch information
@matthiaskrgr
matthiaskrgr authored Mar 1, 2024
2 parents 4f32f78 + d6332ae commit 0d2205f
Show file tree
Hide file tree
Showing 4 changed files with 59 additions and 65 deletions.
17 changes: 7 additions & 10 deletions compiler/rustc_mir_build/src/build/matches/mod.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1097,21 +1097,18 @@ enum TestKind<'tcx> {
variants: BitSet<VariantIdx>,
},

/// Test what value an integer, `bool`, or `char` has.
/// Test what value an integer or `char` has.
SwitchInt {
/// The type of the value that we're testing.
switch_ty: Ty<'tcx>,
/// The (ordered) set of values that we test for.
///
/// For integers and `char`s we create a branch to each of the values in
/// `options`, as well as an "otherwise" branch for all other values, even
/// in the (rare) case that `options` is exhaustive.
///
/// For `bool` we always generate two edges, one for `true` and one for
/// `false`.
/// We create a branch to each of the values in `options`, as well as an "otherwise" branch
/// for all other values, even in the (rare) case that `options` is exhaustive.
options: FxIndexMap<Const<'tcx>, u128>,
},

/// Test what value a `bool` has.
If,

/// Test for equality with value, possibly after an unsizing coercion to
/// `ty`,
Eq {
Expand Down Expand Up @@ -1617,7 +1614,7 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
// a test like SwitchInt, we may want to add cases based on the candidates that are
// available
match test.kind {
TestKind::SwitchInt { switch_ty: _, ref mut options } => {
TestKind::SwitchInt { ref mut options } => {
for candidate in candidates.iter() {
if !self.add_cases_to_switch(&match_place, candidate, options) {
break;
Expand Down
77 changes: 38 additions & 39 deletions compiler/rustc_mir_build/src/build/matches/test.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -34,12 +34,12 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
TestKind::Switch { adt_def, variants: BitSet::new_empty(adt_def.variants().len()) }
}

TestCase::Constant { .. } if match_pair.pattern.ty.is_bool() => TestKind::If,

TestCase::Constant { .. } if is_switch_ty(match_pair.pattern.ty) => {
// For integers, we use a `SwitchInt` match, which allows
// us to handle more cases.
TestKind::SwitchInt {
switch_ty: match_pair.pattern.ty,

// these maps are empty to start; cases are
// added below in add_cases_to_switch
options: Default::default(),
Expand Down Expand Up @@ -182,34 +182,29 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
);
}

TestKind::SwitchInt { switch_ty, ref options } => {
let terminator = if *switch_ty.kind() == ty::Bool {
assert!(!options.is_empty() && options.len() <= 2);
let [first_bb, second_bb] = *target_blocks else {
bug!("`TestKind::SwitchInt` on `bool` should have two targets")
};
let (true_bb, false_bb) = match options[0] {
1 => (first_bb, second_bb),
0 => (second_bb, first_bb),
v => span_bug!(test.span, "expected boolean value but got {:?}", v),
};
TerminatorKind::if_(Operand::Copy(place), true_bb, false_bb)
} else {
// The switch may be inexhaustive so we have a catch all block
debug_assert_eq!(options.len() + 1, target_blocks.len());
let otherwise_block = *target_blocks.last().unwrap();
let switch_targets = SwitchTargets::new(
options.values().copied().zip(target_blocks),
otherwise_block,
);
TerminatorKind::SwitchInt {
discr: Operand::Copy(place),
targets: switch_targets,
}
TestKind::SwitchInt { ref options } => {
// The switch may be inexhaustive so we have a catch-all block
debug_assert_eq!(options.len() + 1, target_blocks.len());
let otherwise_block = *target_blocks.last().unwrap();
let switch_targets = SwitchTargets::new(
options.values().copied().zip(target_blocks),
otherwise_block,
);
let terminator = TerminatorKind::SwitchInt {
discr: Operand::Copy(place),
targets: switch_targets,
};
self.cfg.terminate(block, self.source_info(match_start_span), terminator);
}

TestKind::If => {
let [false_bb, true_bb] = *target_blocks else {
bug!("`TestKind::If` should have two targets")
};
let terminator = TerminatorKind::if_(Operand::Copy(place), true_bb, false_bb);
self.cfg.terminate(block, self.source_info(match_start_span), terminator);
}

TestKind::Eq { value, ty } => {
let tcx = self.tcx;
let [success_block, fail_block] = *target_blocks else {
Expand Down Expand Up @@ -589,14 +584,14 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
//
// FIXME(#29623) we could use PatKind::Range to rule
// things out here, in some cases.
(TestKind::SwitchInt { switch_ty: _, options }, TestCase::Constant { value })
(TestKind::SwitchInt { options }, TestCase::Constant { value })
if is_switch_ty(match_pair.pattern.ty) =>
{
fully_matched = true;
let index = options.get_index_of(value).unwrap();
Some(index)
}
(TestKind::SwitchInt { switch_ty: _, options }, TestCase::Range(range)) => {
(TestKind::SwitchInt { options }, TestCase::Range(range)) => {
fully_matched = false;
let not_contained =
self.values_not_contained_in_range(&*range, options).unwrap_or(false);
Expand All @@ -608,6 +603,18 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
})
}

(&TestKind::If, TestCase::Constant { value }) => {
fully_matched = true;
let value = value.try_eval_bool(self.tcx, self.param_env).unwrap_or_else(|| {
span_bug!(test.span, "expected boolean value but got {value:?}")
});
Some(value as usize)
}
(&TestKind::If, _) => {
fully_matched = false;
None
}

(
&TestKind::Len { len: test_len, op: BinOp::Eq },
&TestCase::Slice { len, variable_length },
Expand Down Expand Up @@ -746,29 +753,21 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
impl Test<'_> {
pub(super) fn targets(&self) -> usize {
match self.kind {
TestKind::Eq { .. } | TestKind::Range(_) | TestKind::Len { .. } => 2,
TestKind::Eq { .. } | TestKind::Range(_) | TestKind::Len { .. } | TestKind::If => 2,
TestKind::Switch { adt_def, .. } => {
// While the switch that we generate doesn't test for all
// variants, we have a target for each variant and the
// otherwise case, and we make sure that all of the cases not
// specified have the same block.
adt_def.variants().len() + 1
}
TestKind::SwitchInt { switch_ty, ref options, .. } => {
if switch_ty.is_bool() {
// `bool` is special cased in `perform_test` to always
// branch to two blocks.
2
} else {
options.len() + 1
}
}
TestKind::SwitchInt { ref options } => options.len() + 1,
}
}
}

fn is_switch_ty(ty: Ty<'_>) -> bool {
ty.is_integral() || ty.is_char() || ty.is_bool()
ty.is_integral() || ty.is_char()
}

fn trait_method<'tcx>(
Expand Down
15 changes: 7 additions & 8 deletions ..._scopes.complicated_match.panic-abort.SimplifyCfg-initial.after-ElaborateDrops.after.diff
View file
Original file line number Diff line number Diff line change
Expand Up @@ -42,15 +42,11 @@
}

bb2: {
- switchInt((_2.0: bool)) -> [0: bb4, otherwise: bb3];
- switchInt((_2.0: bool)) -> [0: bb3, otherwise: bb4];
+ switchInt((_2.0: bool)) -> [0: bb3, otherwise: bb17];
}

bb3: {
- falseEdge -> [real: bb20, imaginary: bb4];
- }
-
- bb4: {
StorageLive(_15);
_15 = (_2.1: bool);
StorageLive(_16);
Expand All @@ -59,16 +55,19 @@
+ goto -> bb16;
}

bb4: {
- falseEdge -> [real: bb20, imaginary: bb3];
- }
-
- bb5: {
- falseEdge -> [real: bb13, imaginary: bb3];
- falseEdge -> [real: bb13, imaginary: bb4];
- }
-
- bb6: {
- falseEdge -> [real: bb8, imaginary: bb5];
- }
-
- bb7: {
+ bb4: {
_0 = const 1_i32;
- drop(_7) -> [return: bb18, unwind: bb25];
+ drop(_7) -> [return: bb15, unwind: bb22];
Expand Down Expand Up @@ -184,7 +183,7 @@
StorageDead(_12);
StorageDead(_8);
StorageDead(_6);
- falseEdge -> [real: bb2, imaginary: bb3];
- falseEdge -> [real: bb2, imaginary: bb4];
+ goto -> bb2;
}

Expand Down
15 changes: 7 additions & 8 deletions ...scopes.complicated_match.panic-unwind.SimplifyCfg-initial.after-ElaborateDrops.after.diff
View file
Original file line number Diff line number Diff line change
Expand Up @@ -42,15 +42,11 @@
}

bb2: {
- switchInt((_2.0: bool)) -> [0: bb4, otherwise: bb3];
- switchInt((_2.0: bool)) -> [0: bb3, otherwise: bb4];
+ switchInt((_2.0: bool)) -> [0: bb3, otherwise: bb17];
}

bb3: {
- falseEdge -> [real: bb20, imaginary: bb4];
- }
-
- bb4: {
StorageLive(_15);
_15 = (_2.1: bool);
StorageLive(_16);
Expand All @@ -59,16 +55,19 @@
+ goto -> bb16;
}

bb4: {
- falseEdge -> [real: bb20, imaginary: bb3];
- }
-
- bb5: {
- falseEdge -> [real: bb13, imaginary: bb3];
- falseEdge -> [real: bb13, imaginary: bb4];
- }
-
- bb6: {
- falseEdge -> [real: bb8, imaginary: bb5];
- }
-
- bb7: {
+ bb4: {
_0 = const 1_i32;
- drop(_7) -> [return: bb18, unwind: bb25];
+ drop(_7) -> [return: bb15, unwind: bb22];
Expand Down Expand Up @@ -184,7 +183,7 @@
StorageDead(_12);
StorageDead(_8);
StorageDead(_6);
- falseEdge -> [real: bb2, imaginary: bb3];
- falseEdge -> [real: bb2, imaginary: bb4];
+ goto -> bb2;
}

Expand Down

0 comments on commit 0d2205f

Please sign in to comment.