From f5db46c97bf02647f76a98290ec4198754fed4ae Mon Sep 17 00:00:00 2001 From: Scott McMurray Date: Sat, 22 Jun 2024 23:00:44 -0700 Subject: [PATCH 1/2] Also get `add nuw` from `uN::checked_add` --- library/core/src/num/uint_macros.rs | 15 +++++- tests/codegen/checked_math.rs | 14 +++++ ...p_forward.PreCodegen.after.panic-abort.mir | 53 +++++++------------ ..._forward.PreCodegen.after.panic-unwind.mir | 53 +++++++------------ 4 files changed, 67 insertions(+), 68 deletions(-) diff --git a/library/core/src/num/uint_macros.rs b/library/core/src/num/uint_macros.rs index 00450c2cda3e6..7c7a2e921e80e 100644 --- a/library/core/src/num/uint_macros.rs +++ b/library/core/src/num/uint_macros.rs @@ -455,8 +455,19 @@ macro_rules! uint_impl { without modifying the original"] #[inline] pub const fn checked_add(self, rhs: Self) -> Option { - 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 , + // 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 diff --git a/tests/codegen/checked_math.rs b/tests/codegen/checked_math.rs index 41016e3b7bec4..75df5866d6ea0 100644 --- a/tests/codegen/checked_math.rs +++ b/tests/codegen/checked_math.rs @@ -84,3 +84,17 @@ pub fn checked_shr_signed(a: i32, b: u32) -> Option { // 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() +} diff --git a/tests/mir-opt/pre-codegen/checked_ops.step_forward.PreCodegen.after.panic-abort.mir b/tests/mir-opt/pre-codegen/checked_ops.step_forward.PreCodegen.after.panic-abort.mir index e31a8cb693704..8b568d825b244 100644 --- a/tests/mir-opt/pre-codegen/checked_ops.step_forward.PreCodegen.after.panic-abort.mir +++ b/tests/mir-opt/pre-codegen/checked_ops.step_forward.PreCodegen.after.panic-abort.mir @@ -5,21 +5,14 @@ fn step_forward(_1: u16, _2: usize) -> u16 { debug n => _2; let mut _0: u16; scope 1 (inlined ::forward) { - let mut _8: u16; + let mut _7: u16; scope 2 { } scope 3 (inlined ::forward_checked) { scope 4 { scope 6 (inlined core::num::::checked_add) { - let mut _7: bool; - scope 7 { - } - scope 8 (inlined core::num::::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:: for u16>::try_from) { @@ -27,11 +20,11 @@ fn step_forward(_1: u16, _2: usize) -> u16 { let mut _4: u16; } } - scope 10 (inlined Option::::is_none) { - scope 11 (inlined Option::::is_some) { + scope 7 (inlined Option::::is_none) { + scope 8 (inlined Option::::is_some) { } } - scope 12 (inlined core::num::::wrapping_add) { + scope 9 (inlined core::num::::wrapping_add) { } } @@ -39,7 +32,7 @@ fn step_forward(_1: u16, _2: usize) -> u16 { 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: { @@ -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::::MAX, const 1_u16) -> [success: bb6, unwind unreachable]; } bb6: { - assert(!const true, "attempt to compute `{} + {}`, which would overflow", const core::num::::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; } diff --git a/tests/mir-opt/pre-codegen/checked_ops.step_forward.PreCodegen.after.panic-unwind.mir b/tests/mir-opt/pre-codegen/checked_ops.step_forward.PreCodegen.after.panic-unwind.mir index 8cc9be27e21d8..2120699e39f51 100644 --- a/tests/mir-opt/pre-codegen/checked_ops.step_forward.PreCodegen.after.panic-unwind.mir +++ b/tests/mir-opt/pre-codegen/checked_ops.step_forward.PreCodegen.after.panic-unwind.mir @@ -5,21 +5,14 @@ fn step_forward(_1: u16, _2: usize) -> u16 { debug n => _2; let mut _0: u16; scope 1 (inlined ::forward) { - let mut _8: u16; + let mut _7: u16; scope 2 { } scope 3 (inlined ::forward_checked) { scope 4 { scope 6 (inlined core::num::::checked_add) { - let mut _7: bool; - scope 7 { - } - scope 8 (inlined core::num::::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:: for u16>::try_from) { @@ -27,11 +20,11 @@ fn step_forward(_1: u16, _2: usize) -> u16 { let mut _4: u16; } } - scope 10 (inlined Option::::is_none) { - scope 11 (inlined Option::::is_some) { + scope 7 (inlined Option::::is_none) { + scope 8 (inlined Option::::is_some) { } } - scope 12 (inlined core::num::::wrapping_add) { + scope 9 (inlined core::num::::wrapping_add) { } } @@ -39,7 +32,7 @@ fn step_forward(_1: u16, _2: usize) -> u16 { 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: { @@ -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::::MAX, const 1_u16) -> [success: bb6, unwind continue]; } bb6: { - assert(!const true, "attempt to compute `{} + {}`, which would overflow", const core::num::::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; } From a53dd2fc717d55216e183279e1658e3f770abff0 Mon Sep 17 00:00:00 2001 From: Scott McMurray Date: Sun, 23 Jun 2024 10:26:05 -0700 Subject: [PATCH 2/2] Is it worth keeping the `unlikely!`? --- library/core/src/num/uint_macros.rs | 2 +- ...p_forward.PreCodegen.after.panic-abort.mir | 36 +++++++++++-------- ..._forward.PreCodegen.after.panic-unwind.mir | 36 +++++++++++-------- 3 files changed, 45 insertions(+), 29 deletions(-) diff --git a/library/core/src/num/uint_macros.rs b/library/core/src/num/uint_macros.rs index 7c7a2e921e80e..42f23f7ed71a9 100644 --- a/library/core/src/num/uint_macros.rs +++ b/library/core/src/num/uint_macros.rs @@ -462,7 +462,7 @@ macro_rules! uint_impl { // Per , // LLVM is happy to re-form the intrinsic later if useful. - if intrinsics::add_with_overflow(self, rhs).1 { + if unlikely!(intrinsics::add_with_overflow(self, rhs).1) { None } else { // SAFETY: Just checked it doesn't overflow diff --git a/tests/mir-opt/pre-codegen/checked_ops.step_forward.PreCodegen.after.panic-abort.mir b/tests/mir-opt/pre-codegen/checked_ops.step_forward.PreCodegen.after.panic-abort.mir index 8b568d825b244..69c11ebcacced 100644 --- a/tests/mir-opt/pre-codegen/checked_ops.step_forward.PreCodegen.after.panic-abort.mir +++ b/tests/mir-opt/pre-codegen/checked_ops.step_forward.PreCodegen.after.panic-abort.mir @@ -5,7 +5,7 @@ fn step_forward(_1: u16, _2: usize) -> u16 { debug n => _2; let mut _0: u16; scope 1 (inlined ::forward) { - let mut _7: u16; + let mut _8: u16; scope 2 { } scope 3 (inlined ::forward_checked) { @@ -13,6 +13,7 @@ fn step_forward(_1: u16, _2: usize) -> u16 { scope 6 (inlined core::num::::checked_add) { let mut _5: (u16, bool); let mut _6: bool; + let mut _7: bool; } } scope 5 (inlined convert::num::ptr_try_from_impls:: for u16>::try_from) { @@ -32,45 +33,52 @@ fn step_forward(_1: u16, _2: usize) -> u16 { StorageLive(_4); StorageLive(_3); _3 = Gt(_2, const 65535_usize); - switchInt(move _3) -> [0: bb1, otherwise: bb4]; + switchInt(move _3) -> [0: bb1, otherwise: bb5]; } bb1: { _4 = _2 as u16 (IntToInt); StorageDead(_3); + StorageLive(_7); StorageLive(_6); StorageLive(_5); _5 = AddWithOverflow(_1, _4); _6 = (_5.1: bool); - switchInt(move _6) -> [0: bb2, otherwise: bb3]; + _7 = unlikely(move _6) -> [return: bb2, unwind unreachable]; } bb2: { - StorageDead(_5); - StorageDead(_6); - goto -> bb6; + switchInt(move _7) -> [0: bb3, otherwise: bb4]; } bb3: { StorageDead(_5); StorageDead(_6); - goto -> bb5; + StorageDead(_7); + goto -> bb7; } bb4: { - StorageDead(_3); - goto -> bb5; + StorageDead(_5); + StorageDead(_6); + StorageDead(_7); + goto -> bb6; } bb5: { - assert(!const true, "attempt to compute `{} + {}`, which would overflow", const core::num::::MAX, const 1_u16) -> [success: bb6, unwind unreachable]; + StorageDead(_3); + goto -> bb6; } bb6: { - StorageLive(_7); - _7 = _2 as u16 (IntToInt); - _0 = Add(_1, _7); - StorageDead(_7); + assert(!const true, "attempt to compute `{} + {}`, which would overflow", const core::num::::MAX, const 1_u16) -> [success: bb7, unwind unreachable]; + } + + bb7: { + StorageLive(_8); + _8 = _2 as u16 (IntToInt); + _0 = Add(_1, _8); + StorageDead(_8); StorageDead(_4); return; } diff --git a/tests/mir-opt/pre-codegen/checked_ops.step_forward.PreCodegen.after.panic-unwind.mir b/tests/mir-opt/pre-codegen/checked_ops.step_forward.PreCodegen.after.panic-unwind.mir index 2120699e39f51..e6ea6c510019c 100644 --- a/tests/mir-opt/pre-codegen/checked_ops.step_forward.PreCodegen.after.panic-unwind.mir +++ b/tests/mir-opt/pre-codegen/checked_ops.step_forward.PreCodegen.after.panic-unwind.mir @@ -5,7 +5,7 @@ fn step_forward(_1: u16, _2: usize) -> u16 { debug n => _2; let mut _0: u16; scope 1 (inlined ::forward) { - let mut _7: u16; + let mut _8: u16; scope 2 { } scope 3 (inlined ::forward_checked) { @@ -13,6 +13,7 @@ fn step_forward(_1: u16, _2: usize) -> u16 { scope 6 (inlined core::num::::checked_add) { let mut _5: (u16, bool); let mut _6: bool; + let mut _7: bool; } } scope 5 (inlined convert::num::ptr_try_from_impls:: for u16>::try_from) { @@ -32,45 +33,52 @@ fn step_forward(_1: u16, _2: usize) -> u16 { StorageLive(_4); StorageLive(_3); _3 = Gt(_2, const 65535_usize); - switchInt(move _3) -> [0: bb1, otherwise: bb4]; + switchInt(move _3) -> [0: bb1, otherwise: bb5]; } bb1: { _4 = _2 as u16 (IntToInt); StorageDead(_3); + StorageLive(_7); StorageLive(_6); StorageLive(_5); _5 = AddWithOverflow(_1, _4); _6 = (_5.1: bool); - switchInt(move _6) -> [0: bb2, otherwise: bb3]; + _7 = unlikely(move _6) -> [return: bb2, unwind unreachable]; } bb2: { - StorageDead(_5); - StorageDead(_6); - goto -> bb6; + switchInt(move _7) -> [0: bb3, otherwise: bb4]; } bb3: { StorageDead(_5); StorageDead(_6); - goto -> bb5; + StorageDead(_7); + goto -> bb7; } bb4: { - StorageDead(_3); - goto -> bb5; + StorageDead(_5); + StorageDead(_6); + StorageDead(_7); + goto -> bb6; } bb5: { - assert(!const true, "attempt to compute `{} + {}`, which would overflow", const core::num::::MAX, const 1_u16) -> [success: bb6, unwind continue]; + StorageDead(_3); + goto -> bb6; } bb6: { - StorageLive(_7); - _7 = _2 as u16 (IntToInt); - _0 = Add(_1, _7); - StorageDead(_7); + assert(!const true, "attempt to compute `{} + {}`, which would overflow", const core::num::::MAX, const 1_u16) -> [success: bb7, unwind continue]; + } + + bb7: { + StorageLive(_8); + _8 = _2 as u16 (IntToInt); + _0 = Add(_1, _8); + StorageDead(_8); StorageDead(_4); return; }