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

AVX regular fft #390

Merged
merged 1 commit into from
Mar 6, 2024
Merged

AVX regular fft #390

merged 1 commit into from
Mar 6, 2024

Conversation

spapinistarkware
Copy link
Contributor

@spapinistarkware spapinistarkware commented Feb 22, 2024

This change is Reviewable

@spapinistarkware spapinistarkware force-pushed the spapini/02-22-AVX_regular_fft branch from 9f46325 to 0cebf44 Compare March 4, 2024 09:24
@spapinistarkware spapinistarkware force-pushed the spapini/02-22-Separate_ifft branch from 9bf0b87 to 9099710 Compare March 4, 2024 11:04
@spapinistarkware spapinistarkware force-pushed the spapini/02-22-AVX_regular_fft branch from 0cebf44 to 1aec621 Compare March 4, 2024 11:04
@spapinistarkware spapinistarkware force-pushed the spapini/02-22-Separate_ifft branch 4 times, most recently from 9ecf5ef to 5694c61 Compare March 6, 2024 13:48
@spapinistarkware spapinistarkware force-pushed the spapini/02-22-AVX_regular_fft branch 2 times, most recently from de3f95c to 436ea83 Compare March 6, 2024 13:50
@ilyalesokhin-starkware
Copy link
Collaborator

src/core/backend/avx512/fft/ifft.rs line 83 at r1 (raw file):

        ifft_vecwise_loop(values, twiddle_dbl, fft_layers - VECWISE_FFT_BITS, index_h);
        for layer in (VECWISE_FFT_BITS..fft_layers).step_by(3) {
            match fft_layers - layer {

are you trying to sneak this back in?

Code quote:

        for layer in (VECWISE_FFT_BITS..fft_layers).step_by(3) {
            match fft_layers - layer {

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 all commit messages.
Reviewable status: 0 of 2 files reviewed, 1 unresolved discussion (waiting on @spapinistarkware)


src/core/backend/avx512/fft/ifft.rs line 83 at r1 (raw file):

Previously, ilyalesokhin-starkware wrote…

are you trying to sneak this back in?

do you think doing a match on every layer is better?

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 2 files reviewed, 1 unresolved discussion (waiting on @ilyalesokhin-starkware)


src/core/backend/avx512/fft/ifft.rs line 83 at r1 (raw file):

Previously, ilyalesokhin-starkware wrote…

do you think doing a match on every layer is better?

Since I added the fft, I want it to be visible that fft is exactly the same thing in reverse order.
The while didn't really show this, since I needed to start the while from something weird like (fft_layers-4)/3*3+fft_layers

@spapinistarkware spapinistarkware force-pushed the spapini/02-22-Separate_ifft branch from 5694c61 to 52840de Compare March 6, 2024 14:26
@spapinistarkware spapinistarkware changed the base branch from spapini/02-22-Separate_ifft to dev March 6, 2024 14:30
@ilyalesokhin-starkware
Copy link
Collaborator

src/core/backend/avx512/fft/ifft.rs line 83 at r1 (raw file):

Previously, spapinistarkware (Shahar Papini) wrote…

Since I added the fft, I want it to be visible that fft is exactly the same thing in reverse order.
The while didn't really show this, since I needed to start the while from something weird like (fft_layers-4)/3*3+fft_layers

where do you need to start from something weird?

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 2 files reviewed, 1 unresolved discussion (waiting on @ilyalesokhin-starkware)


src/core/backend/avx512/fft/ifft.rs line 83 at r1 (raw file):

Previously, ilyalesokhin-starkware wrote…

where do you need to start from something weird?

In the regular fft (which is starting from higher bits).
And I want it to look exactly like the reverse of this (easier to understand why they are inverse of each other)

@ilyalesokhin-starkware
Copy link
Collaborator

src/core/backend/avx512/fft/ifft.rs line 696 at r1 (raw file):

    #[test]
    fn test_ifft_lower_with_vecwise() {
        for log_size in 5..=11 {

what is this syntax?

Code quote:

5..=11 

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 2 of 2 files at r1.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @spapinistarkware)

@spapinistarkware spapinistarkware force-pushed the spapini/02-22-AVX_regular_fft branch 2 times, most recently from 2c277df to 52f967c Compare March 6, 2024 14:50
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 2 files reviewed, 2 unresolved discussions (waiting on @ilyalesokhin-starkware)


src/core/backend/avx512/fft/ifft.rs line 696 at r1 (raw file):

Previously, ilyalesokhin-starkware wrote…

what is this syntax?

Done.

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 2 of 2 files at r2, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @spapinistarkware)

Copy link

graphite-app bot commented Mar 6, 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/390)
<!-- Reviewable:end -->
@spapinistarkware spapinistarkware force-pushed the spapini/02-22-AVX_regular_fft branch from 52f967c to 58647c1 Compare March 6, 2024 14:57
@graphite-app graphite-app bot merged commit 58647c1 into dev Mar 6, 2024
6 of 7 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