specific simd batch inverse functions

[ohad-starkware]

No description provided.

[ohad-starkware]

• use parallel batch inverse in logup #987
• parallel batch inverse #984
• specific simd batch inverse functions #985 👈 (View in Graphite)
• chunked batch inverse #983
• allocating batch inverse #977
• remove not used FieldOps trait #969
• dev

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

[reviewable-StarkWare]

This change is 

[shaharsamocha7]

Reviewed all commit messages.
Reviewable status: 0 of 3 files reviewed, 3 unresolved discussions (waiting on @ohad-starkware)

---

crates/prover/src/core/backend/simd/m31.rs line 255 at r1 (raw file):

    }

    fn invert_many(column: &[Self]) -> Vec<Self> {

It makes sense to call those batch_inverse as well, right?

Suggestion:

fn batch_inverse(column: &[Self]) -> Vec<Self> {

---

crates/prover/src/core/backend/simd/m31.rs line 256 at r1 (raw file):

    fn invert_many(column: &[Self]) -> Vec<Self> {
        const OPTIMAL_PACKED_M31_BATCH_INVERSE_CHUNK_SIZE: usize = 1 << 9;

I think those constant should be on top of file and be documented

Code quote:

const OPTIMAL_PACKED_M31_BATCH_INVERSE_CHUNK_SIZE: usize = 1 << 9;

---

crates/prover/src/core/backend/simd/qm31.rs line 176 at r1 (raw file):

    fn invert_many(column: &[Self]) -> Vec<Self> {
        const OPTIMAL_PACKED_QM31_BATCH_INVERSE_CHUNK_SIZE: usize = 1 << 11;

how come that for QM31 this number is higher than for M31
Shouldn't it be the other way around?

Code quote:

 const OPTIMAL_PACKED_QM31_BATCH_INVERSE_CHUNK_SIZE: usize = 1 << 11;

[ohad-starkware]

Reviewable status: 0 of 3 files reviewed, 3 unresolved discussions (waiting on @shaharsamocha7)

---

crates/prover/src/core/backend/simd/m31.rs line 255 at r1 (raw file):

Previously, shaharsamocha7 wrote…

It makes sense to call those batch_inverse as well, right?

I have too, just didnt rebase
done.

---

crates/prover/src/core/backend/simd/m31.rs line 256 at r1 (raw file):

Previously, shaharsamocha7 wrote…

I think those constant should be on top of file and be documented

ill document
re top of the file, I disagree - this number is specifically tailored for this function call, no one else should be able to use it, not even in this file.

[shaharsamocha7]

Reviewed 3 of 3 files at r2, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @ohad-starkware)

---

crates/prover/src/core/backend/simd/m31.rs line 256 at r1 (raw file):

Previously, ohad-starkware (Ohad) wrote…

ill document
re top of the file, I disagree - this number is specifically tailored for this function call, no one else should be able to use it, not even in this file.

I see your point about scoping but my view is about code structure.
For me, it make more sense that these 3 constants would be defined in the same location in code.
That is because if one of them needs to be changed (due to different benchmarks) it will also make sense to change the others.
Ideally, I think that the magic number should be only for M31 type, and the other constants should be defined relatively to that. e.g.

// Optimal chunk sizes were determined empirically on an intel 155u machine.
const PACKED_M31_BATCH_INVERSE_CHUNK_SIZE: usize = 1 << 9;
const PACKED_CM31_BATCH_INVERSE_CHUNK_SIZE = 2 *  PACKED_M31_BATCH_INVERSE_CHUNK_SIZE;
const PACKED_QM31_BATCH_INVERSE_CHUNK_SIZE = 2 * PACKED_CM31_BATCH_INVERSE_CHUNK_SIZE;

Your call, unblocking

[ohad-starkware]

Reviewable status: 0 of 4 files reviewed, 2 unresolved discussions (waiting on @ohad-starkware and @shaharsamocha7)

---

crates/prover/src/core/backend/simd/m31.rs line 256 at r1 (raw file):

Previously, shaharsamocha7 wrote…

I see your point about scoping but my view is about code structure.
For me, it make more sense that these 3 constants would be defined in the same location in code.
That is because if one of them needs to be changed (due to different benchmarks) it will also make sense to change the others.
Ideally, I think that the magic number should be only for M31 type, and the other constants should be defined relatively to that. e.g.

// Optimal chunk sizes were determined empirically on an intel 155u machine.
const PACKED_M31_BATCH_INVERSE_CHUNK_SIZE: usize = 1 << 9;
const PACKED_CM31_BATCH_INVERSE_CHUNK_SIZE = 2 *  PACKED_M31_BATCH_INVERSE_CHUNK_SIZE;
const PACKED_QM31_BATCH_INVERSE_CHUNK_SIZE = 2 * PACKED_CM31_BATCH_INVERSE_CHUNK_SIZE;

Your call, unblocking

about the location, I took the suggestion
re relative, I'm not sure it will always apply, so I left it as raw numbers, no need to couple them IMO.

---

crates/prover/src/core/backend/simd/qm31.rs line 176 at r1 (raw file):

Previously, shaharsamocha7 wrote…

how come that for QM31 this number is higher than for M31
Shouldn't it be the other way around?

right, it does make sense, I guess the final inverse is the main culprit,
I have bench code at ohad/bench_batch_inverse (it's rough). We can revisit these numbers when we do machine-specific optimizations.

[codecov-commenter]

Codecov Report

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

Project coverage is 92.26%. Comparing base (eef4936) to head (03135cd).
Report is 4 commits behind head on dev.

Files with missing lines	Patch %	Lines
crates/prover/src/core/backend/simd/qm31.rs	0.00%	3 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##              dev     #985      +/-   ##
==========================================
- Coverage   92.27%   92.26%   -0.02%     
==========================================
  Files         105      105              
  Lines       14221    14230       +9     
  Branches    14221    14230       +9     
==========================================
+ Hits        13123    13129       +6     
- Misses       1024     1027       +3     
  Partials       74       74              

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

[shaharsamocha7]

Reviewed 4 of 4 files at r3, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @ohad-starkware)

[ohad-starkware]

Merge activity

• Jan 29, 4:50 AM EST: A user started a stack merge that includes this pull request via Graphite.
• Jan 29, 4:51 AM EST: A user merged this pull request with Graphite.