Use precomputed twiddles in avx

[spapinistarkware]

This change is 

[graphite-app]

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.

[spapinistarkware]

• ColumnOps #459
• Use precomputed twiddles in avx #443 👈
• Precompute twiddles #401
• Remove FRI PolyOps dependency #399
• avx slow eval #398
• avx poly extend #397
• AVX fft eval on larger domains #396
• Reverse order of evaluations #395
• Incorporate fft in AVX backend #394
• AVX regular fft #390
• Separate ifft #389
• Support ifft for itnermediate sizes #383
• AVX fft benchmark #382
• ifft_full #381
• Save twiddles #380
• ifft_lower #379
• avx ifft3 #378
• avx vecwise butterflies #377
• avx vecwise ibutterfly #368
• avx ibutterfly #367
• avx butterfly #366
• avx poly #365
• dev

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

Join @spapinistarkware and the rest of your teammates on Graphite

[shaharsamocha7]

Reviewed 1 of 2 files at r2, 6 of 6 files at r4, all commit messages.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @spapinistarkware)

---

src/core/backend/avx512/circle.rs line 22 at r4 (raw file):

// Decide if and when it's ok and what to do if it's not.
impl PolyOps for AVX512Backend {
    type Twiddles = Vec<i32>;

I think that something should be written here w.r.t why the twiddle in the avx is a vec of i32.

Code quote:

type Twiddles = Vec<i32>

---

src/core/backend/avx512/circle.rs line 118 at r4 (raw file):

                    &twiddles[layer_i]
                        [i << (fft_log_size - 2 - layer_i)..(i + 1) << (fft_log_size - 2 - layer_i)]
                })

Why do we want this change?

Code quote:

            let subdomain_twiddles = (0..(fft_log_size - 1))
                .map(|layer_i| {
                    &twiddles[layer_i]
                        [i << (fft_log_size - 2 - layer_i)..(i + 1) << (fft_log_size - 2 - layer_i)]
                })

---

src/core/backend/avx512/circle.rs line 147 at r4 (raw file):

    }

    fn precompute_twiddles(coset: Coset) -> TwiddleTree<Self> {

Do we maybe want the twiddles to be stored also in avx registers?

Code quote:

fn precompute_twiddles(coset: Coset) -> TwiddleTree<Self> {

---

src/core/backend/avx512/circle.rs line 152 at r4 (raw file):

        // TODO(spapini): Optimize.
        for layer in &rfft::get_twiddle_dbls(coset)[1..] {

Is this the only use of get_twiddle_dbls?
Why are we storing the layer0 twiddles there if we are going to ignore them anyway?

Code quote:

&rfft::get_twiddle_dbls(coset)[1..] 

---

src/core/backend/avx512/circle.rs line 155 at r4 (raw file):

            twiddles.extend(layer);
        }
        twiddles.push(2);

Explain

Code quote:

twiddles.push(2)

[spapinistarkware]

Reviewable status: 2 of 7 files reviewed, 5 unresolved discussions (waiting on @shaharsamocha7)

---

src/core/backend/avx512/circle.rs line 22 at r4 (raw file):

Previously, shaharsamocha7 wrote…

I think that something should be written here w.r.t why the twiddle in the avx is a vec of i32.

Done.

---

src/core/backend/avx512/circle.rs line 118 at r4 (raw file):

Previously, shaharsamocha7 wrote…

Why do we want this change?

It seems that without it, I would need to access twiddles[layer_i-1], and doing the -1 is weird.

---

src/core/backend/avx512/circle.rs line 147 at r4 (raw file):

Previously, shaharsamocha7 wrote…

Do we maybe want the twiddles to be stored also in avx registers?

Not all twiddles can, and its not that important.
Except for the first layer, only the first is loaded as an AVX register. The other are broadcasted.

---

src/core/backend/avx512/circle.rs line 152 at r4 (raw file):

Previously, shaharsamocha7 wrote…

Is this the only use of get_twiddle_dbls?
Why are we storing the layer0 twiddles there if we are going to ignore them anyway?

Done.

---

src/core/backend/avx512/circle.rs line 155 at r4 (raw file):

Previously, shaharsamocha7 wrote…

Explain

Done.

[shaharsamocha7]

Reviewed 5 of 5 files at r5, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @spapinistarkware)

---

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

}

pub fn get_itwiddle_dbls(mut coset: Coset) -> Vec<Vec<i32>> {

Document that this returns all the twiddles that relevant only for the line domains.

Code quote:

pub fn get_itwiddle_dbls(mut coset: Coset) -> Vec<Vec<i32>> {

[spapinistarkware]

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @shaharsamocha7)

---

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

Previously, shaharsamocha7 wrote…

Document that this returns all the twiddles that relevant only for the line domains.

Done.

[shaharsamocha7]

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

[graphite-app]

Merge activity

• Mar 14, 11:16 AM EDT: spapinistarkware added this pull request to the Graphite merge queue.
• Mar 14, 11:22 AM EDT: spapinistarkware merged this pull request with the Graphite merge queue.