From d0669a473795e59a8dea1f8a6797aeeec971bc73 Mon Sep 17 00:00:00 2001 From: DianQK Date: Thu, 18 Apr 2024 22:08:09 +0800 Subject: [PATCH 1/3] Don't perform unsigned comparisons for signed integers --- .../rustc_mir_transform/src/match_branches.rs | 10 ++- ...h_i128_u128.MatchBranchSimplification.diff | 61 +++++++++---------- ..._failed_2_a.MatchBranchSimplification.diff | 37 +++++++++++ ..._failed_2_b.MatchBranchSimplification.diff | 37 +++++++++++ tests/mir-opt/matches_reduce_branches.rs | 29 ++++++++- 5 files changed, 136 insertions(+), 38 deletions(-) create mode 100644 tests/mir-opt/matches_reduce_branches.match_i8_i16_failed_2_a.MatchBranchSimplification.diff create mode 100644 tests/mir-opt/matches_reduce_branches.match_i8_i16_failed_2_b.MatchBranchSimplification.diff diff --git a/compiler/rustc_mir_transform/src/match_branches.rs b/compiler/rustc_mir_transform/src/match_branches.rs index 4d9a198eeb293..62d6525e4394e 100644 --- a/compiler/rustc_mir_transform/src/match_branches.rs +++ b/compiler/rustc_mir_transform/src/match_branches.rs @@ -399,7 +399,10 @@ impl<'tcx> SimplifyMatch<'tcx> for SimplifyToExp { if ((f_c.const_.ty().is_signed() || discr_ty.is_signed()) && int_equal(f, first_val, discr_size) && int_equal(s, second_val, discr_size)) - || (Some(f) == ScalarInt::try_from_uint(first_val, f.size()) + || (!f_c.const_.ty().is_signed() + && !discr_ty.is_signed() + && Some(f) + == ScalarInt::try_from_uint(first_val, f.size()) && Some(s) == ScalarInt::try_from_uint(second_val, s.size())) => { @@ -449,7 +452,10 @@ impl<'tcx> SimplifyMatch<'tcx> for SimplifyToExp { { continue; } - if Some(f) == ScalarInt::try_from_uint(other_val, f.size()) { + if !is_signed + && !s_c.const_.ty().is_signed() + && Some(f) == ScalarInt::try_from_uint(other_val, f.size()) + { continue; } return None; diff --git a/tests/mir-opt/matches_reduce_branches.match_i128_u128.MatchBranchSimplification.diff b/tests/mir-opt/matches_reduce_branches.match_i128_u128.MatchBranchSimplification.diff index 31ce51dc6de25..1f20349fdece4 100644 --- a/tests/mir-opt/matches_reduce_branches.match_i128_u128.MatchBranchSimplification.diff +++ b/tests/mir-opt/matches_reduce_branches.match_i128_u128.MatchBranchSimplification.diff @@ -5,42 +5,37 @@ debug i => _1; let mut _0: u128; let mut _2: i128; -+ let mut _3: i128; bb0: { _2 = discriminant(_1); -- switchInt(move _2) -> [1: bb3, 2: bb4, 3: bb5, 340282366920938463463374607431768211455: bb2, otherwise: bb1]; -- } -- -- bb1: { -- unreachable; -- } -- -- bb2: { -- _0 = const core::num::::MAX; -- goto -> bb6; -- } -- -- bb3: { -- _0 = const 1_u128; -- goto -> bb6; -- } -- -- bb4: { -- _0 = const 2_u128; -- goto -> bb6; -- } -- -- bb5: { -- _0 = const 3_u128; -- goto -> bb6; -- } -- -- bb6: { -+ StorageLive(_3); -+ _3 = move _2; -+ _0 = _3 as u128 (IntToInt); -+ StorageDead(_3); + switchInt(move _2) -> [1: bb3, 2: bb4, 3: bb5, 340282366920938463463374607431768211455: bb2, otherwise: bb1]; + } + + bb1: { + unreachable; + } + + bb2: { + _0 = const core::num::::MAX; + goto -> bb6; + } + + bb3: { + _0 = const 1_u128; + goto -> bb6; + } + + bb4: { + _0 = const 2_u128; + goto -> bb6; + } + + bb5: { + _0 = const 3_u128; + goto -> bb6; + } + + bb6: { return; } } diff --git a/tests/mir-opt/matches_reduce_branches.match_i8_i16_failed_2_a.MatchBranchSimplification.diff b/tests/mir-opt/matches_reduce_branches.match_i8_i16_failed_2_a.MatchBranchSimplification.diff new file mode 100644 index 0000000000000..16d5aa049757b --- /dev/null +++ b/tests/mir-opt/matches_reduce_branches.match_i8_i16_failed_2_a.MatchBranchSimplification.diff @@ -0,0 +1,37 @@ +- // MIR for `match_i8_i16_failed_2_a` before MatchBranchSimplification ++ // MIR for `match_i8_i16_failed_2_a` after MatchBranchSimplification + + fn match_i8_i16_failed_2_a(_1: EnumAi8) -> i16 { + debug i => _1; + let mut _0: i16; + let mut _2: i8; + + bb0: { + _2 = discriminant(_1); + switchInt(move _2) -> [255: bb3, 2: bb4, 253: bb2, otherwise: bb1]; + } + + bb1: { + unreachable; + } + + bb2: { + _0 = const -3_i16; + goto -> bb5; + } + + bb3: { + _0 = const 255_i16; + goto -> bb5; + } + + bb4: { + _0 = const 2_i16; + goto -> bb5; + } + + bb5: { + return; + } + } + diff --git a/tests/mir-opt/matches_reduce_branches.match_i8_i16_failed_2_b.MatchBranchSimplification.diff b/tests/mir-opt/matches_reduce_branches.match_i8_i16_failed_2_b.MatchBranchSimplification.diff new file mode 100644 index 0000000000000..603ad87aec250 --- /dev/null +++ b/tests/mir-opt/matches_reduce_branches.match_i8_i16_failed_2_b.MatchBranchSimplification.diff @@ -0,0 +1,37 @@ +- // MIR for `match_i8_i16_failed_2_b` before MatchBranchSimplification ++ // MIR for `match_i8_i16_failed_2_b` after MatchBranchSimplification + + fn match_i8_i16_failed_2_b(_1: EnumAi8) -> i16 { + debug i => _1; + let mut _0: i16; + let mut _2: i8; + + bb0: { + _2 = discriminant(_1); + switchInt(move _2) -> [255: bb3, 2: bb4, 253: bb2, otherwise: bb1]; + } + + bb1: { + unreachable; + } + + bb2: { + _0 = const 253_i16; + goto -> bb5; + } + + bb3: { + _0 = const -1_i16; + goto -> bb5; + } + + bb4: { + _0 = const 2_i16; + goto -> bb5; + } + + bb5: { + return; + } + } + diff --git a/tests/mir-opt/matches_reduce_branches.rs b/tests/mir-opt/matches_reduce_branches.rs index ca3e5f747d10d..d81fd1d4fcdae 100644 --- a/tests/mir-opt/matches_reduce_branches.rs +++ b/tests/mir-opt/matches_reduce_branches.rs @@ -225,6 +225,30 @@ fn match_i8_i16_failed(i: EnumAi8) -> i16 { } } +// We cannot transform it, even though `-1i8` and `255i16` are the same in terms of bits, +// what we actually require is that they are considered equal in a signed comparison. +// EMIT_MIR matches_reduce_branches.match_i8_i16_failed_2_a.MatchBranchSimplification.diff +fn match_i8_i16_failed_2_a(i: EnumAi8) -> i16 { + // CHECK-LABEL: fn match_i8_i16_failed_2_a( + // CHECK: switchInt + match i { + EnumAi8::A => 255, + EnumAi8::B => 2, + EnumAi8::C => -3, + } +} + +// EMIT_MIR matches_reduce_branches.match_i8_i16_failed_2_b.MatchBranchSimplification.diff +fn match_i8_i16_failed_2_b(i: EnumAi8) -> i16 { + // CHECK-LABEL: fn match_i8_i16_failed_2_b( + // CHECK: switchInt + match i { + EnumAi8::A => -1, + EnumAi8::B => 2, + EnumAi8::C => 253, + } +} + #[repr(i16)] enum EnumAi16 { A = -1, @@ -253,12 +277,11 @@ enum EnumAi128 { D = -1, } +// FIXME: This transform is reasonable. // EMIT_MIR matches_reduce_branches.match_i128_u128.MatchBranchSimplification.diff fn match_i128_u128(i: EnumAi128) -> u128 { // CHECK-LABEL: fn match_i128_u128( - // CHECK-NOT: switchInt - // CHECK: _0 = _3 as u128 (IntToInt); - // CHECH: return + // CHECK: switchInt match i { EnumAi128::A => 1, EnumAi128::B => 2, From b894e53bae4f9775269f437e0f571fb3bdf927ff Mon Sep 17 00:00:00 2001 From: DianQK Date: Thu, 18 Apr 2024 23:06:40 +0800 Subject: [PATCH 2/3] Add comments for `int_equal` --- compiler/rustc_mir_transform/src/match_branches.rs | 2 ++ 1 file changed, 2 insertions(+) diff --git a/compiler/rustc_mir_transform/src/match_branches.rs b/compiler/rustc_mir_transform/src/match_branches.rs index 62d6525e4394e..cb53885b7a116 100644 --- a/compiler/rustc_mir_transform/src/match_branches.rs +++ b/compiler/rustc_mir_transform/src/match_branches.rs @@ -368,6 +368,8 @@ impl<'tcx> SimplifyMatch<'tcx> for SimplifyToExp { return None; } + // For signed comparisons, we need to consider different bit widths, + // so we need to transform to i128 for comparison. fn int_equal(l: ScalarInt, r: impl Into, size: Size) -> bool { l.try_to_int(l.size()).unwrap() == ScalarInt::try_from_uint(r, size).unwrap().try_to_int(size).unwrap() From f38b16e6d13fb79195d3c4aefe4101508f82a0b6 Mon Sep 17 00:00:00 2001 From: Quentin Dian Date: Fri, 19 Apr 2024 07:50:37 +0800 Subject: [PATCH 3/3] Explain what happens in `match_i8_i16_failed_2_a` Co-authored-by: Ralf Jung --- tests/mir-opt/matches_reduce_branches.rs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/tests/mir-opt/matches_reduce_branches.rs b/tests/mir-opt/matches_reduce_branches.rs index d81fd1d4fcdae..e8cc072490644 100644 --- a/tests/mir-opt/matches_reduce_branches.rs +++ b/tests/mir-opt/matches_reduce_branches.rs @@ -226,7 +226,8 @@ fn match_i8_i16_failed(i: EnumAi8) -> i16 { } // We cannot transform it, even though `-1i8` and `255i16` are the same in terms of bits, -// what we actually require is that they are considered equal in a signed comparison. +// what we actually require is that they are considered equal in a signed comparison, +// after sign-extending to the larger type. // EMIT_MIR matches_reduce_branches.match_i8_i16_failed_2_a.MatchBranchSimplification.diff fn match_i8_i16_failed_2_a(i: EnumAi8) -> i16 { // CHECK-LABEL: fn match_i8_i16_failed_2_a(