Skip to content

Conversation

@benbencik
Copy link

@benbencik benbencik commented Oct 8, 2025

Description

The primary motivation of this PR is to create a generalized path toward vectorized/ SIMD instruction optimizations for finite fields in Arkworks.

In the process, we pick up a non-trivial performance boost in serial.

  • We introduce SmallFp, a procedural macro for instantiating prime fields with modulus < 2^127
  • In the place of BigInt, this macro uses native integer types (u8, u16, u32, u64, u128)
  • For a summary of results and benchmarking please see section "results" in slides
  • There a no breaking changes in this effort

SLIDES: https://andrewzitek.xyz/images/small_fp_slides.pdf

Closes: #1038

This work was done in collaboration with @z-tech

@benbencik benbencik marked this pull request as ready for review October 9, 2025 10:07
@benbencik benbencik requested review from a team as code owners October 9, 2025 10:07
@benbencik benbencik requested review from Pratyush and z-tech and removed request for a team October 9, 2025 10:07
@z-tech

This comment was marked as outdated.

@benbencik

This comment was marked as outdated.

@z-tech

This comment was marked as outdated.

@z-tech
Copy link
Contributor

z-tech commented Nov 7, 2025

@Pratyush @weikengchen @mmaker I've updated the PR and Issue with more information and the presentation slides: https://andrewzitek.xyz/images/small_fp_slides.pdf

We present this to Ale at 17h GMT+1 today. You are very welcome to attend and we are happy to schedule this 15 min presentation with you at another time.

Thanks.

@weikengchen
Copy link
Member

I feel that the current shape is okay-enough for a review... let me take a look


impl<P: SmallFpConfig> From<u16> for SmallFp<P> {
fn from(other: u16) -> Self {
let other_as_t = match P::T::try_from(other.into()) {
Copy link
Member

Choose a reason for hiding this comment

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

I am just curious why not just always do

SmallFp::new(other % P::MODULUS)

or the like (due to types), since I feel it would be faster than the try_from and many other things that involve function calls.

Copy link
Contributor

Choose a reason for hiding this comment

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

We know that other is u16 but the argument to SmallFp::new() is type T in {u8, u16, u32, u64, u128} so the cleanest I can figure out how to make it would be something like:

fn from(other: u16) -> Self {
        let modulus_as_u128: u128 = P::MODULUS.into();
        let other_as_u128: u128 = other.into();
        let reduced = other_as_u128 % modulus_as_u128;
        // let reduced_as_t = P::T::try_from(reduced).unwrap(); <-- this fails bc the error doesn't implement debug
        let reduced_as_t = P::T::try_from(reduced).unwrap_or_else(|_| panic!("Reduced value should fit in T"));
        SmallFp::new(reduced_as_t)
}

But by including the match, being that other is u16 we'd probably fall into the Ok() branch a huge amount of the time (but not all the time) and skip the u128 reduction.

Copy link
Member

Choose a reason for hiding this comment

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

I think I am in favor of the new one because it offers another advantage --- it seems to be mostly constant-time, while the previous one doesn't.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hey, I did another pass I think it's this that's needed to match the behavior of the BigInt impl: benbencik#10

@weikengchen
Copy link
Member

(I think I have 6 files left but would love to hear from you on the ones above first)

@weikengchen
Copy link
Member

We probably will find a way to bypass the nightly compilation failures in another PR... so ignore that for now
One way is to add unused_unsafe for now.

@weikengchen
Copy link
Member

oh there is already a PR in the work for that: #1059 (comment)

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.

4 participants