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

Expand generic field usage #272

Merged
merged 37 commits into from Sep 13, 2022
Merged

Expand generic field usage #272

merged 37 commits into from Sep 13, 2022

Conversation

tzerrell
Copy link
Member

@tzerrell tzerrell commented Sep 8, 2022

Move forward replacing hardcoded baby bear with generic field (that for now is generally still baby bear). At this point the non-genericized code is largely limited to the automatically generated circuit code and the adapter code that interfaces with it.

I have design questions / uncertainties on a few details, which I'll call attention to with inline comments.

@tzerrell tzerrell self-assigned this Sep 8, 2022
// no-op base case
}

// TODO: This generates butterfly functions up to $n = 32, but will panic if $n
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 am generating butterfly functions up to $n = 32, but for some fields (e.g. baby bear) these aren't all legal and some will panic. Would it be better to return Options and handle bad calls?

}

#[cfg(feature = "host")]
mod host {
use super::*;
use crate::adapter::{PolyExt, PolyExtContext};

pub struct CpuVerifyHal<'a, S: Sha, C: PolyExt> {
// TODO: Not certain how best to handle the unused Field that's needed to
Copy link
Member Author

Choose a reason for hiding this comment

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

The CpuVerifyHal needs to know which Field elements it will be working with, but it doesn't actually directly use the field -- is this the right design for providing that information?

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 you can use PhantomData for this purpose.

https://doc.rust-lang.org/std/marker/struct.PhantomData.html

Something was wrong with the perf after FRI field genericization, so reverting for now
@tzerrell
Copy link
Member Author

tzerrell commented Sep 8, 2022

The updates in FRI broke performance, so I have reverted them for now; I'm looking into reapplying them without taking a perf hit.

@tzerrell tzerrell marked this pull request as draft September 8, 2022 21:46
@tzerrell tzerrell marked this pull request as ready for review September 12, 2022 23:29
@tzerrell
Copy link
Member Author

Thank you to @shkoo for using his new perf tool to track down the performance loss in FRI!

Copy link
Member

@flaub flaub left a comment

Choose a reason for hiding this comment

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

LGTM

@tzerrell tzerrell merged commit f99fe08 into main Sep 13, 2022
@tzerrell tzerrell deleted the tzerrell-generic-fp branch September 13, 2022 18:08
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.

None yet

3 participants