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

ifft_full #381

Merged
merged 1 commit into from
Mar 4, 2024
Merged

ifft_full #381

merged 1 commit into from
Mar 4, 2024

Conversation

spapinistarkware
Copy link
Contributor

@spapinistarkware spapinistarkware commented Feb 22, 2024

This change is Reviewable

@spapinistarkware spapinistarkware force-pushed the spapini/02-21-Save_twiddles branch 2 times, most recently from 7df206b to 19779ce Compare March 3, 2024 09:13
@spapinistarkware spapinistarkware force-pushed the spapini/02-21-ifft_full branch from 1c21bc6 to ded50d7 Compare March 3, 2024 09:13
@spapinistarkware spapinistarkware force-pushed the spapini/02-21-Save_twiddles branch from 19779ce to becaa49 Compare March 3, 2024 11:49
@spapinistarkware spapinistarkware force-pushed the spapini/02-21-ifft_full branch from ded50d7 to 919bfcb Compare March 3, 2024 11:49
@spapinistarkware spapinistarkware force-pushed the spapini/02-21-Save_twiddles branch from becaa49 to 5615ce4 Compare March 4, 2024 09:24
@spapinistarkware spapinistarkware force-pushed the spapini/02-21-ifft_full branch from 919bfcb to 63849ac Compare March 4, 2024 09:24
@spapinistarkware spapinistarkware force-pushed the spapini/02-21-Save_twiddles branch from 5615ce4 to 436d4b5 Compare March 4, 2024 10:02
@spapinistarkware spapinistarkware force-pushed the spapini/02-21-ifft_full branch 2 times, most recently from 60c1624 to 6ad5cc4 Compare March 4, 2024 11:03
@spapinistarkware spapinistarkware force-pushed the spapini/02-21-ifft_full branch 3 times, most recently from 5f4316d to 12cfd0d Compare March 4, 2024 11:10
@ilyalesokhin-starkware
Copy link
Collaborator

src/core/backend/avx512/fft.rs line 104 at r2 (raw file):

/// Transposes the AVX vectors in the given array.
/// Swaps the bit index abc <-> cba, where |a|=|c| and |b| = 0 or 1, according to the parity of

is it obvious that this is ok?

Code quote:

Swaps the bit index abc <-> cba, where |a|=|c| and |b| = 0 or 1

Copy link
Collaborator

@ilyalesokhin-starkware ilyalesokhin-starkware left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 3 files reviewed, 2 unresolved discussions (waiting on @spapinistarkware)


src/core/backend/avx512/fft.rs line 197 at r2 (raw file):

    for index_h in 0..(1 << (log_size - fft_layers - VECS_LOG_SIZE)) {
        for layer in (0..fft_layers).step_by(3) {
            match fft_layers - layer {

Suggestion:

 while fft_layers - layer >= 3 {
     ...

Copy link
Collaborator

@ilyalesokhin-starkware ilyalesokhin-starkware left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 2 files at r2.
Reviewable status: 0 of 3 files reviewed, 2 unresolved discussions (waiting on @spapinistarkware)

Copy link
Contributor Author

@spapinistarkware spapinistarkware left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 3 files reviewed, 2 unresolved discussions (waiting on @ilyalesokhin-starkware)


src/core/backend/avx512/fft.rs line 197 at r2 (raw file):

    for index_h in 0..(1 << (log_size - fft_layers - VECS_LOG_SIZE)) {
        for layer in (0..fft_layers).step_by(3) {
            match fft_layers - layer {

Look at the last version

Copy link
Contributor Author

@spapinistarkware spapinistarkware left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 3 files reviewed, 2 unresolved discussions (waiting on @ilyalesokhin-starkware)


src/core/backend/avx512/fft.rs line 104 at r2 (raw file):

Previously, ilyalesokhin-starkware wrote…

is it obvious that this is ok?

No, but it is ok.

@spapinistarkware spapinistarkware force-pushed the spapini/02-21-Save_twiddles branch from 436d4b5 to 20a6d22 Compare March 4, 2024 11:35
@spapinistarkware spapinistarkware changed the base branch from spapini/02-21-Save_twiddles to dev March 4, 2024 11:38
@spapinistarkware spapinistarkware force-pushed the spapini/02-21-ifft_full branch from 12cfd0d to aec5081 Compare March 4, 2024 11:44
Copy link
Collaborator

@ilyalesokhin-starkware ilyalesokhin-starkware left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewed 1 of 2 files at r1, 2 of 2 files at r3, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @spapinistarkware)

Copy link

graphite-app bot commented Mar 4, 2024

Merge activity

<!-- Reviewable:start -->
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/starkware-libs/stwo/381)
<!-- Reviewable:end -->
@spapinistarkware spapinistarkware force-pushed the spapini/02-21-ifft_full branch from aec5081 to 3865705 Compare March 4, 2024 11:56
@graphite-app graphite-app bot merged commit 3865705 into dev Mar 4, 2024
5 of 6 checks passed
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