Tidy up bigint multiplication methods

[clarfonthey]

This tidies up the library version of the bigint multiplication methods after the addition of the intrinsics in #133663. It follows this summary of what's desired for these methods.

Note that, if 2H = N, then uH::MAX * uH::MAX + uH::MAX + uH::MAX is uN::MAX, and that we can effectively add two "carry" values without overflowing.

For ease of terminology, the "low-order" or "least significant" or "wrapping" half of multiplication will be called the low part, and the "high-order" or "most significant" or "overflowing" half of multiplication will be called the high part. In all cases, the return convention is (low, high) and left unchanged by this PR, to be litigated later.

API Changes

The original API:

impl uN {
    // computes self * rhs
    pub const fn widening_mul(self, rhs: uN) -> (uN, uN);

    // computes self * rhs + carry
    pub const fn carrying_mul(self, rhs: uN, carry: uN) -> (uN, uN);
}

The added API:

impl uN {
    // computes self * rhs + carry1 + carry2
    pub const fn carrying2_mul(self, rhs: uN, carry: uN, add: uN) -> (uN, uN);
}
impl iN {
    // note that the low part is unsigned
    pub const fn widening_mul(self, rhs: iN) -> (uN, iN);
    pub const fn carrying_mul(self, rhs: iN, carry: iN) -> (uN, iN);
    pub const fn carrying_mul_add(self, rhs: iN, carry: iN, add: iN) -> (uN, iN);
}

Additionally, a naive implementation has been added for u128 and i128 since there are no double-wide types for those. Eventually, an intrinsic will be added to make these more efficient, but rather than doing this all at once, the library changes are added first.

Justifications for API

The unsigned parts are done to ensure consistency with overflowing addition: for a two's complement integer, you want to have unsigned overflow semantics for all parts of the integer except the highest one. This is because overflow for unsigned integers happens on the highest bit (from MAX to zero), whereas overflow for signed integers happens on the second highest bit (from MAX to MIN). Since the sign information only matters in the highest part, we use unsigned overflow for everything but that part.

There is still discussion on the merits of signed bigint addition methods, since getting the behaviour right is very subtle, but at least for signed bigint multiplication, the sign of the operands does make a difference. So, it feels appropriate that at least until we've nailed down the final API, there should be an option to do signed versions of these methods.

Additionally, while it's unclear whether we need all three versions of bigint multiplication (widening, carrying-1, and carrying-2), since it's possible to have up to two carries without overflow, there should at least be a method to allow that. We could potentially only offer the carry-2 method and expect that adding zero carries afterword will optimise correctly, but again, this can be litigated before stabilisation.

Note on documentation

While a lot of care was put into the documentation for the widening_mul and carrying_mul methods on unsigned integers, I have not taken this same care for carrying_mul_add or the signed versions. While I have updated the doc tests to be more appropriate, there will likely be many documentation changes done before stabilisation.

Note on tests

Alongside this change, I've added several tests to ensure that these methods work as expected. These are alongside the codegen tests for the intrinsics.

[rustbot]

r? @jhpratt

rustbot has assigned @jhpratt.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

[clarfonthey]

@rustbot author

Currently drafted since tests are failing and I need to fix them, but I wanted to at least save the stuff I typed up for the description.

[jhpratt]

Let me know when it's ready and I'll take a look. Unless you'd like feedback beforehand, of course.

[bors]

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

[clarfonthey]

Let me know when it's ready and I'll take a look. Unless you'd like feedback beforehand, of course.

Feel free to review the parts of the code that do work, and the API specifically as mentioned in the description. I just know that my naïve implementation for i128 is wrong and haven't fixed it yet. I mostly just wanted to clarify that I do know that it doesn't work, and but wanted to get this PR out in the meantime.

[clarfonthey]

Oh cool, finally got it to work. Glad I added some tests with MIN and MAX to check for overflow.

@rustbot review

[clarfonthey]

This was just wrong before but unused, so, it never triggered any errors.

[clarfonthey]

This helped with debugging, so, I kept it.

[bors]

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

[bors]

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

[scottmcm]

My PR added an intrinsic for a*b+c+d, but didn't expose it, so taking this for that part would be good, and I didn't do anything for signed numbers either. So using this PR for that stuff sounds at least plausible.

[clarfonthey]

@rustbot author

[clarfonthey]

@rustbot ready

I think I've updated everything, including the PR description, after the other PR which added an intrinsic for this. Hopefully everything looks good now.

[scottmcm]

You might want to fix up the OP a bit -- it still says "Eventually, an intrinsic will be added to make these more efficient" and "carrying2_mul", for example.

[scottmcm]

suggestion: would be good to also mention carrying_mul_add here, probably.

[scottmcm]

Actually, all the code looks good here, so

@bors r+

(I would slightly prefer the methods not to be moved around for a simpler diff in uint_macros, but meh.)

[bors]

📌 Commit f228458 has been approved by scottmcm

It is now in the queue for this repository.

[bors]

⌛ Testing commit f228458 with merge d117b7f...

[bors]

☀️ Test successful - checks-actions
Approved by: scottmcm
Pushing d117b7f to master...

[rust-timer]

Finished benchmarking commit (d117b7f): comparison URL.

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

@rustbot label: -perf-regression

Instruction count

This is the most reliable metric that we have; it was used to determine the overall result at the top of this comment. However, even this metric can sometimes exhibit noise.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	0.1%	[0.1%, 0.1%]	2
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-0.4%	[-0.4%, -0.4%]	1
All ❌✅ (primary)	-	-	0

Max RSS (memory usage)

Results (primary -1.2%, secondary -1.0%)

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.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-1.2%	[-1.5%, -1.0%]	2
Improvements ✅ (secondary)	-1.0%	[-1.0%, -1.0%]	1
All ❌✅ (primary)	-1.2%	[-1.5%, -1.0%]	2

Cycles

Results (primary -3.6%, secondary -0.8%)

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.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	2.0%	[2.0%, 2.0%]	1
Improvements ✅ (primary)	-3.6%	[-3.6%, -3.6%]	1
Improvements ✅ (secondary)	-3.6%	[-3.6%, -3.6%]	1
All ❌✅ (primary)	-3.6%	[-3.6%, -3.6%]	1

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 762.584s -> 762.425s (-0.02%)
Artifact size: 325.56 MiB -> 325.57 MiB (0.00%)