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.

Sign up for GitHub

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

Jump to bottom

Improve primitive biginteger operations #204

Closed
wants to merge 4 commits into from
Closed

Improve primitive biginteger operations #204

wants to merge 4 commits into from

Conversation

jon-chuang
Copy link
Contributor

@jon-chuang jon-chuang commented Feb 5, 2021 •
edited
Loading

Description

Details and benchmarks discussed here: #198
Related PR: #199, now #205

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 (master)
  • 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

@weikengchen
Copy link
Member

@Pratyush I think #205 and this one is solving the same issue and could be reconciled together.

@jon-chuang jon-chuang mentioned this pull request Feb 5, 2021
Merged
6 tasks
ValarDragon
ValarDragon reviewed Feb 5, 2021
View reviewed changes
ff/src/biginteger/macros.rs
@@ -119,8 +164,14 @@ macro_rules! bigint_impl {
}

#[inline]
#[ark_ff_asm::unroll_for_loops]
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 we need to benchmark this change on larger fields as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, you are probably right. This will currently involve changing a lot of Cargo.toml files to point to my local repo. I'll try to fix some of the issues I find in this process.

@jon-chuang
Copy link
Contributor Author

Things I am not sure about:

  • necessity of removing derive(Eq)?

ValarDragon
ValarDragon reviewed Feb 5, 2021
View reviewed changes
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.

All of these changes LGTM. Holding off on approving until we have confirmed performance improvements of always unrolling on varying field sizes.

@Pratyush
Copy link
Member

Pratyush commented Feb 5, 2021 •
edited
Loading

@ValarDragon this PR is subsumed by #205, minus the is_zero change (assembly is more compact for the iterator version at higher number of limbs, and at lower number of limbs it's unrolled). I adapted the rest of @jon-chuang's changes.

@ValarDragon
Copy link
Member

I thought this PR also removed more asserts? I saw two removed here, but only one in the other PR. Let me recount though.

@ValarDragon
Copy link
Member

Recounted, it is indeed not fully subsumed. The assert for mul_in_place removed here is not removed in #205 atm.

@jon-chuang
Copy link
Contributor Author

closed in favour of #205

@jon-chuang jon-chuang closed this Feb 6, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ValarDragon ValarDragon ValarDragon left review comments

@Pratyush Pratyush Awaiting requested review from Pratyush

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@jon-chuang @weikengchen @Pratyush @ValarDragon