mem squish

[ohad-starkware]

This change is 

[ohad-starkware]

• verify batched logup #249
• batched addr->id logup #248
• mem squish #241 👈 (View in Graphite)
• fmt for cairo #237
• main

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

[shaharsamocha7]

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

---

stwo_cairo_prover/crates/prover/src/components/memory/memory_address_to_id/component.rs line 19 at r1 (raw file):

// during The merkle commitment and FRI, as this component is usually the tallest in the Cairo AIR.
// TODO(Ohad): Change split to 8 after seq is implemented. NOTE: it is possible to split further
// with an expansion trick similar to the one used in XOR. Investigate if it is worth it.

I don't like those changes as we probably need to change everything here again.
I think the correct approach is to use the EXPAND_BITS as we have in XOR.

Code quote:

// TODO(Ohad): Change split to 8 after seq is implemented. NOTE: it is possible to split further
// with an expansion trick similar to the one used in XOR. Investigate if it is worth it.

---

stwo_cairo_prover/out.txt line 1 at r1 (raw file):

Cool file but we don't need it in git :)

[shaharsamocha7]

Reviewable status: 1 of 4 files reviewed, 4 unresolved discussions (waiting on @ohad-starkware)

---

stwo_cairo_prover/crates/prover/src/components/memory/memory_address_to_id/prover.rs line 83 at r1 (raw file):

        let next_multiple_of_16 = ((self.ids.len() + 15) >> 4) << 4;
        self.ids.resize(next_multiple_of_16, 0);
        self.multiplicities.resize(next_multiple_of_16, 0);

can we replace this by asserting that ids.len() >= 256?

Code quote:

        // Pad to a multiple of `N_LANES`.
        let next_multiple_of_16 = ((self.ids.len() + 15) >> 4) << 4;
        self.ids.resize(next_multiple_of_16, 0);
        self.multiplicities.resize(next_multiple_of_16, 0);

---

stwo_cairo_prover/crates/prover/src/components/memory/memory_address_to_id/component.rs line 59 at r1 (raw file):

    fn evaluate<E: EvalAtRow>(&self, mut eval: E) -> E {
        let address = eval.next_trace_mask();
        for i in 0..SPLIT_SIZE {

I think SPLIT_SIZE should be generic

Code quote:

for i in 0..SPLIT_SIZE {

[ohad-starkware]

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

---

stwo_cairo_prover/out.txt line 1 at r1 (raw file):

Previously, shaharsamocha7 wrote…

Cool file but we don't need it in git :)

:(

---

stwo_cairo_prover/crates/prover/src/components/memory/memory_address_to_id/component.rs line 19 at r1 (raw file):

Previously, shaharsamocha7 wrote…

I don't like those changes as we probably need to change everything here again.
I think the correct approach is to use the EXPAND_BITS as we have in XOR.

deleted comment

---

stwo_cairo_prover/crates/prover/src/components/memory/memory_address_to_id/component.rs line 59 at r1 (raw file):

Previously, shaharsamocha7 wrote…

I think SPLIT_SIZE should be generic

does it matter? youll use this const as the generic parameter.

---

stwo_cairo_prover/crates/prover/src/components/memory/memory_address_to_id/prover.rs line 83 at r1 (raw file):

Previously, shaharsamocha7 wrote…

can we replace this by asserting that ids.len() >= 256?

im doing this to avoid dealing with .remainder

[shaharsamocha7]

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

---

stwo_cairo_prover/crates/prover/src/components/memory/memory_address_to_id/prover.rs line 83 at r1 (raw file):

Previously, ohad-starkware (Ohad) wrote…

im doing this to avoid dealing with .remainder

Will it work if the memory size < SPLIT_SIZE * N_LANES?
Maybe we can just assert that

---

stwo_cairo_prover/crates/prover/src/components/memory/memory_address_to_id/prover.rs line 81 at r2 (raw file):

        // Pad to a multiple of `N_LANES`.
        let next_multiple_of_16 = ((self.ids.len() + 15) >> 4) << 4;

I think it will be more clear if you call div_ceil here

Code quote:

((self.ids.len() + 15) >> 4) << 4

---

stwo_cairo_prover/crates/prover/src/components/memory/memory_address_to_id/component.rs line 82 at r2 (raw file):

        let preprocessed_log_sizes = vec![self.log_size];
        let trace_log_sizes = vec![self.log_size; N_TRACE_COLUMNS];
        let interaction_log_sizes = vec![self.log_size; SECURE_EXTENSION_DEGREE * SPLIT_SIZE];

Do we want to combine the lookups here?

Code quote:

let interaction_log_sizes = vec![self.log_size; SECURE_EXTENSION_DEGREE * SPLIT_SIZE];

[shaharsamocha7]

Reviewed 1 of 4 files at r1.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @ohad-starkware)

---

stwo_cairo_prover/crates/prover/src/components/memory/memory_address_to_id/prover.rs line 97 at r2 (raw file):

        let inc =
            PackedM31::from_array(std::array::from_fn(|i| M31::from_u32_unchecked((i) as u32)));
        for i in 0..packed_size {

rename

Suggestion:

for i in 0..n_packed_rows

---

stwo_cairo_prover/crates/prover/src/components/memory/memory_address_to_id/prover.rs line 108 at r2 (raw file):

            trace[1 + chunk_idx * N_ID_AND_MULT_COLUMNS_PER_CHUNK].data[i] = id;
            trace[2 + chunk_idx * N_ID_AND_MULT_COLUMNS_PER_CHUNK].data[i] = multiplicity;
        }

Instead of this logic, couldn't you create the columns using from_iter, take funcs?

Code quote:

        for (i, (id, multiplicity)) in zip(id_it, multiplicities_it).enumerate() {
            let chunk_idx = i / packed_size;
            let i = i % packed_size;
            trace[1 + chunk_idx * N_ID_AND_MULT_COLUMNS_PER_CHUNK].data[i] = id;
            trace[2 + chunk_idx * N_ID_AND_MULT_COLUMNS_PER_CHUNK].data[i] = multiplicity;
        }

---

stwo_cairo_prover/crates/prover/src/components/memory/memory_address_to_id/prover.rs line 138 at r2 (raw file):

pub struct InteractionClaimGenerator {
    pub addresses: Vec<PackedM31>,

maybe comment that these are not all the addresses, just those up to SPLIT_SIZE

Code quote:

pub addresses: Vec<PackedM31>,

[ohad-starkware]

Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @shaharsamocha7)

---

stwo_cairo_prover/crates/prover/src/components/memory/memory_address_to_id/component.rs line 82 at r2 (raw file):

Previously, shaharsamocha7 wrote…

Do we want to combine the lookups here?

ill have a pr for that shortly

---

stwo_cairo_prover/crates/prover/src/components/memory/memory_address_to_id/prover.rs line 83 at r1 (raw file):

Previously, shaharsamocha7 wrote…

Will it work if the memory size < SPLIT_SIZE * N_LANES?
Maybe we can just assert that

I think test_memory_trace_prover tests that

---

stwo_cairo_prover/crates/prover/src/components/memory/memory_address_to_id/prover.rs line 81 at r2 (raw file):

Previously, shaharsamocha7 wrote…

I think it will be more clear if you call div_ceil here

done

---

stwo_cairo_prover/crates/prover/src/components/memory/memory_address_to_id/prover.rs line 97 at r2 (raw file):

Previously, shaharsamocha7 wrote…

rename

Done.

---

stwo_cairo_prover/crates/prover/src/components/memory/memory_address_to_id/prover.rs line 108 at r2 (raw file):

Previously, shaharsamocha7 wrote…

Instead of this logic, couldn't you create the columns using from_iter, take funcs?

no, im because im padding every column instead of padding only the last
wdyt?

---

stwo_cairo_prover/crates/prover/src/components/memory/memory_address_to_id/prover.rs line 138 at r2 (raw file):

Previously, shaharsamocha7 wrote…

maybe comment that these are not all the addresses, just those up to SPLIT_SIZE

suggestion?

[shaharsamocha7]

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

---

stwo_cairo_prover/crates/prover/src/components/memory/memory_address_to_id/prover.rs line 97 at r2 (raw file):

Previously, ohad-starkware (Ohad) wrote…

Done.

forgot to push or didn't change?
packed_size stand for 16 so it is confusing IMO

---

stwo_cairo_prover/crates/prover/src/components/memory/memory_address_to_id/prover.rs line 108 at r2 (raw file):

Previously, ohad-starkware (Ohad) wrote…

no, im because im padding every column instead of padding only the last
wdyt?

Can't we only pad the last column? I think it will be more straightforward, no?
Let me know what you think

---

stwo_cairo_prover/crates/prover/src/components/memory/memory_address_to_id/prover.rs line 138 at r2 (raw file):

Previously, ohad-starkware (Ohad) wrote…

suggestion?

Ok forget it as we are going to discard this anyway, right?

[ohad-starkware]

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

---

stwo_cairo_prover/crates/prover/src/components/memory/memory_address_to_id/prover.rs line 97 at r2 (raw file):

Previously, shaharsamocha7 wrote…

forgot to push or didn't change?
packed_size stand for 16 so it is confusing IMO

Done.

---

stwo_cairo_prover/crates/prover/src/components/memory/memory_address_to_id/prover.rs line 108 at r2 (raw file):

Previously, shaharsamocha7 wrote…

Can't we only pad the last column? I think it will be more straightforward, no?
Let me know what you think

in the take approache I will have more padding in the tail, because the 0s will have to be generated by the iterator
in the current approach, I'm taking a zeroed vector and writing where I want

---

stwo_cairo_prover/crates/prover/src/components/memory/memory_address_to_id/prover.rs line 138 at r2 (raw file):

Previously, shaharsamocha7 wrote…

Ok forget it as we are going to discard this anyway, right?

yes

[shaharsamocha7]

Reviewed 1 of 1 files at r4.
Reviewable status: all files reviewed (commit messages unreviewed), 1 unresolved discussion (waiting on @ohad-starkware)

[shaharsamocha7]

Reviewable status: all files reviewed (commit messages unreviewed), 1 unresolved discussion (waiting on @ohad-starkware)

---

stwo_cairo_prover/crates/prover/src/components/memory/memory_address_to_id/prover.rs line 108 at r2 (raw file):

Previously, ohad-starkware (Ohad) wrote…

in the take approache I will have more padding in the tail, because the 0s will have to be generated by the iterator
in the current approach, I'm taking a zeroed vector and writing where I want

I'm ok with that impl, but I think it's worth a comment about the structure of the trace
I.e. comment that each split takes exactly self.ids / SPLIT_SIZE values and the rest of the column is padded

[ohad-starkware]

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

---

stwo_cairo_prover/crates/prover/src/components/memory/memory_address_to_id/prover.rs line 108 at r2 (raw file):

Previously, shaharsamocha7 wrote…

I'm ok with that impl, but I think it's worth a comment about the structure of the trace
I.e. comment that each split takes exactly self.ids / SPLIT_SIZE values and the rest of the column is padded

done

[shaharsamocha7]

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

---

stwo_cairo_prover/crates/prover/src/components/memory/memory_address_to_id/component.rs line 28 at r5 (raw file):

/// ID1 = [id3, id4, id5, 0]
/// ID2 = [id6, id7, id8, 0]
/// ID3 = [id9, id10, 0, 0]

Suggestion:

/// 1. The ID and Multiplicity vectors are split to 'N_SPLIT_CHUNKS' chunks of size
///    `ids.len()` / `N_SPLIT_CHUNKS`.
/// 2. The chunks are padded with 0s to the next power of 2.
/// 
/// #  Example
/// ID = [id0..id10], N_SPLIT_CHUNKS = 4
///
/// ID0 = [id0, id1, id2, 0]
/// ID1 = [id3, id4, id5, 0]
/// ID2 = [id6, id7, id8, 0]
/// ID3 = [id9, id10, 0, 0]

[shaharsamocha7]

Reviewed 1 of 2 files at r5, 1 of 1 files at r6.
Reviewable status: all files reviewed (commit messages unreviewed), all discussions resolved (waiting on @ohad-starkware)

[ohad-starkware]

Merge activity

• Dec 17, 4:12 AM EST: A user started a stack merge that includes this pull request via Graphite.
• Dec 17, 4:12 AM EST: A user merged this pull request with Graphite.