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

Add Fp64 #141

Merged
merged 15 commits into from
Dec 16, 2020
Merged

Add Fp64 #141

merged 15 commits into from
Dec 16, 2020

Conversation

Pratyush
Copy link
Member

@Pratyush Pratyush commented Dec 16, 2020

Description

Adds Fp64.


Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.

  • Targeted PR against correct branch (main)
  • Linked to Github issue with discussion and accepted design OR have an explanation in the PR that describes this work.
  • Wrote unit tests
  • Updated relevant documentation in the code
  • Added a relevant changelog entry to the Pending section in CHANGELOG.md
  • Re-reviewed Files changed in the Github PR explorer

ff/src/fields/arithmetic.rs Outdated Show resolved Hide resolved
Comment on lines +124 to +127
let subtractor = (2 * ($limbs - 1usize))
.checked_sub(i + 1)
.map(|index| r[index])
.unwrap_or(0);
Copy link
Member Author

Choose a reason for hiding this comment

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

Required to avoid underflowing. I have confirmed locally that it doesn't affect performance.

Copy link
Member

Choose a reason for hiding this comment

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

Can a comment be added for the arithmetic operations these lines are doing / intending to do? (Looks like propagating a carry bit?)

I agree this matches what was previously implemented.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not too sure myself, but I added a comment explaining what is computed.

Copy link
Member

@ValarDragon ValarDragon Dec 16, 2020

Choose a reason for hiding this comment

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

Ok cool!

Kinda worrisome if neither of us understand the code, we should file an issue to decipher & document whats going on in this function 😅

Copy link
Member Author

Choose a reason for hiding this comment

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

Agreed, @jon-chuang should have the best state, I believe?

ff/src/fields/arithmetic.rs Outdated Show resolved Hide resolved
@Pratyush Pratyush requested a review from ValarDragon December 16, 2020 03:18
Copy link
Member

@ValarDragon ValarDragon left a comment

Choose a reason for hiding this comment

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

LGTM, left a couple of requests for additional comments, then I think its good to merge

ff/src/fields/arithmetic.rs Show resolved Hide resolved
@@ -22,7 +23,7 @@ macro_rules! impl_field_mul_assign {
#[cfg(use_asm)]
#[allow(unsafe_code, unused_mut)]
{
if $limbs <= 6 {
if $limbs <= 6 && $limbs > 1 {
Copy link
Member

Choose a reason for hiding this comment

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

Not related to this PR, but could there be a comment for why theres a different method for limbs > 6? Is it just that the larger limb size case isn't implemented? (Doesn't need to block this PR)

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, larger limb size is not implemented

Comment on lines +124 to +127
let subtractor = (2 * ($limbs - 1usize))
.checked_sub(i + 1)
.map(|index| r[index])
.unwrap_or(0);
Copy link
Member

Choose a reason for hiding this comment

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

Can a comment be added for the arithmetic operations these lines are doing / intending to do? (Looks like propagating a carry bit?)

I agree this matches what was previously implemented.

ff/src/fields/arithmetic.rs Show resolved Hide resolved
Copy link
Member

@ValarDragon ValarDragon left a comment

Choose a reason for hiding this comment

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

LGTM

@Pratyush
Copy link
Member Author

This passes tests locally for the prime 257.

@Pratyush Pratyush merged commit 23c8178 into master Dec 16, 2020
@Pratyush Pratyush deleted the fp64 branch December 16, 2020 05:32
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.

2 participants