Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Reimplement carrying_add and borrowing_sub for signed integers. #93873

Merged
merged 3 commits into from
Sep 9, 2022
Merged

Reimplement carrying_add and borrowing_sub for signed integers. #93873

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
1266099
Implement carrying_add and borrowing_sub on signed numbers
Stovent Feb 10, 2022
5c69055
Correct signed bit int documentation
Stovent Feb 11, 2022
b998d82
Remove too long example
Stovent Feb 11, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
78 changes: 78 additions & 0 deletions library/core/src/num/int_macros.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1512,6 +1512,51 @@ macro_rules! int_impl {
(a as Self, b)
}

/// Calculates `self + rhs + carry` without the ability to overflow.
///
/// Performs "signed ternary addition" which takes in an extra bit to add, and may return an
/// additional bit of overflow. This signed function is used only on the highest-ordered data,
/// for which the signed overflow result indicates whether the big integer overflowed or not.
///
/// # Examples
///
/// Basic usage:
///
/// ```
/// #![feature(bigint_helper_methods)]
#[doc = concat!("assert_eq!(5", stringify!($SelfT), ".carrying_add(2, false), (7, false));")]
#[doc = concat!("assert_eq!(5", stringify!($SelfT), ".carrying_add(2, true), (8, false));")]
#[doc = concat!("assert_eq!(", stringify!($SelfT), "::MAX.carrying_add(1, false), (", stringify!($SelfT), "::MIN, true));")]
#[doc = concat!("assert_eq!(", stringify!($SelfT), "::MAX.carrying_add(0, true), (", stringify!($SelfT), "::MIN, true));")]
#[doc = concat!("assert_eq!(", stringify!($SelfT), "::MAX.carrying_add(1, true), (", stringify!($SelfT), "::MIN + 1, true));")]
#[doc = concat!("assert_eq!(",
stringify!($SelfT), "::MAX.carrying_add(", stringify!($SelfT), "::MAX, true), ",
"(-1, true));"
)]
#[doc = concat!("assert_eq!(", stringify!($SelfT), "::MIN.carrying_add(-1, true), (", stringify!($SelfT), "::MIN, false));")]
#[doc = concat!("assert_eq!(0", stringify!($SelfT), ".carrying_add(", stringify!($SelfT), "::MAX, true), (", stringify!($SelfT), "::MIN, true));")]
/// ```
///
/// If `carry` is false, this method is equivalent to [`overflowing_add`](Self::overflowing_add):
///
/// ```
/// #![feature(bigint_helper_methods)]
#[doc = concat!("assert_eq!(5_", stringify!($SelfT), ".carrying_add(2, false), 5_", stringify!($SelfT), ".overflowing_add(2));")]
#[doc = concat!("assert_eq!(", stringify!($SelfT), "::MAX.carrying_add(1, false), ", stringify!($SelfT), "::MAX.overflowing_add(1));")]
/// ```
#[unstable(feature = "bigint_helper_methods", issue = "85532")]
#[rustc_const_unstable(feature = "const_bigint_helper_methods", issue = "85532")]
#[must_use = "this returns the result of the operation, \
without modifying the original"]
#[inline]
pub const fn carrying_add(self, rhs: Self, carry: bool) -> (Self, bool) {
// note: longer-term this should be done via an intrinsic.
// note: no intermediate overflow is required (https://github.com/rust-lang/rust/issues/85532#issuecomment-1032214946).
let (a, b) = self.overflowing_add(rhs);
let (c, d) = a.overflowing_add(carry as $SelfT);
(c, b != d)
}

/// Calculates `self` + `rhs` with an unsigned `rhs`
///
/// Returns a tuple of the addition along with a boolean indicating
Expand Down Expand Up @@ -1563,6 +1608,39 @@ macro_rules! int_impl {
(a as Self, b)
}

/// Calculates `self - rhs - borrow` without the ability to overflow.
///
/// Performs "signed ternary subtraction" which takes in an extra bit to subtract, and may return an
/// additional bit of overflow. This signed function is used only on the highest-ordered data,
/// for which the signed overflow result indicates whether the big integer overflowed or not.
///
/// # Examples
///
/// Basic usage:
///
/// ```
/// #![feature(bigint_helper_methods)]
#[doc = concat!("assert_eq!(5", stringify!($SelfT), ".borrowing_sub(2, false), (3, false));")]
#[doc = concat!("assert_eq!(5", stringify!($SelfT), ".borrowing_sub(2, true), (2, false));")]
#[doc = concat!("assert_eq!(0", stringify!($SelfT), ".borrowing_sub(1, false), (-1, false));")]
#[doc = concat!("assert_eq!(0", stringify!($SelfT), ".borrowing_sub(1, true), (-2, false));")]
#[doc = concat!("assert_eq!(", stringify!($SelfT), "::MIN.borrowing_sub(1, true), (", stringify!($SelfT), "::MAX - 1, true));")]
#[doc = concat!("assert_eq!(", stringify!($SelfT), "::MAX.borrowing_sub(-1, false), (", stringify!($SelfT), "::MIN, true));")]
#[doc = concat!("assert_eq!(", stringify!($SelfT), "::MAX.borrowing_sub(-1, true), (", stringify!($SelfT), "::MAX, false));")]
/// ```
#[unstable(feature = "bigint_helper_methods", issue = "85532")]
#[rustc_const_unstable(feature = "const_bigint_helper_methods", issue = "85532")]
#[must_use = "this returns the result of the operation, \
without modifying the original"]
#[inline]
pub const fn borrowing_sub(self, rhs: Self, borrow: bool) -> (Self, bool) {
// note: longer-term this should be done via an intrinsic.
// note: no intermediate overflow is required (https://github.com/rust-lang/rust/issues/85532#issuecomment-1032214946).
let (a, b) = self.overflowing_sub(rhs);
let (c, d) = a.overflowing_sub(borrow as $SelfT);
(c, b != d)
Comment on lines +1637 to +1641
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This doesn't generate optimal code for aarch64 targets:
#85532 (comment)

}

/// Calculates `self` - `rhs` with an unsigned `rhs`
///
/// Returns a tuple of the subtraction along with a boolean indicating
Expand Down
1 change: 1 addition & 0 deletions library/core/tests/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
#![feature(array_methods)]
#![feature(array_windows)]
#![feature(bench_black_box)]
#![feature(bigint_helper_methods)]
#![feature(cell_update)]
#![feature(const_assume)]
#![feature(const_black_box)]
Expand Down
26 changes: 26 additions & 0 deletions library/core/tests/num/int_macros.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -338,6 +338,32 @@ macro_rules! int_module {
assert_eq!(MIN.checked_next_multiple_of(-3), None);
assert_eq!(MIN.checked_next_multiple_of(-1), Some(MIN));
}

#[test]
fn test_carrying_add() {
assert_eq!($T::MAX.carrying_add(1, false), ($T::MIN, true));
assert_eq!($T::MAX.carrying_add(0, true), ($T::MIN, true));
assert_eq!($T::MAX.carrying_add(1, true), ($T::MIN + 1, true));
assert_eq!($T::MAX.carrying_add(-1, false), ($T::MAX - 1, false));
assert_eq!($T::MAX.carrying_add(-1, true), ($T::MAX, false)); // no intermediate overflow
assert_eq!($T::MIN.carrying_add(-1, false), ($T::MAX, true));
Copy link
Member

@scottmcm scottmcm Feb 10, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I totally agree with you that these not reporting "internal" overflows is the way to go.

The piece that's still unclear to me: how is it helpful for something labelled bigint_helper_methods for both MIN + -1 and MAX + 1 to have the same true carry? It seems to me like it's fundamentally not enough information to "allows for chaining together multiple additions" (like the doc comment says the method can be used to do).

Said otherwise, I can't see a way that it's useful to pass the output bool back into the input bool of another call. Which makes this seem like a less-necessary ternary overflowing_add rather than anything that can be used for "carrying".

Copy link
Contributor Author

@Stovent Stovent Feb 10, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the example I give in #85532 (comment), to implement the overflowing_add/_sub operations on bit ints, you first need to perform the add operation on the unsigned low order data first.
When carrying_add/borrowing_sub to/from the highest order data, which is signed for signed big ints, the input bool comes from an unsigned overflow, but the output inform on signed overflow, which is exactly the return value necessary to implement the overflowing_add/_sub on bit ints.

To summarize, when implementing bit ints, the workflow is to chain the unsigned overflow from the low order data to the higher order ones, and when performing the last operation, the result is not chained, but in fact used to indicate what happened during the operation.
In other words, signed overflow will never be chained, because it's not indented to. It's intended to inform on the result of the overall operation.

In my experience with the Motorola 68000, this is the intended behavior on big ints. The unsigned overflow flag is chained across ADD operations to perform the addition on a bit int. During the chaining, the signed overflow flag is not used because it is not valid. It becomes valid after the last ADD instruction occurred, because the big int implemented is signed.

I hope this explanation is clear.

Copy link
Contributor Author

@Stovent Stovent Feb 11, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems to me like it's fundamentally not enough information to "allows for chaining together multiple additions" (like the doc comment says the method can be used to do).

Sorry I did not see this part of your message yesterday. I indeed forgot to update the documentation to explain why signed carrying methods should not be chained, I just updated it.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, maybe it would help the conversation to have carrying_mul implemented for signed numbers too?

What would the signature be for that? If I'm following your comment, it sounds like it would take in an unsigned carry, because it could come from uN::carrying_mul? And then what would the outgoing carry be? Is it just a bit again? Or would it be signed?

Basically, if the unsigned version is https://doc.rust-lang.org/std/primitive.u32.html#method.carrying_mul

fn carrying_mul(multiplier: u32, multiplicand: u32, carry: u32) -> (u32 /* product */, u32 /*carry*/)

Then it would seem weird to me if the signed version were

fn carrying_mul(multiplier: i32, multiplicand: i32, carry: u32) -> (i32 /* product */, bool /*carry*/)

Copy link
Contributor Author

@Stovent Stovent Feb 13, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Based on an example I made, the implementations are way different between multiplication and addition/subtraction. I think the signature of carrying_mul (and by extension widening_mul too) would be

fn carrying_mul(multiplier: i32, multiplicand: i32, carry: i32) -> (i32 /* product */, i32 /* carry */);
fn widening_mul(multiplier: i32, multiplicand: i32) -> (i32 /* product */, i32 /* carry */);

You can see my own implementation of widening_mul on a I16 big int here : https://play.rust-lang.org/?version=nightly&mode=release&edition=2021&gist=065c022198027bdba3a3ffc4f90e62e2
It is the same code for both signed and unsigned big ints.

The idea of the implementation is that multiplying two big ints together (for example I16, which is made of an u8 and an i8): lhs * rhs results in computing (lhs.high + lhs.low) * (rhs.high + rhs.low). When we distribute the factors, we get

lhs.high * rhs.high + lhs.high * rhs.low + lhs.low * rhs.high + lhs.low * rhs.low

What may look strange in signed contexts is the fact that the multiplicand is cast from unsigned to signed before being used, but it is mandatory to use signed semantics to obtain the correct result.

If you want to I can provide a complete breakdown on what happens when running this code on both a signed big int and an unsigned big int.

assert_eq!($T::MIN.carrying_add(-1, true), ($T::MIN, false)); // no intermediate overflow
assert_eq!((0 as $T).carrying_add($T::MAX, true), ($T::MIN, true));
assert_eq!((0 as $T).carrying_add($T::MIN, true), ($T::MIN + 1, false));
}

#[test]
fn test_borrowing_sub() {
assert_eq!($T::MIN.borrowing_sub(1, false), ($T::MAX, true));
assert_eq!($T::MIN.borrowing_sub(0, true), ($T::MAX, true));
assert_eq!($T::MIN.borrowing_sub(1, true), ($T::MAX - 1, true));
assert_eq!($T::MIN.borrowing_sub(-1, false), ($T::MIN + 1, false));
assert_eq!($T::MIN.borrowing_sub(-1, true), ($T::MIN, false)); // no intermediate overflow
assert_eq!($T::MAX.borrowing_sub(-1, false), ($T::MIN, true));
assert_eq!($T::MAX.borrowing_sub(-1, true), ($T::MAX, false)); // no intermediate overflow
assert_eq!((0 as $T).borrowing_sub($T::MIN, false), ($T::MIN, true));
assert_eq!((0 as $T).borrowing_sub($T::MIN, true), ($T::MAX, false));
}
}
};
}
22 changes: 22 additions & 0 deletions library/core/tests/num/uint_macros.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -230,6 +230,28 @@ macro_rules! uint_module {
assert_eq!((1 as $T).checked_next_multiple_of(0), None);
assert_eq!(MAX.checked_next_multiple_of(2), None);
}

#[test]
fn test_carrying_add() {
assert_eq!($T::MAX.carrying_add(1, false), (0, true));
assert_eq!($T::MAX.carrying_add(0, true), (0, true));
assert_eq!($T::MAX.carrying_add(1, true), (1, true));

assert_eq!($T::MIN.carrying_add($T::MAX, false), ($T::MAX, false));
assert_eq!($T::MIN.carrying_add(0, true), (1, false));
assert_eq!($T::MIN.carrying_add($T::MAX, true), (0, true));
}

#[test]
fn test_borrowing_sub() {
assert_eq!($T::MIN.borrowing_sub(1, false), ($T::MAX, true));
assert_eq!($T::MIN.borrowing_sub(0, true), ($T::MAX, true));
assert_eq!($T::MIN.borrowing_sub(1, true), ($T::MAX - 1, true));

assert_eq!($T::MAX.borrowing_sub($T::MAX, false), (0, false));
assert_eq!($T::MAX.borrowing_sub(0, true), ($T::MAX - 1, false));
assert_eq!($T::MAX.borrowing_sub($T::MAX, true), ($T::MAX, true));
}
}
};
}