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

mul #219

Merged
merged 1 commit into from
Dec 5, 2024
Merged

mul #219

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 force-pushed the ohad/mul branch 2 times, most recently from 880db38 to cc2f968 Compare December 2, 2024 11:16
This was referenced Dec 2, 2024
@ohad-starkware ohad-starkware force-pushed the ohad/mul branch 2 times, most recently from d8b7f79 to 5cd292c Compare December 3, 2024 11:46
@ohad-starkware ohad-starkware changed the base branch from ohad/assert_eq to graphite-base/219 December 4, 2024 12:07
@ohad-starkware ohad-starkware changed the base branch from graphite-base/219 to main December 4, 2024 12:07
@ohad-starkware ohad-starkware force-pushed the ohad/mul branch 2 times, most recently from 502d05b to ab85e9e Compare December 4, 2024 12: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 10 of 12 files at r1, 1 of 1 files at r2, all commit messages.
Reviewable status: 11 of 12 files reviewed, 4 unresolved discussions (waiting on @ohad-starkware)


a discussion (no related file):
I think Stav should check the changes in state_transitions.rs file


stwo_cairo_prover/crates/prover/src/components/range_check_vector/mod.rs line 78 at r2 (raw file):

generate_range_check_code!([4, 3]);
generate_range_check_code!([6]);
generate_range_check_code!([3]);

Does this work with current impl? (the N_LANES problem)

Code quote:

generate_range_check_code!([3]);

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

                input.casm_states_by_opcode.mul_opcode_is_small_t_is_imm_f,
            ));
        }

You also need to push here the big muls right?
Currently this only handles the small ones

Code quote:

        if !input
            .casm_states_by_opcode
            .mul_opcode_is_small_t_is_imm_f
            .is_empty()
        {
            mul_f_f.push(mul_opcode_is_small_f_is_imm_f::ClaimGenerator::new(
                input.casm_states_by_opcode.mul_opcode_is_small_t_is_imm_f,
            ));
        }

stwo_cairo_prover/crates/prover/src/components/mod.rs line 26 at r2 (raw file):

pub use memory::{memory_address_to_id, memory_id_to_big};
pub use range_check_vector::{
    range_check_19, range_check_3, range_check_4_3, range_check_6, range_check_7_2_5,

I thought we don't want to use that

Code quote:

range_check_3,

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: 11 of 12 files reviewed, 4 unresolved discussions (waiting on @shaharsamocha7 and @Stavbe)


a discussion (no related file):

Previously, shaharsamocha7 wrote…

Here you only add large muls, right?
Don't we have the small mul yet?

the air has rc3 in it and wer'e not supporting that yet


a discussion (no related file):

Previously, shaharsamocha7 wrote…

I think Stav should check the changes in state_transitions.rs file

@Stavbe

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: 11 of 12 files reviewed, 4 unresolved discussions (waiting on @shaharsamocha7 and @Stavbe)


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

Previously, shaharsamocha7 wrote…

You also need to push here the big muls right?
Currently this only handles the small ones

bug, done.

@Stavbe Stavbe requested a review from shaharsamocha7 December 5, 2024 09:21
Copy link
Contributor

@Stavbe Stavbe 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: 11 of 12 files reviewed, 4 unresolved discussions (waiting on @ohad-starkware, @shaharsamocha7, and @Stavbe)


a discussion (no related file):

Previously, ohad-starkware (Ohad) wrote…

@Stavbe

lgtm

@Stavbe Stavbe self-requested a review December 5, 2024 09:21
@ohad-starkware ohad-starkware force-pushed the ohad/mul branch 2 times, most recently from f2d205d to 64a5bbb Compare December 5, 2024 11:40
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: 9 of 13 files reviewed, 4 unresolved discussions (waiting on @shaharsamocha7 and @Stavbe)


stwo_cairo_prover/crates/prover/src/components/mod.rs line 26 at r2 (raw file):

Previously, shaharsamocha7 wrote…

I thought we don't want to use that

and not 6 (yet), fixed


stwo_cairo_prover/crates/prover/src/components/range_check_vector/mod.rs line 78 at r2 (raw file):

Previously, shaharsamocha7 wrote…

Does this work with current impl? (the N_LANES problem)

removed

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 4 of 4 files at r3, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @ohad-starkware and @Stavbe)


stwo_cairo_prover/crates/prover/src/input/state_transitions.rs line 619 at r3 (raw file):

// Returns 'true' if the a memory value is within the range of [-2^27, 2^27 - 1].
fn is_small(val: MemoryValue) -> bool {
    matches!(val, MemoryValue::Small(val) if (val as i128>= SMALL_MIN_VALUE as i128) && (val as i128 <= SMALL_MAX_VALUE as i128))

currently small values are in the range [0, 2**72),
Do we really support small negative values or should we drop it for now?

Code quote:

MemoryValue::Small(val) if (val as i128>= SMALL_MIN_VALUE as i128)

stwo_cairo_prover/crates/prover/src/input/state_transitions.rs line 619 at r3 (raw file):

// Returns 'true' if the a memory value is within the range of [-2^27, 2^27 - 1].
fn is_small(val: MemoryValue) -> bool {
    matches!(val, MemoryValue::Small(val) if (val as i128>= SMALL_MIN_VALUE as i128) && (val as i128 <= SMALL_MAX_VALUE as i128))

Can we test those functions?

Code quote:

// Returns 'true' if all the operands are within the range of [-2^27, 2^27 - 1].
fn are_small_operands(dst: MemoryValue, op0: MemoryValue, op1: MemoryValue) -> bool {
    is_small(dst) && is_small(op0) && is_small(op1)
}

// Returns 'true' if the a memory value is within the range of [-2^27, 2^27 - 1].
fn is_small(val: MemoryValue) -> bool {
    matches!(val, MemoryValue::Small(val) if (val as i128>= SMALL_MIN_VALUE as i128) && (val as i128 <= SMALL_MAX_VALUE as i128))

stwo_cairo_prover/crates/prover/src/input/vm_import/mod.rs line 216 at r3 (raw file):

    }

    // When not ignored, the test passes only with dev_mod = false.

Why do we have this comment?
Either un-ignore it and use dev_mod=false
or just remove the ignore when it is supported.

Code quote:

// When not ignored, the test passes only with dev_mod = false.

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: all files reviewed, 3 unresolved discussions (waiting on @shaharsamocha7 and @Stavbe)


stwo_cairo_prover/crates/prover/src/input/state_transitions.rs line 619 at r3 (raw file):

Previously, shaharsamocha7 wrote…

currently small values are in the range [0, 2**72),
Do we really support small negative values or should we drop it for now?

its a different small, thats changes from stavs pr


stwo_cairo_prover/crates/prover/src/input/state_transitions.rs line 619 at r3 (raw file):

Previously, shaharsamocha7 wrote…

Can we test those functions?

stavs pr


stwo_cairo_prover/crates/prover/src/input/vm_import/mod.rs line 216 at r3 (raw file):

Previously, shaharsamocha7 wrote…

Why do we have this comment?
Either un-ignore it and use dev_mod=false
or just remove the ignore when it is supported.

stavs pr

@ohad-starkware ohad-starkware changed the base branch from main to stav/fix_adapter_big_small December 5, 2024 13:39
@ohad-starkware ohad-starkware changed the base branch from stav/fix_adapter_big_small to main December 5, 2024 13:40
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 @ohad-starkware and @Stavbe)


stwo_cairo_prover/crates/prover/src/input/vm_import/mod.rs line 216 at r3 (raw file):

Previously, ohad-starkware (Ohad) wrote…

stavs pr

This should be removed from there as well
unblocking

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: 12 of 13 files reviewed, 1 unresolved discussion (waiting on @shaharsamocha7 and @Stavbe)


stwo_cairo_prover/crates/prover/src/input/vm_import/mod.rs line 216 at r3 (raw file):

Previously, shaharsamocha7 wrote…

This should be removed from there as well
unblocking

will be removed with all the dev mode stuff soon enough

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, 10:57 AM EST: A user merged this pull request with Graphite.

@ohad-starkware ohad-starkware merged commit 59075f7 into main Dec 5, 2024
5 of 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.

3 participants