Skip to content

Add a compiler intrinsic to back bigint_helper_methods#133663

Merged
bors merged 2 commits into
rust-lang:masterfrom
scottmcm:carrying_mul_add
Dec 27, 2024
Merged

Add a compiler intrinsic to back bigint_helper_methods#133663
bors merged 2 commits into
rust-lang:masterfrom
scottmcm:carrying_mul_add

Conversation

@scottmcm

Copy link
Copy Markdown
Member

cc #85532

This adds a new carrying_mul_add intrinsic, to implement wide_mul and carrying_mul.

It has fallback MIR for all types -- including u128, which isn't currently supported on nightly -- so that it'll continue to work on all backends, including CTFE.

Then it's overridden in cg_llvm to use wider intermediate types, including i256 for u128::carrying_mul.

@rustbot

rustbot commented Nov 30, 2024

Copy link
Copy Markdown
Collaborator

r? @ibraheemdev

rustbot has assigned @ibraheemdev.
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-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Nov 30, 2024
@rustbot

rustbot commented Nov 30, 2024

Copy link
Copy Markdown
Collaborator

Some changes occurred to the intrinsics. Make sure the CTFE / Miri interpreter
gets adapted for the changes, if necessary.

cc @rust-lang/miri, @rust-lang/wg-const-eval

Comment thread library/core/src/intrinsics/fallback.rs Outdated
@clarfonthey

Copy link
Copy Markdown
Contributor

Will take a look at this later, but I like that you managed to make fallback intrinsics work here when I couldn't quite get them to work for #132195.

Comment thread compiler/rustc_codegen_llvm/src/intrinsic.rs Outdated
@bors

bors commented Dec 2, 2024

Copy link
Copy Markdown
Collaborator

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

@clarfonthey

Copy link
Copy Markdown
Contributor

Also just to clarify my opinion on where this stands relative to #132195: I think that this PR just adding the intrinsic is okay, although I would like the intrinsic to also handle the signed case, even if it's only used at the moment for the unsigned case. That way, my PR can just update the library methods to use the intrinsic, without having to worry about the slow implementation.

Although the signed version of the library methods will likely return an unsigned word always for the least-significant / high-order part, it vastly simplifies the intrinsic to just have the same type for the arguments and return value, so, I would prefer that we do this specifically for the intrinsic. Since it doesn't actually affect the returned bits, I think that it's okay to just cast half of it unsigned on the library side. You can see how I did this here in a previous (closed) PR.

Otherwise, LGTM. Also, while the codegen test should suffice, if you want to copy over the library tests I added in my PR to verify that the intrinsic works correctly, feel free. I specifically added a bunch of edge cases with MAX and MIN as operands to ensure that things work correctly.

@scottmcm

scottmcm commented Dec 7, 2024

Copy link
Copy Markdown
Member Author

Success! No more "oh my this is hacky", just a couple of impl consts with fairly normal code.

it vastly simplifies the intrinsic to just have the same type for the arguments and return value

It didn't, actually. Once the fallback was working, the implementation in cg_llvm was super-easy -- after all, at that level the signed and unsigned types are the same anyway.

@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

Comment thread compiler/rustc_codegen_llvm/src/intrinsic.rs Outdated
let high = self.trunc(high, narrow_llty);

let pair_llty = self.type_struct(&[narrow_llty, narrow_llty], false);
let pair = self.const_poison(pair_llty);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Just comparing to my version, this doesn't really matter that much, but const_undef feels slightly more accurate here just semantically. Not actually sure if this affects codegen though.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

poison is always preferred where possible, because it doesn't have the same complications as undef. (undef << 1) & 1 is guaranteed to be 0, but (poison << 1) & 1 is still poison. You'll also see that both what we emit for tuples and what the optimizer does for tuples both use poison for this: https://rust.godbolt.org/z/WTnW4GPE6

@scottmcm scottmcm force-pushed the carrying_mul_add branch 2 times, most recently from ee18903 to f636b64 Compare December 8, 2024 02:23
/// => (2²ⁿ - 2ⁿ⁺¹ + 1) + (2ⁿ⁺¹ - 2)
/// => 2²ⁿ - 1
///
/// For `iN`, the upper bound is MIN * MIN + MAX + MAX => 2²ⁿ⁻² + 2ⁿ - 2,

@scottmcm scottmcm Dec 8, 2024

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Pop Quiz: after a double-width isize::MIN * isize::MIN (which is the largest possible positive result from a multiplication), how many more isize::MAXs can you add to the result before it would need to carry to triple-width?

Answer: isize::MAX + 2 of them!

(The carrying_mul_add really feels so much nicer for unsigned types, since uN::MAX * uN::MAX + uN::MAX + uN::MAX == u2N::MAX -- no more space available after that.)

@ibraheemdev

Copy link
Copy Markdown
Member

This is outside my territory so I'm going to pass this along. r? libs

@rustbot rustbot assigned Amanieu and unassigned ibraheemdev Dec 13, 2024
@bors

bors commented Dec 20, 2024

Copy link
Copy Markdown
Collaborator

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

@rust-log-analyzer

This comment has been minimized.

@bors

bors commented Dec 27, 2024

Copy link
Copy Markdown
Collaborator

💔 Test failed - checks-actions

@bors bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Dec 27, 2024
Including implementing it for `u128`, so it can be defined in `uint_impl!`.

This way it works for all backends, including CTFE.
@scottmcm

Copy link
Copy Markdown
Member Author

Fix was just changing some i64s to {{i32|i64}}s in GEPs.

@bors r=Amanieu

@bors

bors commented Dec 27, 2024

Copy link
Copy Markdown
Collaborator

📌 Commit 4669c0d has been approved by Amanieu

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Dec 27, 2024
@bors bors merged commit 95e66ff into rust-lang:master Dec 27, 2024
@rustbot rustbot added this to the 1.85.0 milestone Dec 27, 2024
@scottmcm scottmcm deleted the carrying_mul_add branch December 29, 2024 19:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. 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.

9 participants