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.

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

Tidy up bigint multiplication implementations #132195

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

clarfonthey
Copy link
Contributor

This tidies up the library version of the bigint multiplication methods, in preparation for eventually adding intrinsic versions. 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, carry1: uN, carry2: 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 carrying2_mul(self, rhs: iN, carry1: iN, carry2: 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.

Justifications for separated _impl methods

Right now, we know that the performance of these methods is less than ideal, since bigint multiplication is definitely an area where having an intrinsic will improve optimisation. Because of the requirement for a distinct implementation for u128, I decided to simply have these separate methods containing the actual implementation and leave the documentation and doc tests inside the macros used to define the methods for all types, so that the documentation between the different versions cannot drift. Once an intrinsic version exists on the bootstrap compiler, the "impl" calls will be replaced with intrinsic calls in all cases.

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 carrying2_mul 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. This should help bolster confidence that the naive implementations are actually correct.

Note on naming

I know that carrying2_mul feels like an awful name, and it likely won't be the final name for this function. The goal here is to offer this function in some form in the library so that the implementation is there and accepted, even though the name of the function will likely change before stabilisation. Remember that all of this is still unstable and these will all be discussed before stabilisation.

@rustbot
Copy link
Collaborator

rustbot commented Oct 26, 2024

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

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Oct 26, 2024
@clarfonthey
Copy link
Contributor Author

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

@rustbot rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Oct 26, 2024
@rust-log-analyzer

This comment has been minimized.

@jhpratt
Copy link
Member

jhpratt commented Oct 26, 2024

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

@bors
Copy link
Contributor

bors commented Oct 27, 2024

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

@clarfonthey
Copy link
Contributor Author

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.

@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@clarfonthey
Copy link
Contributor Author

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

@rustbot review

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Oct 28, 2024
@clarfonthey clarfonthey marked this pull request as ready for review October 28, 2024 22:21
($left:expr, $right:expr$(, $($arg:tt)+)?) => {
{
fn runtime() {
assert_eq!($left, $right, $($arg)*);
assert_eq!($left, $right, $($($arg)*),*);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Comment on lines +112 to +113
($left:expr, $right:expr) => {
assert_eq_const_safe!($left, $right, concat!(stringify!($left), " == ", stringify!($right)));
};
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This helped with debugging, so, I kept it.

@bors
Copy link
Contributor

bors commented Oct 30, 2024

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

@bors
Copy link
Contributor

bors commented Nov 3, 2024

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs Relevant to the library team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants