Skip to content

Commit

Permalink
Auto merge of #126852 - scottmcm:more-checked-math-tweaks, r=<try>
Browse files Browse the repository at this point in the history 
Also get `add nuw` from `uN::checked_add`

When I was doing this for `checked_{sub,shl,shr}`, it was mentioned #124114 (comment) that it'd be worth trying for `checked_add` too.

It makes a particularly-big difference for `x.checked_add(C)`, as doing this means that LLVM removes the intrinsic and does it as a normal `x <= MAX - C` instead.

cc `@DianQK` who had commented about `checked_add` related to rust-lang/hashbrown#509 before

cc llvm/llvm-project#80637 for how LLVM is unlikely to do this itself
  • Loading branch information
@bors
bors committed Jun 23, 2024
2 parents d4cc01c + f5db46c commit ffcb98e
Show file tree
Hide file tree
Showing 4 changed files with 67 additions and 68 deletions.
15 changes: 13 additions & 2 deletions library/core/src/num/uint_macros.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -455,8 +455,19 @@ macro_rules! uint_impl {
without modifying the original"]
#[inline]
pub const fn checked_add(self, rhs: Self) -> Option<Self> {
let (a, b) = self.overflowing_add(rhs);
if unlikely!(b) { None } else { Some(a) }
// This used to use `overflowing_add`, but that means it ends up being
// a `wrapping_add`, losing some optimization opportunities. Notably,
// phrasing it this way helps `.checked_add(1)` optimize to a check
// against `MAX` and a `add nuw`.
// Per <https://github.com/rust-lang/rust/pull/124114#issuecomment-2066173305>,
// LLVM is happy to re-form the intrinsic later if useful.

if intrinsics::add_with_overflow(self, rhs).1 {
None
} else {
// SAFETY: Just checked it doesn't overflow
Some(unsafe { intrinsics::unchecked_add(self, rhs) })
}
}

/// Strict integer addition. Computes `self + rhs`, panicking
Expand Down
14 changes: 14 additions & 0 deletions tests/codegen/checked_math.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -84,3 +84,17 @@ pub fn checked_shr_signed(a: i32, b: u32) -> Option<i32> {
// CHECK: ret { i32, i32 } %[[R1]]
a.checked_shr(b)
}

// CHECK-LABEL: @checked_add_one_unwrap_unsigned
// CHECK-SAME: (i32 noundef %x)
#[no_mangle]
pub fn checked_add_one_unwrap_unsigned(x: u32) -> u32 {
// CHECK: %[[IS_MAX:.+]] = icmp eq i32 %x, -1
// CHECK: br i1 %[[IS_MAX]], label %[[NONE_BB:.+]], label %[[SOME_BB:.+]]
// CHECK: [[SOME_BB]]:
// CHECK: %[[R:.+]] = add nuw i32 %x, 1
// CHECK: ret i32 %[[R]]
// CHECK: [[NONE_BB]]:
// CHECK: call {{.+}}unwrap_failed
x.checked_add(1).unwrap()
}
53 changes: 20 additions & 33 deletions tests/mir-opt/pre-codegen/checked_ops.step_forward.PreCodegen.after.panic-abort.mir
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5,41 +5,34 @@ fn step_forward(_1: u16, _2: usize) -> u16 {
debug n => _2;
let mut _0: u16;
scope 1 (inlined <u16 as Step>::forward) {
let mut _8: u16;
let mut _7: u16;
scope 2 {
}
scope 3 (inlined <u16 as Step>::forward_checked) {
scope 4 {
scope 6 (inlined core::num::<impl u16>::checked_add) {
let mut _7: bool;
scope 7 {
}
scope 8 (inlined core::num::<impl u16>::overflowing_add) {
let mut _5: (u16, bool);
let _6: bool;
scope 9 {
}
}
let mut _5: (u16, bool);
let mut _6: bool;
}
}
scope 5 (inlined convert::num::ptr_try_from_impls::<impl TryFrom<usize> for u16>::try_from) {
let mut _3: bool;
let mut _4: u16;
}
}
scope 10 (inlined Option::<u16>::is_none) {
scope 11 (inlined Option::<u16>::is_some) {
scope 7 (inlined Option::<u16>::is_none) {
scope 8 (inlined Option::<u16>::is_some) {
}
}
scope 12 (inlined core::num::<impl u16>::wrapping_add) {
scope 9 (inlined core::num::<impl u16>::wrapping_add) {
}
}

bb0: {
StorageLive(_4);
StorageLive(_3);
_3 = Gt(_2, const 65535_usize);
switchInt(move _3) -> [0: bb1, otherwise: bb5];
switchInt(move _3) -> [0: bb1, otherwise: bb4];
}

bb1: {
Expand All @@ -49,41 +42,35 @@ fn step_forward(_1: u16, _2: usize) -> u16 {
StorageLive(_5);
_5 = AddWithOverflow(_1, _4);
_6 = (_5.1: bool);
StorageDead(_5);
StorageLive(_7);
_7 = unlikely(move _6) -> [return: bb2, unwind unreachable];
switchInt(move _6) -> [0: bb2, otherwise: bb3];
}

bb2: {
switchInt(move _7) -> [0: bb3, otherwise: bb4];
StorageDead(_5);
StorageDead(_6);
goto -> bb6;
}

bb3: {
StorageDead(_7);
StorageDead(_5);
StorageDead(_6);
goto -> bb7;
goto -> bb5;
}

bb4: {
StorageDead(_7);
StorageDead(_6);
goto -> bb6;
StorageDead(_3);
goto -> bb5;
}

bb5: {
StorageDead(_3);
goto -> bb6;
assert(!const true, "attempt to compute `{} + {}`, which would overflow", const core::num::<impl u16>::MAX, const 1_u16) -> [success: bb6, unwind unreachable];
}

bb6: {
assert(!const true, "attempt to compute `{} + {}`, which would overflow", const core::num::<impl u16>::MAX, const 1_u16) -> [success: bb7, unwind unreachable];
}

bb7: {
StorageLive(_8);
_8 = _2 as u16 (IntToInt);
_0 = Add(_1, _8);
StorageDead(_8);
StorageLive(_7);
_7 = _2 as u16 (IntToInt);
_0 = Add(_1, _7);
StorageDead(_7);
StorageDead(_4);
return;
}
Expand Down
53 changes: 20 additions & 33 deletions tests/mir-opt/pre-codegen/checked_ops.step_forward.PreCodegen.after.panic-unwind.mir
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5,41 +5,34 @@ fn step_forward(_1: u16, _2: usize) -> u16 {
debug n => _2;
let mut _0: u16;
scope 1 (inlined <u16 as Step>::forward) {
let mut _8: u16;
let mut _7: u16;
scope 2 {
}
scope 3 (inlined <u16 as Step>::forward_checked) {
scope 4 {
scope 6 (inlined core::num::<impl u16>::checked_add) {
let mut _7: bool;
scope 7 {
}
scope 8 (inlined core::num::<impl u16>::overflowing_add) {
let mut _5: (u16, bool);
let _6: bool;
scope 9 {
}
}
let mut _5: (u16, bool);
let mut _6: bool;
}
}
scope 5 (inlined convert::num::ptr_try_from_impls::<impl TryFrom<usize> for u16>::try_from) {
let mut _3: bool;
let mut _4: u16;
}
}
scope 10 (inlined Option::<u16>::is_none) {
scope 11 (inlined Option::<u16>::is_some) {
scope 7 (inlined Option::<u16>::is_none) {
scope 8 (inlined Option::<u16>::is_some) {
}
}
scope 12 (inlined core::num::<impl u16>::wrapping_add) {
scope 9 (inlined core::num::<impl u16>::wrapping_add) {
}
}

bb0: {
StorageLive(_4);
StorageLive(_3);
_3 = Gt(_2, const 65535_usize);
switchInt(move _3) -> [0: bb1, otherwise: bb5];
switchInt(move _3) -> [0: bb1, otherwise: bb4];
}

bb1: {
Expand All @@ -49,41 +42,35 @@ fn step_forward(_1: u16, _2: usize) -> u16 {
StorageLive(_5);
_5 = AddWithOverflow(_1, _4);
_6 = (_5.1: bool);
StorageDead(_5);
StorageLive(_7);
_7 = unlikely(move _6) -> [return: bb2, unwind unreachable];
switchInt(move _6) -> [0: bb2, otherwise: bb3];
}

bb2: {
switchInt(move _7) -> [0: bb3, otherwise: bb4];
StorageDead(_5);
StorageDead(_6);
goto -> bb6;
}

bb3: {
StorageDead(_7);
StorageDead(_5);
StorageDead(_6);
goto -> bb7;
goto -> bb5;
}

bb4: {
StorageDead(_7);
StorageDead(_6);
goto -> bb6;
StorageDead(_3);
goto -> bb5;
}

bb5: {
StorageDead(_3);
goto -> bb6;
assert(!const true, "attempt to compute `{} + {}`, which would overflow", const core::num::<impl u16>::MAX, const 1_u16) -> [success: bb6, unwind continue];
}

bb6: {
assert(!const true, "attempt to compute `{} + {}`, which would overflow", const core::num::<impl u16>::MAX, const 1_u16) -> [success: bb7, unwind continue];
}

bb7: {
StorageLive(_8);
_8 = _2 as u16 (IntToInt);
_0 = Add(_1, _8);
StorageDead(_8);
StorageLive(_7);
_7 = _2 as u16 (IntToInt);
_0 = Add(_1, _7);
StorageDead(_7);
StorageDead(_4);
return;
}
Expand Down

0 comments on commit ffcb98e

Please sign in to comment.