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

jump #221

Merged
merged 1 commit into from
Dec 5, 2024
Merged

jump #221

merged 1 commit into from
Dec 5, 2024

Conversation

ohad-starkware
Copy link
Contributor

@ohad-starkware ohad-starkware commented Dec 2, 2024

This change is Reviewable

@ohad-starkware ohad-starkware mentioned this pull request Dec 2, 2024
@ohad-starkware ohad-starkware mentioned this pull request Dec 2, 2024
@ohad-starkware ohad-starkware mentioned this pull request Dec 4, 2024
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 all commit messages.
Reviewable status: 0 of 15 files reviewed, 3 unresolved discussions (waiting on @ohad-starkware)


a discussion (no related file):
Rename the jumps to
jump_x_y_z (x,y,z \in {f,t})


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

    jnz_t_t: Vec<jnz_opcode_is_taken_t_dst_base_fp_t::Claim>,
    jump_f_f: Vec<jump_opcode_is_rel_f_is_imm_f_is_double_deref_f::Claim>,
    jump_f_t: Vec<jump_opcode_is_rel_f_is_imm_f_is_double_deref_t::Claim>,

Rename to jump_fft :)

Suggestion:

jump_f_f_t: Vec<jump_opcode_is_rel_f_is_imm_f_is_double_deref_t::Claim>

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

        self.jump_f_f.iter().for_each(|c| c.mix_into(channel));
        self.jump_f_t.iter().for_each(|c| c.mix_into(channel));
        self.jump_t_f.iter().for_each(|c| c.mix_into(channel));

You missed one

Suggestion:

        self.jump_f_f.iter().for_each(|c| c.mix_into(channel));
        self.jump_f_t.iter().for_each(|c| c.mix_into(channel));
        self.jump_t_f.iter().for_each(|c| c.mix_into(channel));
        self.jump_t_t.iter().for_each(|c| c.mix_into(channel));

Copy link
Contributor Author

@ohad-starkware ohad-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: 0 of 15 files reviewed, 3 unresolved discussions (waiting on @shaharsamocha7)


a discussion (no related file):

Previously, shaharsamocha7 wrote…

Rename the jumps to
jump_x_y_z (x,y,z \in {f,t})

Done.s


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

Previously, shaharsamocha7 wrote…

Rename to jump_fft :)

Done.


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

Previously, shaharsamocha7 wrote…

You missed one

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.

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


stwo_cairo_prover/crates/prover/src/cairo_air/opcodes_air.rs line 154 at r5 (raw file):

        let mut jump_f_t = vec![];
        let mut jump_t_f = vec![];
        let mut jump_t_t = vec![];

also here
and anywhere

Code quote:

        let mut jump_f_f = vec![];
        let mut jump_f_t = vec![];
        let mut jump_t_f = vec![];
        let mut jump_t_t = vec![];

Copy link
Contributor Author

@ohad-starkware ohad-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: 0 of 15 files reviewed, 2 unresolved discussions (waiting on @shaharsamocha7)


stwo_cairo_prover/crates/prover/src/cairo_air/opcodes_air.rs line 154 at r5 (raw file):

Previously, shaharsamocha7 wrote…

also here
and anywhere

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.

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


stwo_cairo_prover/crates/prover/src/cairo_air/opcodes_air.rs line 1184 at r6 (raw file):

            })
            .collect();
        let jump_f_f_interaction_claims = self

:(

Suggestion:

let jump_f_f_f_interaction_claims = self

stwo_cairo_prover/crates/prover/src/cairo_air/opcodes_air.rs line 1693 at r6 (raw file):

            })
            .collect_vec();
        let jump_f_f_components = claim

:( x2

Suggestion:

 let jump_f_f_f_components = claim

Copy link
Contributor Author

@ohad-starkware ohad-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: 14 of 15 files reviewed, 2 unresolved discussions (waiting on @shaharsamocha7)


stwo_cairo_prover/crates/prover/src/cairo_air/opcodes_air.rs line 1184 at r6 (raw file):

Previously, shaharsamocha7 wrote…

:(

Done.


stwo_cairo_prover/crates/prover/src/cairo_air/opcodes_air.rs line 1693 at r6 (raw file):

Previously, shaharsamocha7 wrote…

:( x2

:O

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 1 of 1 files at r7, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @ohad-starkware)

Copy link
Contributor Author

ohad-starkware commented Dec 5, 2024

Merge activity

  • Dec 5, 10:57 AM EST: A user started a stack merge that includes this pull request via Graphite.
  • Dec 5, 11:05 AM EST: Graphite rebased this pull request as part of a merge.
  • Dec 5, 11:09 AM EST: A user merged this pull request with Graphite.

@ohad-starkware ohad-starkware changed the base branch from ohad/add_opcode_gen to graphite-base/221 December 5, 2024 15:58
@ohad-starkware ohad-starkware changed the base branch from graphite-base/221 to main December 5, 2024 16:03
@ohad-starkware ohad-starkware merged commit 820dc76 into main Dec 5, 2024
6 checks passed
This was referenced Dec 8, 2024
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.

2 participants