Reimplement carrying_add and borrowing_sub for signed integers.

[Stovent]

As per the discussion in #85532, this PR reimplements carrying_add and borrowing_sub for signed integers.

It also adds unit tests for both unsigned and signed integers, emphasing on the behaviours of the methods.

[rust-highfive]

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @scottmcm (or someone else) soon.

Please see the contribution instructions for more information.

[scottmcm]

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".

[Stovent]

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.

[Stovent]

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.

[scottmcm]

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*/)

[Stovent]

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.

[scottmcm]

Hello libs-api folks! I'm looking for your thoughts here.

The signed versions of these methods were originally removed per #90541 because they were originally implemented in a way that made the carries incorrect.

This PR adds them back, and implements them in a way that I (and other in the tracking issue #85532) agree is correctly reporting whether an overflow occurred.

However, I'm looking for a second opinion on these methods anyway as I'm unsure whether they should exist in core. The unsigned ones for chaining together scalar operations make sense to me, but these don't chain. See the long conversation above for more from Stovent about their proposed usage.

[scottmcm]

This was discussed in the libs-api meeting today https://hackmd.io/RADbXvw_Q0edNMKh6Sn8Iw?view

The feedback, as I understood it:

• Please give it a new name to clarify that this is a different meaning to the output than the unsigned version. Since this seems like it's arguably mixing signed and unsigned -- especially for the mul version -- then maybe there's something that could be inspired by Tracking Issue for #![feature(mixed_integer_ops)] #87840.

• Please include a more detailed example that shows how this is used in conjunction with the other bigint_helpers.

(I've also added a note about asm tests to the tracking issue, but that doesn't need to be this PR.)

[Stovent]

I have been a little busy these past weeks, sorry for late answer.

First, I made the following example that uses both the signed and unsigned versions of the carrying / borrowing methods.

#![feature(bigint_helper_methods)]
#![feature(array_zip)]

#[derive(Clone, Copy)]
struct SignedBigInt<const N: usize> {
    pub lows: [u8; N],
    pub high: i8,
}

impl<const N: usize> SignedBigInt<N> {
    /// Subtracts `rhs` from `self` and returns true if signed overflow occurs, false otherwise.
    pub fn overflowing_sub(self, rhs: Self) -> (Self, bool) {
        let mut borrow = false;

        let lows = self.lows.zip(rhs.lows).map(|(left, right)| {
            let (res, b) = left.borrowing_sub(right, borrow);
            borrow = b;
            res
        });

        let (high, b) = self.high.borrowing_sub(rhs.high, borrow);

        (Self { lows, high }, b)
    }
}

fn main() {
    let left = SignedBigInt { // -32_768
        lows: [0],
        high: -128,
    };
    let right = SignedBigInt { // 127
        lows: [127],
        high: 0,
    };
    let (res, borrow) = left.overflowing_sub(right); // -32_895 overflow to 32_641

    assert_eq!(res.high, 0x7F);
    assert_eq!(res.lows[0], 0x81);
    assert_eq!(borrow, true);
}

From my experience, the signed methods are only useful for fixed-size big integers, since dynamically-sized big ints operations do not have overflow as we simply allocate the size needed for the result. If you need further examples, please tell me which points I missed in those I sent.

Second, regarding a potential name change. It is true that the signed methods are often used differently than the unsigned ones. However, since their behaviour is similar, just using signed semantics instead of unsigned ones, I am not sure whether a name change is really justified. If you still want to rename these methods, I personally cannot think of better names than the existing ones, but I am open for suggestions.

[JohnCSimon]

As far as I can tell this is for review...
@rustbot label: -S-waiting-on-author +S-waiting-on-review

[scottmcm]

I'm going to flip this over to a team member to decide:

r? rust-lang/libs

[yaahc]

I'm reassigning this PR because I'm taking a break from the review rotation for a little while. Thank you for your patience.

r? rust-lang/libs-api

[Stovent]

No problem ! I am not in a hurry or anything, I will still be available if needed.

[bors]

☔ The latest upstream changes (presumably #96720) made this pull request unmergeable. Please resolve the merge conflicts.

[bors]

☔ The latest upstream changes (presumably #97548) made this pull request unmergeable. Please resolve the merge conflicts.

[m-ou-se]

Sorry about the huge delay, and thanks for working on this.

These methods look good to me, and considering these should/could be intrinsics at some point, they do seem to belong in the standard library.

I have two comments:

• As mentioned above, I'm not sure about their names, since it isn't immediately clear how u8::borrowing_sub and i8::borrowing_sub are used in differently. I can't think of a better name though, so I don't think we should block this addition on the naming. If anyone can think of better naming, please comment on the tracking issue.

• The documentation comment introduces these as operations "without the ability to overflow", which can be confusing. The way I see it, is that they can overflow, but allow you to handle it. However, this also applies to the already existing (unstable) methods on the unsigned types, so I'll add this as a to do item to the tracking issue to be addressed for all methods.

@bors r+

[bors]

📌 Commit b998d82 has been approved by m-ou-se

It is now in the queue for this repository.

[bors]

⌛ Testing commit b998d82 with merge 7200da0...

[bors]

☀️ Test successful - checks-actions
Approved by: m-ou-se
Pushing 7200da0 to master...

[rust-timer]

Finished benchmarking commit (7200da0): comparison URL.

Overall result: ❌✅ regressions and improvements - no action needed

@rustbot label: -perf-regression

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

	mean1	range	count2
Regressions ❌ (primary)	1.1%	[1.1%, 1.1%]	1
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-4.1%	[-4.1%, -4.1%]	1
All ❌✅ (primary)	1.1%	[1.1%, 1.1%]	1

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean1	range	count2
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	1.6%	[0.6%, 2.6%]	4
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-2.2%	[-2.2%, -2.2%]	1
All ❌✅ (primary)	-	-	0

Cycles

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean1	range	count2
Regressions ❌ (primary)	3.3%	[3.3%, 3.3%]	1
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	3.3%	[3.3%, 3.3%]	1

Footnotes

1. the arithmetic mean of the percent change ↩ ↩² ↩³

2. number of relevant changes ↩ ↩² ↩³

[Lohann]

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