Skip to content

add addap/jmp_abs/jmp_rel opcodes#43

Open
giladchase wants to merge 2 commits intogilad/add-lookupfrom
gilad/addap
Open

add addap/jmp_abs/jmp_rel opcodes#43
giladchase wants to merge 2 commits intogilad/add-lookupfrom
gilad/addap

Conversation

@giladchase
Copy link

@giladchase giladchase commented Jan 12, 2025

Only component, the rest is on the way

This change is Reviewable

@giladchase giladchase force-pushed the gilad/addap branch 2 times, most recently from cdac7c2 to 2e2be25 Compare January 19, 2025 06:44
@giladchase giladchase changed the title add addap add addap/jmp_abs/jmp_rel opcodes Jan 19, 2025
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 7 of 7 files at r1, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @giladchase)


crates/prover/src/components/addap_jmpabs_jmprel_opcode/component.rs line 17 at r1 (raw file):

use crate::utils::{Selector, SelectorTrait};

pub const ADD_AP_N_TRACE_CELLS: usize = 7;

ADD_AP is not really correct here

Suggestion:

pub const N_TRACE_CELLS: usize = 7;

crates/prover/src/components/addap_jmpabs_jmprel_opcode/component.rs line 23 at r1 (raw file):

// ```
// jmp_abs_imm = K + 1
// jmp_rel_imm = K + 2

Assumes Opcode_base=K such that:
// ```
// addap_imm = K
// jmp_abs_imm = K + 1
// jmp_rel_imm = K + 2

Code quote:

// Opcode base should be: `addap_imm = K`
// such that:
// ```
// jmp_abs_imm = K + 1
// jmp_rel_imm = K + 2

crates/prover/src/components/addap_jmpabs_jmprel_opcode/component.rs line 25 at r1 (raw file):

// jmp_rel_imm = K + 2
// ```
pub const INSTRUCTION_BASE: M31 = M31::from_u32_unchecked(0);

add a TODO to change the 0 to a real value

Rename above documentation to INSTRUCTION_BASE

Suggestion:

pub const K: M31 = M31::from_u32_unchecked(0);

crates/prover/src/components/addap_jmpabs_jmprel_opcode/component.rs line 60 at r1 (raw file):

                - E::F::from(THREE) * opcode_type.clone() * opcode_type.clone()
                + E::F::from(TWO) * opcode_type.clone(),
        );

Document to the poly
Consider to move this code to a function (maybe it is already exists)

Code quote:

        // assert is_trit.
        eval.add_constraint(
            opcode_type.clone() * opcode_type.clone() * opcode_type.clone()
                - E::F::from(THREE) * opcode_type.clone() * opcode_type.clone()
                + E::F::from(TWO) * opcode_type.clone(),
        );

Copy link
Author

@giladchase giladchase 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: 6 of 8 files reviewed, 4 unresolved discussions (waiting on @shaharsamocha7)


crates/prover/src/components/addap_jmpabs_jmprel_opcode/component.rs line 17 at r1 (raw file):

Previously, shaharsamocha7 wrote…

ADD_AP is not really correct here

Done.


crates/prover/src/components/addap_jmpabs_jmprel_opcode/component.rs line 23 at r1 (raw file):

Previously, shaharsamocha7 wrote…

Assumes Opcode_base=K such that:
// ```
// addap_imm = K
// jmp_abs_imm = K + 1
// jmp_rel_imm = K + 2

Done.


crates/prover/src/components/addap_jmpabs_jmprel_opcode/component.rs line 25 at r1 (raw file):

Previously, shaharsamocha7 wrote…

add a TODO to change the 0 to a real value

Rename above documentation to INSTRUCTION_BASE

That was the opcode above, moved it now to be close to this.


crates/prover/src/components/addap_jmpabs_jmprel_opcode/component.rs line 60 at r1 (raw file):

Previously, shaharsamocha7 wrote…

Document to the poly
Consider to move this code to a function (maybe it is already exists)

Done.

@giladchase giladchase force-pushed the gilad/addap branch 3 times, most recently from 302b01c to 3adeb19 Compare January 19, 2025 11:20
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 2 of 3 files at r2.
Reviewable status: 7 of 8 files reviewed, 1 unresolved discussion (waiting on @giladchase)


crates/prover/src/components/addap_jmpabs_jmprel_opcode/component.rs line 23 at r1 (raw file):

Previously, giladchase wrote…

Done.

actually maybe we want rename K -> INSTRUCTION_BASE and not the other way around
sorry :(


crates/prover/src/components/addap_jmpabs_jmprel_opcode/component.rs line 52 at r2 (raw file):

        // Assert flag is in range: {0,1,2}.
        let opcode_type = eval.next_trace_mask();

match the name with the comment

Suggestion:

        // Assert flag is in range: {0,1,2}.
        let flag = eval.next_trace_mask();

Copy link
Author

@giladchase giladchase 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: 7 of 8 files reviewed, 1 unresolved discussion (waiting on @shaharsamocha7)


crates/prover/src/components/addap_jmpabs_jmprel_opcode/component.rs line 23 at r1 (raw file):

Previously, shaharsamocha7 wrote…

actually maybe we want rename K -> INSTRUCTION_BASE and not the other way around
sorry :(

jj Done


crates/prover/src/components/addap_jmpabs_jmprel_opcode/component.rs line 52 at r2 (raw file):

Previously, shaharsamocha7 wrote…

match the name with the comment

I changed it the other way around, hope that's ok 😬

@semgrep-code-starkware-libs
Copy link

Semgrep found 1 unsafe-usage finding:

Detected 'unsafe' usage, please audit for secure usage

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 3 files at r2, 9 of 9 files at r3, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @giladchase)

- follows current design in stwo-cairo for this opcode.
- inline write_trace_row
- add LookupData that wraps InteractionClaimGenerator fields, which
  also holds n_calls.
- fix edge cases if input is smaller than LOG_N_LANES
@giladchase giladchase requested a review from Alon-Ti February 16, 2025 14:41
@giladchase giladchase force-pushed the gilad/add-lookup branch 2 times, most recently from 86f8551 to dddbf1c Compare February 16, 2025 15:11
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