-
Notifications
You must be signed in to change notification settings - Fork 371
Ark-ff Small Field Support #1044
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
base: master
Are you sure you want to change the base?
Conversation
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
|
@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. |
|
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()) { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
|
(I think I have 6 files left but would love to hear from you on the ones above first) |
Co-authored-by: Weikeng Chen <[email protected]>
Update MODULUS_128 to MODULUS_U128
Use shave bits in sample func
|
We probably will find a way to bypass the nightly compilation failures in another PR... so ignore that for now |
|
oh there is already a PR in the work for that: #1059 (comment) |
Simplify logic of from(unsigned)
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.
SmallFp, a procedural macro for instantiating prime fields with modulus< 2^127u8,u16,u32,u64,u128)SLIDES: https://andrewzitek.xyz/images/small_fp_slides.pdf
Closes: #1038
This work was done in collaboration with @z-tech