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

Resolve Compare FFT implementations #62 #4

Merged
merged 4 commits into from
May 29, 2023

Conversation

einar-taiko
Copy link

@einar-taiko einar-taiko commented May 17, 2023

This resolves https://github.com/taikoxyz/zkevm-circuits/issues/62

  • Fast-forward main to main from PSE.

  • Rebase Brecht's FFT fork on main

  • Rebase Scroll's FFT fork on main

  • Factor out redundancies between forks

  • Reuse tests from branches

  • Verify cargo test

  • Please cargo check --all-targets

  • Please cargo clippy --all-targets

Copy link

@Brechtpd Brechtpd left a comment

Choose a reason for hiding this comment

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

Diff much improved!

halo2_proofs/src/arithmetic.rs Outdated Show resolved Hide resolved
halo2_proofs/src/plonk/evaluation.rs Outdated Show resolved Hide resolved
halo2_proofs/src/poly.rs Outdated Show resolved Hide resolved
halo2_proofs/src/poly/domain.rs Outdated Show resolved Hide resolved
@einar-taiko einar-taiko force-pushed the einar/pr/fft branch 6 times, most recently from a770eeb to 8506f67 Compare May 21, 2023 11:53
@einar-taiko einar-taiko marked this pull request as ready for review May 21, 2023 11:55
@einar-taiko einar-taiko requested a review from Brechtpd May 21, 2023 11:56
@einar-taiko
Copy link
Author

einar-taiko commented May 21, 2023

@Brechtpd I have reverted as much as possible to produce a minimal diff -- including the things you mentioned.

I have also chosen recursive_fft as the default now.

Please have another look at the diff and see if there are further changes you would like.

Copy link

@Brechtpd Brechtpd left a comment

Choose a reason for hiding this comment

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

Ah nice diff now! Just some comments, overall looks great now!

halo2_proofs/src/poly/domain.rs Outdated Show resolved Hide resolved
halo2_proofs/src/poly/domain.rs Outdated Show resolved Hide resolved
halo2_proofs/src/fft.rs Outdated Show resolved Hide resolved
halo2_proofs/benches/fft.rs Outdated Show resolved Hide resolved
@einar-taiko
Copy link
Author

@Brechtpd Please confirm which implementation you would like as default, keeping in mind that recursive is slightly faster for k=25 while it runs OOM for k=29. Besides that, I'd say the PR is ready.

Copy link

@Brechtpd Brechtpd left a comment

Choose a reason for hiding this comment

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

Just some nitpicks mostly about the diff, otherwise LGTM!

halo2_proofs/src/plonk/evaluation.rs Outdated Show resolved Hide resolved
halo2_proofs/src/plonk/circuit.rs Outdated Show resolved Hide resolved
halo2_proofs/src/poly/domain.rs Outdated Show resolved Hide resolved
halo2_proofs/src/poly/domain.rs Outdated Show resolved Hide resolved
halo2_proofs/src/poly/domain.rs Outdated Show resolved Hide resolved
halo2_proofs/src/poly/domain.rs Outdated Show resolved Hide resolved
halo2_proofs/src/poly/domain.rs Outdated Show resolved Hide resolved
@einar-taiko
Copy link
Author

All suggestions implemented.

@Brechtpd Please confirm which implementation you would like as default, keeping in mind that recursive is slightly faster for k=25 while it runs OOM for k=29. [...]

@Brechtpd Please let me know which you prefer as default before we merge. It currently defaults to baseline.

@Brechtpd
Copy link

Brechtpd commented May 28, 2023

@Brechtpd Please let me know which you prefer as default before we merge. It currently defaults to baseline.

Let's go with the parallel one, I think that one will is safer when running with a non power of 2 number of cores/hyper threading.

@einar-taiko
Copy link
Author

Let's go with the parallel one.

Done.

@Brechtpd
Copy link

Still two diff changes left that I unresolved, see above. After that were done!

@einar-taiko
Copy link
Author

Still two diff changes left that I unresolved, see above. After that were done!

@Brechtpd Not sure what went wrong last night. The last two should now be reverted in 5bc5ca5.

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