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

Cross-sizes wide multiplication & Mul trait implementation #226

Closed
wants to merge 27 commits into from

Conversation

ycscaly
Copy link
Contributor

@ycscaly ycscaly commented May 3, 2023

This implements Mul<Self, Output = Self::Concat> for all types as well as Mul<Rhs, Output = Uint<{nlimbs!($self_bits) + nlimbs!($rhs_bits)}>> for all types.

This is possible because mul_wide now takes a second generic paramater:

pub const fn mul_wide<const RHS_LIMBS: usize>(
        &self,
        rhs: &Uint<RHS_LIMBS>,
    ) -> (Uint<RHS_LIMBS>, Self);

One could debate whether this is a breaking change, for this changes the signature of a public function, but I'd argue against that, for Rust allows type inference in a way that simplifies down to the exact same signature for the special case where Rhs = Self which was the case for mul_wide() before this change.

This means that I did not need to change any code that used mul_wide, and for example, U64::ONE.mul_wide(&U64::ZERO) compiles without needing to specify the generic parameters.

In order to implement this, I needed to also implement cross-sizes concatenation, which is why I already merged #224 into this PR, so it should be reviewed & approved before this one.

Resolves #209

@tarcieri
Copy link
Member

tarcieri commented May 3, 2023

This feels like a combinatorial explosion. Are you checking how it impacts compile times?

@ycscaly
Copy link
Contributor Author

ycscaly commented May 3, 2023

This feels like a combinatorial explosion. Are you checking how it impacts compile times?

On my PC it's an increase from 1.47s to 4.98s:
Before change:

➜  crypto-bigint git:(master) ✗ cargo clean
➜  crypto-bigint git:(master) ✗ cargo build
   Compiling libc v0.2.139
   Compiling cfg-if v1.0.0
   Compiling subtle v2.5.0
   Compiling getrandom v0.2.8
   Compiling rand_core v0.6.4
   Compiling crypto-bigint v0.5.2 (/Users/jcscaly/projects/crypto-bigint)
    Finished dev [unoptimized + debuginfo] target(s) in 1.47s
➜  crypto-bigint git:(master) ✗

After change:

➜  crypto-bigint git:(mul) ✗ cargo clean
➜  crypto-bigint git:(mul) ✗ cargo build
   Compiling libc v0.2.139
   Compiling cfg-if v1.0.0
   Compiling subtle v2.5.0
   Compiling getrandom v0.2.8
   Compiling rand_core v0.6.4
   Compiling crypto-bigint v0.5.2 (/Users/jcscaly/projects/crypto-bigint)
    Finished dev [unoptimized + debuginfo] target(s) in 4.98s
➜  crypto-bigint git:(mul) ✗

Seems fine to me, but I'll let you decide on that.

The output binary size is roughly 4x as well.

One thing that it does impact is Intellij with its macro-expansion feature, but I think that's also acceptable, again up to you

@tarcieri
Copy link
Member

tarcieri commented May 3, 2023

On my PC it's an increase from 1.47s to 4.98s

Oof, so a ~350% increase.

This really feels like the sort of problem it would be worth waiting for const generics to properly address, rather than providing a "brute force" solution that adds hundreds (thousands?) or trait impls.

@ycscaly
Copy link
Contributor Author

ycscaly commented May 3, 2023

On my PC it's an increase from 1.47s to 4.98s

Oof, so a ~350% increase.

This really feels like the sort of problem it would be worth waiting for const generics to properly address, rather than providing a "brute force" solution that adds hundreds (thousands?) or trait impls.

How would const-generics properly address this?

I am writing my library now, and if I won't have these features I'd have to fork which I really am trying my best to avoid here. I prefer having a temporary solution that will be replaced in the future rather than having nothing.

Would feature-gating this be an acceptable compromise?

Another option would be to export the macro, and not instantiate it with any specific values. So that later on in my crate I can choose the exact types I need and implement the cross-support needed for them specifically by calling this macro. But the macro would still be in crypto-bigint. This would give me the needed functionality, and add no overhead of compilation time / size.

@tarcieri
Copy link
Member

tarcieri commented May 3, 2023

Yes, an off-by-default feature gate would be ok, preferably one that happens prior to the macro being expanded.

Maybe move all of that to its own module and feature gate the module?

@ycscaly
Copy link
Contributor Author

ycscaly commented May 3, 2023

Yes, an off-by-default feature gate would be ok

Do you prefer feature-gating over the option to export the macro only (and not call it within crypto-bigint but within the consumer)?

@tarcieri
Copy link
Member

tarcieri commented May 3, 2023

I don't see how exporting the macro would help. It's adding From impls on types defined in crypto-bigint. Those aren't useful outside the crate.

src/traits.rs Outdated Show resolved Hide resolved
# Conflicts:
#	src/uint/concat.rs
@ycscaly ycscaly requested a review from tarcieri May 3, 2023 14:39
@ycscaly
Copy link
Contributor Author

ycscaly commented May 7, 2023

Replaced by #230

@ycscaly ycscaly closed this May 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Multiplying U2048 by U2048 to get U4096
2 participants