Bitwise Xor Struct

[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
• Add Bitwise Xor to Preprocessed Column Enum #380
• Bitwise Xor Struct #379 👈 (View in Graphite)
• main

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

[shaharsamocha7]

Reviewed all commit messages.
Reviewable status: 0 of 1 files reviewed, 4 unresolved discussions (waiting on @alon-f, @andrewmilson, and @Gali-StarkWare)

---

stwo_cairo_prover/crates/prover/src/cairo_air/preprocessed.rs line 108 at r1 (raw file):

/// Columns for the bitwise xor preprocessed table.
/// The table has three columns (use col_index to select the column when needed):
/// 0: first limb, 1: second limb, 2: result of the limbs' bitwise xor.

Those are implementation details.
In the outer doc "///" you should document the struct itself rather then the impl
e.g. what are n_bits/n_expand_bits

Code quote:

/// The table has three columns (use col_index to select the column when needed):
/// 0: first limb, 1: second limb, 2: result of the limbs' bitwise xor.

---

stwo_cairo_prover/crates/prover/src/cairo_air/preprocessed.rs line 115 at r1 (raw file):

    pub col_index: usize,
}
impl BitwiseXor {

Can you add explicit function for packed_at_lhs, packed_at_rhs?
I think that using those two it will be easier to implement both gen_column_simd, packed_at

Code quote:

impl BitwiseXor {

---

stwo_cairo_prover/crates/prover/src/cairo_air/preprocessed.rs line 130 at r1 (raw file):

                "preprocessed_bitwise_xor_{}_{}_{}",
                self.n_bits, self.n_expand_bits, self.col_index
            ),

I don't think n_expand_bits should be part of the identity

Code quote:

            id: format!(
                "preprocessed_bitwise_xor_{}_{}_{}",
                self.n_bits, self.n_expand_bits, self.col_index
            ),

---

stwo_cairo_prover/crates/prover/src/cairo_air/preprocessed.rs line 139 at r1 (raw file):

    pub const fn column_bits(&self) -> u32 {
        2 * self.limb_bits()
    }

Document (or maybe also rename)

Code quote:

    pub const fn limb_bits(&self) -> u32 {
        self.n_bits - self.n_expand_bits
    }

    pub const fn column_bits(&self) -> u32 {
        2 * self.limb_bits()
    }

[shaharsamocha7]

Reviewable status: 0 of 1 files reviewed, 5 unresolved discussions (waiting on @alon-f, @andrewmilson, and @Gali-StarkWare)

---

stwo_cairo_prover/crates/prover/src/cairo_air/preprocessed.rs line 112 at r1 (raw file):

pub struct BitwiseXor {
    pub n_bits: u32,
    pub n_expand_bits: u32,

I don't think that expand_bits should be here

Code quote:

    pub n_expand_bits: u32,

[Gali-StarkWare]

Reviewable status: 0 of 1 files reviewed, 5 unresolved discussions (waiting on @alon-f, @andrewmilson, and @shaharsamocha7)

---

stwo_cairo_prover/crates/prover/src/cairo_air/preprocessed.rs line 108 at r1 (raw file):

Previously, shaharsamocha7 wrote…

Those are implementation details.
In the outer doc "///" you should document the struct itself rather then the impl
e.g. what are n_bits/n_expand_bits

Better?

---

stwo_cairo_prover/crates/prover/src/cairo_air/preprocessed.rs line 112 at r1 (raw file):

Previously, shaharsamocha7 wrote…

I don't think that expand_bits should be here

Done.

---

stwo_cairo_prover/crates/prover/src/cairo_air/preprocessed.rs line 115 at r1 (raw file):

Previously, shaharsamocha7 wrote…

Can you add explicit function for packed_at_lhs, packed_at_rhs?
I think that using those two it will be easier to implement both gen_column_simd, packed_at

Done. Do you want me to use these functions to implement gen_column_simd? Or should we go for the naive implementation in the enum itself and then I can delete it from the struct?

---

stwo_cairo_prover/crates/prover/src/cairo_air/preprocessed.rs line 130 at r1 (raw file):

Previously, shaharsamocha7 wrote…

I don't think n_expand_bits should be part of the identity

I don't necessarily agree but I've already removed it :)

---

stwo_cairo_prover/crates/prover/src/cairo_air/preprocessed.rs line 139 at r1 (raw file):

Previously, shaharsamocha7 wrote…

Document (or maybe also rename)

Done. IMO log_size is self-explanatory, WDYT?

[shaharsamocha7]

Reviewed all commit messages.
Reviewable status: 0 of 1 files reviewed, 2 unresolved discussions (waiting on @alon-f, @andrewmilson, and @Gali-StarkWare)

---

stwo_cairo_prover/crates/prover/src/cairo_air/preprocessed.rs line 108 at r1 (raw file):

Previously, Gali-StarkWare wrote…

Better?

You can remove the last sentence same as the in stwo 1000 pr.

---

stwo_cairo_prover/crates/prover/src/cairo_air/preprocessed.rs line 149 at r2 (raw file):

                })
                .collect(),
            _ => unreachable!(),

This can also use your lhs, rhs functions

Code quote:

        let col: BaseColumn = match self.col_index {
            0 => (0..(1 << self.log_size()))
                .map(|i| BaseField::from_u32_unchecked((i >> self.n_bits) as u32))
                .collect(),
            1 => (0..(1 << self.log_size()))
                .map(|i| BaseField::from_u32_unchecked((i & ((1 << self.n_bits) - 1)) as u32))
                .collect(),
            2 => (0..(1 << self.log_size()))
                .map(|i| {
                    BaseField::from_u32_unchecked(
                        ((i >> self.n_bits) ^ (i & ((1 << self.n_bits) - 1))) as u32,
                    )
                })
                .collect(),
            _ => unreachable!(),

[Gali-StarkWare]

Reviewable status: 0 of 1 files reviewed, 2 unresolved discussions (waiting on @alon-f, @andrewmilson, and @shaharsamocha7)

---

stwo_cairo_prover/crates/prover/src/cairo_air/preprocessed.rs line 108 at r1 (raw file):

Previously, shaharsamocha7 wrote…

You can remove the last sentence same as the in stwo 1000 pr.

Done.

---

stwo_cairo_prover/crates/prover/src/cairo_air/preprocessed.rs line 149 at r2 (raw file):

Previously, shaharsamocha7 wrote…

This can also use your lhs, rhs functions

Yes, that's why I wrote: Do you want me to use these functions to implement gen_column_simd? Or should we go for the naive implementation in the enum itself and then I can delete it from the struct?

[shaharsamocha7]

Reviewed all commit messages.
Reviewable status: 0 of 1 files reviewed, 1 unresolved discussion (waiting on @alon-f, @andrewmilson, and @Gali-StarkWare)

---

stwo_cairo_prover/crates/prover/src/cairo_air/preprocessed.rs line 149 at r2 (raw file):

Previously, Gali-StarkWare wrote…

Yes, that's why I wrote: Do you want me to use these functions to implement gen_column_simd? Or should we go for the naive implementation in the enum itself and then I can delete it from the struct?

Two options, either use the packed at instead that code or to delete it for now
and consider how to add it in a later pr

[Gali-StarkWare]

Reviewable status: 0 of 1 files reviewed, 1 unresolved discussion (waiting on @alon-f, @andrewmilson, and @shaharsamocha7)

---

stwo_cairo_prover/crates/prover/src/cairo_air/preprocessed.rs line 149 at r2 (raw file):

Previously, shaharsamocha7 wrote…

Two options, either use the packed at instead that code or to delete it for now
and consider how to add it in a later pr

Deleted and implemented it in the enum in the next PR

[shaharsamocha7]

Reviewable status: 0 of 1 files reviewed, 1 unresolved discussion (waiting on @alon-f, @andrewmilson, and @Gali-StarkWare)

---

stwo_cairo_prover/crates/prover/src/cairo_air/preprocessed.rs line 216 at r4 (raw file):

        assert_eq!(packed_bitwise_xor, expected_bitwise_xor);
    }

I suggest to check all the xor columns on a specific index, WDYT?

Suggestion:

    #[test]
    fn test_packed_at_bitwise_xor() {
        let bitwise_a = BitwiseXor::new(LOG_SIZE, 0);
        let bitwise_b = BitwiseXor::new(LOG_SIZE, 1);
        let bitwise_xor = BitwiseXor::new(LOG_SIZE, 2);
        let index: usize = 41;
        let a: u32 = index as u32 >> LOG_SIZE;
        let b: u32 = index as u32 & ((1 << LOG_SIZE) - 1);
        let expected_xor = a ^ b;

        let res_a = bitwise_a.packed_at(index / N_LANES).to_array()[index % N_LANES];
        let res_b = bitwise_b.packed_at(index / N_LANES).to_array()[index % N_LANES];
        let res_xor = bitwise_xor.packed_at(index / N_LANES).to_array()[index % N_LANES];

        assert_eq!(res_a.0, a);
        assert_eq!(res_b.0, b);
        assert_eq!(res_xor.0, expected_xor);
    }

[ohad-starkware]

Reviewed all commit messages.
Reviewable status: 0 of 1 files reviewed, 6 unresolved discussions (waiting on @alon-f, @andrewmilson, and @Gali-StarkWare)

---

stwo_cairo_prover/crates/prover/src/cairo_air/preprocessed.rs line 105 at r4 (raw file):

}

/// A preprocessed table for the xor operation of 2 n_bits numbers.

A table of a,b,c columns, where

Suggestion:

/// A table of a,b,c, where a,b,c are `n_bits` integers and a ^ b = c.

---

stwo_cairo_prover/crates/prover/src/cairo_air/preprocessed.rs line 106 at r4 (raw file):

/// A preprocessed table for the xor operation of 2 n_bits numbers.
/// The index_in_table is the column index in the preprocessed table.

use # Attributes for that, also attribute doesn't match doc

---

stwo_cairo_prover/crates/prover/src/cairo_air/preprocessed.rs line 108 at r4 (raw file):

/// The index_in_table is the column index in the preprocessed table.
#[derive(Debug)]
pub struct BitwiseXor {

?

Suggestion:

pub struct BitwiseXorColumn {

---

stwo_cairo_prover/crates/prover/src/cairo_air/preprocessed.rs line 110 at r4 (raw file):

pub struct BitwiseXor {
    pub n_bits: u32,
    pub col_index: usize,

use an enum for column identification in the table, makes the assert below redundent

---

stwo_cairo_prover/crates/prover/src/cairo_air/preprocessed.rs line 113 at r4 (raw file):

}
impl BitwiseXor {
    pub const fn new(n_bits: u32, col_index: usize) -> Self {

you want people to go through this constructor for the assert, so unpub the attributes.

---

stwo_cairo_prover/crates/prover/src/cairo_air/preprocessed.rs line 124 at r4 (raw file):

                self.n_bits, self.col_index
            ),
        }

it's very clear that this is a preprocessed column ID

Suggestion:

    pub fn id(&self) -> PreProcessedColumnId {
        PreProcessedColumnId {
            id: format!(
                "bitwise_xor_{}_{}",
                self.n_bits, self.col_index
            ),
        }

---

stwo_cairo_prover/crates/prover/src/cairo_air/preprocessed.rs line 131 at r4 (raw file):

    }

    fn packed_at_lhs(&self, vec_row: usize) -> PackedM31 {

don't mix M31 with Basefield,
IMO in AIR context the correct term is M31 , and in the prover context the term is BaseField

---

stwo_cairo_prover/crates/prover/src/cairo_air/preprocessed.rs line 163 at r4 (raw file):

            _ => unreachable!(),
        }
    }

Suggestion:

    pub fn packed_at(&self, vec_row: usize) -> PackedM31 {
        let lhs = || -> u32x16 {
            (SIMD_ENUMERATION_0 + Simd::splat((vec_row * N_LANES) as u32)) >> self.n_bits
        };
        let rhs = || -> u32x16 {
            (SIMD_ENUMERATION_0 + Simd::splat((vec_row * N_LANES) as u32))
                & Simd::splat((1 << self.n_bits) - 1)
        };
        let simd = match self.col {
            XorColumn::A => lhs(),
            XorColumn::B => rhs(),
            XorColumn::C => lhs() ^ rhs(),
        };
        unsafe { PackedM31::from_simd_unchecked(simd) }
    }

[Gali-StarkWare]

Reviewable status: 0 of 1 files reviewed, 6 unresolved discussions (waiting on @alon-f, @andrewmilson, @ohad-starkware, and @shaharsamocha7)

---

stwo_cairo_prover/crates/prover/src/cairo_air/preprocessed.rs line 105 at r4 (raw file):

Previously, ohad-starkware (Ohad) wrote…

A table of a,b,c columns, where

Done.

---

stwo_cairo_prover/crates/prover/src/cairo_air/preprocessed.rs line 108 at r4 (raw file):

Previously, ohad-starkware (Ohad) wrote…

?

IMO there is no need for that

---

stwo_cairo_prover/crates/prover/src/cairo_air/preprocessed.rs line 110 at r4 (raw file):

Previously, ohad-starkware (Ohad) wrote…

use an enum for column identification in the table, makes the assert below redundent

I like it, but the problem air-infra will send 0/1/2 (we can try to change their mind). That's why we didn't do it in the first place.

---

stwo_cairo_prover/crates/prover/src/cairo_air/preprocessed.rs line 124 at r4 (raw file):

Previously, ohad-starkware (Ohad) wrote…

it's very clear that this is a preprocessed column ID

It's for consistency with the rest of the columns, changed it for seq as well (is first is already being deleted by Shahar)

---

stwo_cairo_prover/crates/prover/src/cairo_air/preprocessed.rs line 131 at r4 (raw file):

Previously, ohad-starkware (Ohad) wrote…

don't mix M31 with Basefield,
IMO in AIR context the correct term is M31 , and in the prover context the term is BaseField

Done.

---

stwo_cairo_prover/crates/prover/src/cairo_air/preprocessed.rs line 216 at r4 (raw file):

Previously, shaharsamocha7 wrote…

I suggest to check all the xor columns on a specific index, WDYT?

Done.

---

stwo_cairo_prover/crates/prover/src/cairo_air/preprocessed.rs line 163 at r4 (raw file):

            _ => unreachable!(),
        }
    }

Shahar wanted it to be in different functions. Why to use splat? Efficiency? Ilike that from_u32_unchecked keeps the unsafe in one place.

[ohad-starkware]

Reviewed all commit messages.
Reviewable status: 0 of 1 files reviewed, 2 unresolved discussions (waiting on @alon-f, @andrewmilson, @Gali-StarkWare, and @shaharsamocha7)

---

stwo_cairo_prover/crates/prover/src/cairo_air/preprocessed.rs line 110 at r4 (raw file):

Previously, Gali-StarkWare wrote…

I like it, but the problem air-infra will send 0/1/2 (we can try to change their mind). That's why we didn't do it in the first place.

yikes ok

---

stwo_cairo_prover/crates/prover/src/cairo_air/preprocessed.rs line 163 at r4 (raw file):

Previously, Gali-StarkWare wrote…

Shahar wanted it to be in different functions. Why to use splat? Efficiency? Ilike that from_u32_unchecked keeps the unsafe in one place.

I think my suggestion is 1.simpler, 2.shorter, 3.much more efficient (not because of splat, but the try into and collect vec)
also unsafe is still in one location

[ohad-starkware]

Reviewable status: 0 of 1 files reviewed, 2 unresolved discussions (waiting on @alon-f, @andrewmilson, @Gali-StarkWare, and @shaharsamocha7)

---

stwo_cairo_prover/crates/prover/src/cairo_air/preprocessed.rs line 163 at r4 (raw file):

Previously, ohad-starkware (Ohad) wrote…

I think my suggestion is 1.simpler, 2.shorter, 3.much more efficient (not because of splat, but the try into and collect vec)
also unsafe is still in one location

and unwrap!!
you have 2 redundent ifs and 1 not needed dynamic allocation

[Gali-StarkWare]

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

---

stwo_cairo_prover/crates/prover/src/cairo_air/preprocessed.rs line 163 at r4 (raw file):

Previously, ohad-starkware (Ohad) wrote…

and unwrap!!
you have 2 redundent ifs and 1 not needed dynamic allocation

Done. Also added the enum, wdyt?

[ohad-starkware]

Reviewable status: 0 of 1 files reviewed, 2 unresolved discussions (waiting on @alon-f, @andrewmilson, @Gali-StarkWare, and @shaharsamocha7)

---

stwo_cairo_prover/crates/prover/src/cairo_air/preprocessed.rs line 163 at r4 (raw file):

Previously, Gali-StarkWare wrote…

Done. Also added the enum, wdyt?

I don't like it :(
if you want to translate between two interfaces, the codegen would be the place.
otherwise, and preferably, just use the AIR team definitions, this is a NIT pick really

[shaharsamocha7]

Reviewable status: 0 of 1 files reviewed, 2 unresolved discussions (waiting on @alon-f, @andrewmilson, and @ohad-starkware)

---

stwo_cairo_prover/crates/prover/src/cairo_air/preprocessed.rs line 163 at r4 (raw file):

Previously, ohad-starkware (Ohad) wrote…

I don't like it :(
if you want to translate between two interfaces, the codegen would be the place.
otherwise, and preferably, just use the AIR team definitions, this is a NIT pick really

Like on this implementation!
If the air infra uses 0,1,2 this translation between numbers to enum will happen anyway
I don't see why it is better to be in the code gen rather than here.
@ohad-starkware can you explain?

[ohad-starkware]

Reviewable status: 0 of 1 files reviewed, 2 unresolved discussions (waiting on @alon-f, @andrewmilson, and @shaharsamocha7)

---

stwo_cairo_prover/crates/prover/src/cairo_air/preprocessed.rs line 163 at r4 (raw file):

Previously, shaharsamocha7 wrote…

Like on this implementation!
If the air infra uses 0,1,2 this translation between numbers to enum will happen anyway
I don't see why it is better to be in the code gen rather than here.
@ohad-starkware can you explain?

as is the introduction of this enum is redundant,
if your interface is A why translate it to B in the same context you are using B
this is equivalent to documenting each case

[ohad-starkware]

Reviewable status: 0 of 1 files reviewed, 2 unresolved discussions (waiting on @alon-f, @andrewmilson, and @shaharsamocha7)

---

stwo_cairo_prover/crates/prover/src/cairo_air/preprocessed.rs line 163 at r4 (raw file):

Previously, ohad-starkware (Ohad) wrote…

as is the introduction of this enum is redundant,
if your interface is A why translate it to B in the same context you are using B
this is equivalent to documenting each case

I have no concrete problem with it btw, but don't think it's a change to the positive

[shaharsamocha7]

Reviewable status: 0 of 1 files reviewed, 2 unresolved discussions (waiting on @alon-f, @andrewmilson, and @ohad-starkware)

---

stwo_cairo_prover/crates/prover/src/cairo_air/preprocessed.rs line 163 at r4 (raw file):

Previously, ohad-starkware (Ohad) wrote…

I have no concrete problem with it btw, but don't think it's a change to the positive

Ok I agree.
Let's discard this enum in this pr and we can think if we want to change to enum in a later pr but then new() should get the enum and not a usize, WDYT?

[ohad-starkware]

Reviewable status: 0 of 1 files reviewed, all discussions resolved (waiting on @alon-f and @andrewmilson)

---

stwo_cairo_prover/crates/prover/src/cairo_air/preprocessed.rs line 163 at r4 (raw file):

Previously, shaharsamocha7 wrote…

Ok I agree.
Let's discard this enum in this pr and we can think if we want to change to enum in a later pr but then new() should get the enum and not a usize, WDYT?

yes

[Gali-StarkWare]

Reviewed 1 of 1 files at r9, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @alon-f, @andrewmilson, and @shaharsamocha7)

[shaharsamocha7]

Reviewed all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @alon-f and @andrewmilson)