Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add Range Check 128 Builtin Files #371

Merged
merged 1 commit into from
Jan 27, 2025
Merged

Add Range Check 128 Builtin Files #371

merged 1 commit into from
Jan 27, 2025

Conversation

Gali-StarkWare
Copy link
Contributor

@Gali-StarkWare Gali-StarkWare commented Jan 20, 2025

This change is Reviewable

Copy link
Contributor Author

Gali-StarkWare commented Jan 20, 2025

Copy link

@alon-f alon-f left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:lgtm:

Reviewed 4 of 4 files at r1, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @shaharsamocha7)

Copy link
Contributor

@shaharsamocha7 shaharsamocha7 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


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

}
impl ClaimGenerator {
    pub fn new(n_rows: usize, range_check_builtin_segment_start: u32) -> Self {

Let's assert here the n_rows is a power of 2

Code quote:

pub fn new(n_rows: usize, range_check_builtin_segment_start: u32) -> Self {

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

            #[allow(clippy::needless_range_loop)]
            for i in 0..N_LANES {
                if bit_reverse_index(

We are in the case that all the rows are not padded and thus this if will always pass
Can you remove?


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

                logup_gen.finalize_at([(1 << log_size) - 1, self.n_rows - 1]);
            (trace, total_sum, Some((claimed_sum, self.n_rows - 1)))
        };

replace

Suggestion:

let (trace, claimed_sum) = logup_gen.finalize_last();

Copy link
Contributor

@shaharsamocha7 shaharsamocha7 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


stwo_cairo_prover/crates/prover/src/components/range_check_builtin_bits_128/component.rs line 28 at r1 (raw file):

#[derive(Copy, Clone, Serialize, Deserialize, CairoSerialize)]
pub struct Claim {
    pub n_rows: usize,

Consider to change this to log_size

Suggestion:

    pub log_size: usize,

Copy link
Contributor Author

@Gali-StarkWare Gali-StarkWare left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @andrewmilson and @shaharsamocha7)


stwo_cairo_prover/crates/prover/src/components/range_check_builtin_bits_128/component.rs line 28 at r1 (raw file):

Previously, shaharsamocha7 wrote…

Consider to change this to log_size

Done.


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

Previously, shaharsamocha7 wrote…

Let's assert here the n_rows is a power of 2

Changed to log_size


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

Previously, shaharsamocha7 wrote…

We are in the case that all the rows are not padded and thus this if will always pass
Can you remove?

Done.


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

Previously, shaharsamocha7 wrote…

replace

Done.

Copy link
Contributor

@shaharsamocha7 shaharsamocha7 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:lgtm:

Reviewed 2 of 2 files at r2, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @andrewmilson)

Copy link
Contributor Author

@Gali-StarkWare Gali-StarkWare left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 3 of 4 files reviewed, 1 unresolved discussion (waiting on @andrewmilson and @shaharsamocha7)


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

        SimdBackend: BackendForChannel<MC>,
    {
        let log_size = self.log_size;

I see we don't always work with max(log_size, LOG_N_LANES), maybe I can initialise it with it (after checking that the received log_size != 0) and then we don't have to remember to apply it everywhere?

Copy link
Contributor

@shaharsamocha7 shaharsamocha7 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:lgtm:

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

Copy link
Contributor

@shaharsamocha7 shaharsamocha7 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 1 of 4 files at r1.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @andrewmilson)

Copy link
Contributor Author

Gali-StarkWare commented Jan 27, 2025

Merge activity

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

@Gali-StarkWare Gali-StarkWare merged commit ff1a2c2 into main Jan 27, 2025
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants