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

Use PackedBaseField in avx fft #455

Merged
merged 1 commit into from
Mar 12, 2024

Conversation

spapinistarkware
Copy link
Contributor

@spapinistarkware spapinistarkware commented Mar 11, 2024

This change is Reviewable

Copy link

graphite-app bot commented Mar 11, 2024

Your org has enabled the Graphite merge queue for merging into dev

Add the label “merge-queue” to the PR and Graphite will automatically add it to the merge queue when it’s ready to merge.

You must have a Graphite account in order to use the merge queue. Sign up using this link.

Copy link
Contributor Author

spapinistarkware commented Mar 11, 2024

This stack of pull requests is managed by Graphite. Learn more about stacking.

Join @spapinistarkware and the rest of your teammates on Graphite Graphite

@spapinistarkware spapinistarkware force-pushed the spapini/03-11-Use_PackedBaseField_in_avx_fft branch from a08a2e9 to 1768412 Compare March 11, 2024 06:26
@ilyalesokhin-starkware
Copy link
Collaborator

src/core/backend/avx512/m31.rs line 41 at r2 (raw file):

    }

    pub fn permute_with(self, idx: __m512i, other: Self) -> Self {

document.

condiser
adding
interleave_lower_halfs
and
interleave_upper_halfs

functions.

Code quote:

permute_with

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

@spapinistarkware spapinistarkware force-pushed the spapini/03-11-Use_PackedBaseField_in_avx_fft branch from 1768412 to 2af81cb Compare March 12, 2024 05:46
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 4 files reviewed, 1 unresolved discussion (waiting on @ilyalesokhin-starkware)


src/core/backend/avx512/m31.rs line 41 at r2 (raw file):

Previously, ilyalesokhin-starkware wrote…

document.

condiser
adding
interleave_lower_halfs
and
interleave_upper_halfs

functions.

Done.

@ilyalesokhin-starkware
Copy link
Collaborator

src/core/backend/avx512/m31.rs line 41 at r2 (raw file):

Previously, spapinistarkware (Shahar Papini) wrote…

Done.

you didn't document permute_with, tough I guess it is no longer used.

@spapinistarkware spapinistarkware force-pushed the spapini/03-11-Use_PackedBaseField_in_avx_fft branch from 2af81cb to 078632b Compare March 12, 2024 09:12
@spapinistarkware spapinistarkware changed the base branch from spapini/03-07-Use_precomputed_twiddles_in_avx to dev March 12, 2024 09:12
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 4 files reviewed, all discussions resolved (waiting on @ilyalesokhin-starkware)


src/core/backend/avx512/m31.rs line 41 at r2 (raw file):

Previously, ilyalesokhin-starkware wrote…

you didn't document permute_with, tough I guess it is no longer used.

Sorry, meant to delete it.

@codecov-commenter
Copy link

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 96.04%. Comparing base (7ce4ce7) to head (078632b).

Additional details and impacted files
@@            Coverage Diff             @@
##              dev     #455      +/-   ##
==========================================
- Coverage   96.05%   96.04%   -0.02%     
==========================================
  Files          55       55              
  Lines        8536     8512      -24     
  Branches     8536     8512      -24     
==========================================
- Hits         8199     8175      -24     
  Misses        289      289              
  Partials       48       48              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

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 4 of 4 files at r3, 4 of 4 files at r4.
Reviewable status: all files reviewed (commit messages unreviewed), all discussions resolved (waiting on @spapinistarkware)

Copy link

graphite-app bot commented Mar 12, 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/455)
<!-- Reviewable:end -->
@spapinistarkware spapinistarkware force-pushed the spapini/03-11-Use_PackedBaseField_in_avx_fft branch from 078632b to 0d4ebb5 Compare March 12, 2024 11:54
@ilyalesokhin-starkware
Copy link
Collaborator

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

    //  0 1 0 1 0 1 0 1 0 1 0 1 0 1 0 1
    let t = _mm512_set1_epi64(std::mem::transmute(twiddle3_dbl));
    // Apply the permutation, resulting in indexing a:bcid.

where is the permutation?

Code quote:

  // Apply the permutation, resulting in indexing a:bcid.

@graphite-app graphite-app bot merged commit 0d4ebb5 into dev Mar 12, 2024
8 of 9 checks passed
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: all files reviewed (commit messages unreviewed), 1 unresolved discussion


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

Previously, ilyalesokhin-starkware wrote…

where is the permutation?

Look above, it's

// At each layer we apply the following permutation to the index:
//   i:abcd => d:iabc

@ilyalesokhin-starkware
Copy link
Collaborator

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

    // We need to permute the 512-bit registers to get the right order for the butterflies.
    // Denote the index of the 16 M31 elements in register i as i:abcd.
    // At each layer we apply the following permutation to the index:

Suggestion:

 // At each layer, we interleave the registers which applies the following permutation to the index:

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.

3 participants