ColumnOps

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

---

src/core/backend/mod.rs line 18 at r2 (raw file):

}

pub trait ColumnOps<T> {

We are defining columns only for felts, right?

Suggestion:

F: Field

---

src/core/backend/avx512/mod.rs line 63 at r2 (raw file):

}

impl FieldOps<BaseField> for AVX512Backend {

Consider moving this to the avx field file.

Code quote:

impl FieldOps<BaseField> for AVX512Backend {

[spapinistarkware]

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

---

src/core/backend/mod.rs line 18 at r2 (raw file):

Previously, shaharsamocha7 wrote…

We are defining columns only for felts, right?

If it's only for felts, that it would be more sensible to add this in FieldOps.
I think there's also value in other columns. For Hashes, and twiddle factors (which are not reduced, and hence, not BaseField)

---

src/core/backend/avx512/mod.rs line 63 at r2 (raw file):

Previously, shaharsamocha7 wrote…

Consider moving this to the avx field file.

Noted.
don't think there enough to put there just yet.

[codecov-commenter]

Codecov Report

Attention: Patch coverage is 57.14286% with 3 lines in your changes are missing coverage. Please review.

Project coverage is 95.56%. Comparing base (882e7f6) to head (a265fbb).

Files	Patch %	Lines
src/core/backend/mod.rs	0.00%	3 Missing ⚠️

Additional details and impacted files

@@                              Coverage Diff                               @@
##           spapini/03-07-Use_precomputed_twiddles_in_avx     #459   +/-   ##
==============================================================================
  Coverage                                          95.56%   95.56%           
==============================================================================
  Files                                                 56       57    +1     
  Lines                                               8870     8870           
  Branches                                            8870     8870           
==============================================================================
  Hits                                                8477     8477           
  Misses                                               345      345           
  Partials                                              48       48           

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

[shaharsamocha7]

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

---

src/core/backend/mod.rs line 18 at r2 (raw file):

Previously, spapinistarkware (Shahar Papini) wrote…

If it's only for felts, that it would be more sensible to add this in FieldOps.
I think there's also value in other columns. For Hashes, and twiddle factors (which are not reduced, and hence, not BaseField)

Now I see why they are not BaseField...

[shaharsamocha7]

Reviewed 4 of 4 files at r5, 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.

[shaharsamocha7]

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