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

Save twiddles #380

Merged
merged 1 commit into from
Mar 4, 2024
Merged

Save twiddles #380

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 from 1754cef to dbdee89 Compare February 26, 2024 13:38
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 1 files reviewed, 1 unresolved discussion (waiting on @spapinistarkware)


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

/// # Safety
/// This function is safe.
pub unsafe fn vecwise_ibutterflies(

this is only used on the first 4 layers? you can't use it in the middle of the FFT?

Code quote:

vecwise_ibutterflies

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


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

Previously, ilyalesokhin-starkware wrote…

this is only used on the first 4 layers? you can't use it in the middle of the FFT?

In this implementation, no.
It's good for ffts up to 2^28 elements, which I think is good for now.
igher than that, we would also need to transpose inside the vec, which I currently don't do.

@spapinistarkware spapinistarkware force-pushed the spapini/02-21-ifft_lower branch from 5054180 to 83b4507 Compare March 3, 2024 07:18
@spapinistarkware spapinistarkware force-pushed the spapini/02-21-Save_twiddles branch from dbdee89 to 7df206b Compare March 3, 2024 07:18
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 1 files at r3, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @spapinistarkware)

@spapinistarkware spapinistarkware force-pushed the spapini/02-21-ifft_lower branch from 83b4507 to 63d67e8 Compare March 3, 2024 09:13
@spapinistarkware spapinistarkware force-pushed the spapini/02-21-Save_twiddles branch from 7df206b to 19779ce Compare March 3, 2024 09:13
@spapinistarkware spapinistarkware force-pushed the spapini/02-21-ifft_lower branch from 63d67e8 to 06dd0af Compare March 3, 2024 11:49
@spapinistarkware spapinistarkware force-pushed the spapini/02-21-Save_twiddles branch from 19779ce to becaa49 Compare March 3, 2024 11:49
@ilyalesokhin-starkware
Copy link
Collaborator

src/core/backend/avx512/fft.rs line 274 at r4 (raw file):

    };
    let t = _mm512_permutexvar_epi32(INDICES_FROM_T1, t);
    let t = _mm512_xor_epi32(t, NEGATION_MASK);

what is this?

Code quote:

    const INDICES_FROM_T1: __m512i = unsafe {
        core::mem::transmute([
            0b0001, 0b0001, 0b0000, 0b0000, 0b0011, 0b0011, 0b0010, 0b0010, 0b0101, 0b0101, 0b0100,
            0b0100, 0b0111, 0b0111, 0b0110, 0b0110,
        ])
    };
    const NEGATION_MASK: __m512i = unsafe {
        core::mem::transmute([0i32, -2, -2, 0, 0, -2, -2, 0, 0, -2, -2, 0, 0, -2, -2, 0])
    };
    let t = _mm512_permutexvar_epi32(INDICES_FROM_T1, t);
    let t = _mm512_xor_epi32(t, NEGATION_MASK);

@spapinistarkware spapinistarkware force-pushed the spapini/02-21-ifft_lower branch 2 times, most recently from 54fd21a to 7ea6c55 Compare March 3, 2024 12:56
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 1 files reviewed, 1 unresolved discussion (waiting on @ilyalesokhin-starkware)


src/core/backend/avx512/fft.rs line 274 at r4 (raw file):

Previously, ilyalesokhin-starkware wrote…

what is this?

// See the comments in its body for more info.

@spapinistarkware spapinistarkware force-pushed the spapini/02-21-ifft_lower branch 2 times, most recently from 887603e to 50b6cbb Compare March 4, 2024 09:24
@spapinistarkware spapinistarkware force-pushed the spapini/02-21-Save_twiddles branch from becaa49 to 5615ce4 Compare March 4, 2024 09:24
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 1 files reviewed, 2 unresolved discussions (waiting on @spapinistarkware)


src/core/backend/avx512/fft.rs line 328 at r5 (raw file):

    };
    let t = _mm512_permutexvar_epi32(INDICES_FROM_T1, t);
    let t = _mm512_xor_epi32(t, NEGATION_MASK);

can you move this to a function?

compute_layer0_twiddles or something like that?

Code quote:

    const INDICES_FROM_T1: __m512i = unsafe {
        core::mem::transmute([
            0b0001, 0b0001, 0b0000, 0b0000, 0b0011, 0b0011, 0b0010, 0b0010, 0b0101, 0b0101, 0b0100,
            0b0100, 0b0111, 0b0111, 0b0110, 0b0110,
        ])
    };
    const NEGATION_MASK: __m512i = unsafe {
        core::mem::transmute([0i32, -2, -2, 0, 0, -2, -2, 0, 0, -2, -2, 0, 0, -2, -2, 0])
    };
    let t = _mm512_permutexvar_epi32(INDICES_FROM_T1, t);
    let t = _mm512_xor_epi32(t, NEGATION_MASK);

@spapinistarkware spapinistarkware force-pushed the spapini/02-21-ifft_lower branch from 50b6cbb to fc239c1 Compare March 4, 2024 09:54
@spapinistarkware spapinistarkware changed the base branch from spapini/02-21-ifft_lower to dev March 4, 2024 09:57
@spapinistarkware spapinistarkware force-pushed the spapini/02-21-Save_twiddles branch from 5615ce4 to 436d4b5 Compare March 4, 2024 10:02
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 1 files reviewed, 2 unresolved discussions (waiting on @ilyalesokhin-starkware)


src/core/backend/avx512/fft.rs line 328 at r5 (raw file):

Previously, ilyalesokhin-starkware wrote…

can you move this to a function?

compute_layer0_twiddles or something like that?

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.

Reviewed 1 of 1 files at r6, 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/380)
<!-- Reviewable:end -->
@spapinistarkware spapinistarkware force-pushed the spapini/02-21-Save_twiddles branch from 436d4b5 to 20a6d22 Compare March 4, 2024 11:35
@graphite-app graphite-app bot merged commit 20a6d22 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