Verify Bitwise Xor 9 Files

[Gali-StarkWare]

This change is 

[Gali-StarkWare]

• Integrate Bitwise Builtin #395
• Integrate Verify Bitwise Xor 9 #402
• Bitwise Builtin Files #381
• Verify Bitwise Xor 9 Files #401 👈 (View in Graphite)
• Add Bitwise Xor to Preprocessed Column Enum #380
• Bitwise Xor Struct #379
• main

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

[ohad-starkware]

Reviewed 5 of 6 files at r1, all commit messages.
Reviewable status: 5 of 6 files reviewed, 2 unresolved discussions (waiting on @Gali-StarkWare and @shaharsamocha7)

---

stwo_cairo_prover/crates/prover/src/components/verify_bitwise_xor_9/prover.rs line 93 at r1 (raw file):

fn write_trace_simd(
    log_size: u32,
    mults: AtomicMultiplicityColumn,

isnt log_size encoded in mults?
also, if you call into_simd_vec right away, it should be called from the outside, and write_trace_simd does not need to be aware of MultiPlicityColumn. this is because you might want to use a different struct for mults, and this coupling has no benefit.

Code quote:

    log_size: u32,
    mults: AtomicMultiplicityColumn,

---

stwo_cairo_prover/crates/prover/src/components/utils.rs line 54 at r1 (raw file):

}

#[derive(Default)]

why? this sound wrong to me

[shaharsamocha7]

Reviewed 3 of 5 files at r3, all commit messages.
Reviewable status: 4 of 6 files reviewed, 8 unresolved discussions (waiting on @Gali-StarkWare and @ohad-starkware)

---

stwo_cairo_prover/crates/prover/src/components/utils.rs line 54 at r1 (raw file):

Previously, ohad-starkware (Ohad) wrote…

why? this sound wrong to me

agree

---

stwo_cairo_prover/crates/prover/src/components/verify_bitwise_xor_9/component.rs line 70 at r3 (raw file):

    #[allow(unused_parens)]
    #[allow(clippy::double_parens)]
    #[allow(non_snake_case)]

Are those needed?

Code quote:

    #[allow(unused_parens)]
    #[allow(clippy::double_parens)]
    #[allow(non_snake_case)]

---

stwo_cairo_prover/crates/prover/src/components/verify_bitwise_xor_9/component.rs line 72 at r3 (raw file):

    #[allow(non_snake_case)]
    fn evaluate<E: EvalAtRow>(&self, mut eval: E) -> E {
        let BitwiseXor0 = eval

Suggestion change to good rust names

Suggestion:

let xor_a = eval

---

stwo_cairo_prover/crates/prover/src/components/verify_bitwise_xor_9/component.rs line 73 at r3 (raw file):

    fn evaluate<E: EvalAtRow>(&self, mut eval: E) -> E {
        let BitwiseXor0 = eval
            .get_preprocessed_column(PreProcessedColumn::BitwiseXor(BitwiseXor::new(9, 0)).id());

consider to move to a constant

Suggestion:

        let BitwiseXor0 = eval
            .get_preprocessed_column(PreProcessedColumn::BitwiseXor(BitwiseXor::new(N_BITS, 0)).id());

---

stwo_cairo_prover/crates/prover/src/components/verify_bitwise_xor_9/prover.rs line 42 at r3 (raw file):

pub type InputType = [M31; 3];
pub type PackedInputType = [PackedM31; 3];
const N_BITS: u32 = 9;

Move this constant to component.rs

Code quote:

const N_BITS: u32 = 9;

---

stwo_cairo_prover/crates/prover/src/components/verify_bitwise_xor_9/prover.rs line 44 at r3 (raw file):

const N_BITS: u32 = 9;
const N_TRACE_COLUMNS: usize = 1;
const LOG_SIZE: u32 = 18;

Suggestion:

const LOG_SIZE: u32 = N_BITS * 2;

---

stwo_cairo_prover/crates/prover/src/components/verify_bitwise_xor_9/prover.rs line 68 at r3 (raw file):

        let log_size = self.log_size;
        let (trace, lookup_data) = write_trace_simd(log_size, self.mults);

remove empty line

[Gali-StarkWare]

Reviewable status: 3 of 6 files reviewed, 8 unresolved discussions (waiting on @Gali-StarkWare, @ohad-starkware, and @shaharsamocha7)

---

stwo_cairo_prover/crates/prover/src/components/utils.rs line 54 at r1 (raw file):

Previously, shaharsamocha7 wrote…

agree

Because of the derive default of ClaimGenerator (it's created using just LOG_SIZE). I can put #[allow(clippy::new_without_default)] on this ClaimGenerator instead.

---

stwo_cairo_prover/crates/prover/src/components/verify_bitwise_xor_9/component.rs line 70 at r3 (raw file):

Previously, shaharsamocha7 wrote…

Are those needed?

Not after the renaming you suggested, removed them.

---

stwo_cairo_prover/crates/prover/src/components/verify_bitwise_xor_9/component.rs line 72 at r3 (raw file):

Previously, shaharsamocha7 wrote…

Suggestion change to good rust names

Done.

---

stwo_cairo_prover/crates/prover/src/components/verify_bitwise_xor_9/component.rs line 73 at r3 (raw file):

Previously, shaharsamocha7 wrote…

consider to move to a constant

Done.

---

stwo_cairo_prover/crates/prover/src/components/verify_bitwise_xor_9/prover.rs line 93 at r1 (raw file):

Previously, ohad-starkware (Ohad) wrote…

isnt log_size encoded in mults?
also, if you call into_simd_vec right away, it should be called from the outside, and write_trace_simd does not need to be aware of MultiPlicityColumn. this is because you might want to use a different struct for mults, and this coupling has no benefit.

1. Yes, but there is no len method on the multiplicity column and data is private.
2. Done.

---

stwo_cairo_prover/crates/prover/src/components/verify_bitwise_xor_9/prover.rs line 42 at r3 (raw file):

Previously, shaharsamocha7 wrote…

Move this constant to component.rs

Done.

---

stwo_cairo_prover/crates/prover/src/components/verify_bitwise_xor_9/prover.rs line 68 at r3 (raw file):

Previously, shaharsamocha7 wrote…

remove empty line

Done.

---

stwo_cairo_prover/crates/prover/src/components/verify_bitwise_xor_9/prover.rs line 44 at r3 (raw file):

const N_BITS: u32 = 9;
const N_TRACE_COLUMNS: usize = 1;
const LOG_SIZE: u32 = 18;

Done.

[shaharsamocha7]

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

---

stwo_cairo_prover/crates/prover/src/components/verify_bitwise_xor_9/prover.rs line 93 at r1 (raw file):

Previously, Gali-StarkWare wrote…

1. Yes, but there is no len method on the multiplicity column and data is private.
2. Done.

now that it is Vec you do have the log_size, consider to remove

---

stwo_cairo_prover/crates/prover/src/components/verify_bitwise_xor_9/prover.rs line 109 at r5 (raw file):

            *lookup_data.verify_bitwise_xor_9_0 = [
                BitwiseXor::new(9, 0).packed_at(row_index),

N_BITS

Code quote:

9

[Gali-StarkWare]

Reviewable status: 5 of 6 files reviewed, 3 unresolved discussions (waiting on @ohad-starkware and @shaharsamocha7)

---

stwo_cairo_prover/crates/prover/src/components/verify_bitwise_xor_9/prover.rs line 93 at r1 (raw file):

Previously, shaharsamocha7 wrote…

now that it is Vec you do have the log_size, consider to remove

Done.

---

stwo_cairo_prover/crates/prover/src/components/verify_bitwise_xor_9/prover.rs line 109 at r5 (raw file):

Previously, shaharsamocha7 wrote…

N_BITS

Oops, done.

[shaharsamocha7]

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

---

stwo_cairo_prover/crates/prover/src/components/verify_bitwise_xor_9/prover.rs line 93 at r1 (raw file):

Previously, Gali-StarkWare wrote…

Done.

IIUC, ohad suggested to remove log_size as parameter.
Does it mean you considered and decided to keep it?

[Gali-StarkWare]

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

---

stwo_cairo_prover/crates/prover/src/components/verify_bitwise_xor_9/prover.rs line 93 at r1 (raw file):

Previously, shaharsamocha7 wrote…

IIUC, ohad suggested to remove log_size as parameter.
Does it mean you considered and decided to keep it?

I removed it as an attribute from ClaimGenerator, and then calculated it in write_trace function. I changed it so it would be calculated in write_trace_simd as well, but IMO it's code duplication.

[shaharsamocha7]

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

[ohad-starkware]

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

---

stwo_cairo_prover/crates/prover/src/components/verify_bitwise_xor_9/prover.rs line 93 at r1 (raw file):

Previously, Gali-StarkWare wrote…

I removed it as an attribute from ClaimGenerator, and then calculated it in write_trace function. I changed it so it would be calculated in write_trace_simd as well, but IMO it's code duplication.

you already have LOG_SIZE as a const

---

stwo_cairo_prover/crates/prover/src/components/utils.rs line 54 at r1 (raw file):

Previously, Gali-StarkWare wrote…

Because of the derive default of ClaimGenerator (it's created using just LOG_SIZE). I can put #[allow(clippy::new_without_default)] on this ClaimGenerator instead.

yes that is better, and I think the lint is incorrect here (so the allow is fine)

---

stwo_cairo_prover/crates/prover/src/components/verify_bitwise_xor_9/prover.rs line 64 at r7 (raw file):

    {
        let mults = self.mults.into_simd_vec();
        let log_size = mults.len().ilog2();

you already have LOG_SIZE as a const

---

stwo_cairo_prover/crates/prover/src/components/verify_bitwise_xor_9/prover.rs line 89 at r7 (raw file):

fn write_trace_simd(mults: Vec<PackedM31>) -> (ComponentTrace<N_TRACE_COLUMNS>, LookupData) {
    let log_size = mults.len().ilog2();

same

---

stwo_cairo_prover/crates/prover/src/components/verify_bitwise_xor_9/prover.rs line 106 at r7 (raw file):

            *lookup_data.verify_bitwise_xor_9_0 = [
                BitwiseXor::new(N_BITS, 0).packed_at(row_index),

unrelated to this pr -
calling new in a loop is weird IMO, even though it is probably optimized out.
what happens when the struct is complex? and the new function carries some logic that cant be optimized out?

[shaharsamocha7]

Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @Gali-StarkWare)

---

stwo_cairo_prover/crates/prover/src/components/verify_bitwise_xor_9/prover.rs line 106 at r7 (raw file):

Previously, ohad-starkware (Ohad) wrote…

unrelated to this pr -
calling new in a loop is weird IMO, even though it is probably optimized out.
what happens when the struct is complex? and the new function carries some logic that cant be optimized out?

agree that BitwiseXor::new(N_BITS, i) should be called outside of the loop