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

Added multi-pop for span. #5588

Merged
merged 1 commit into from
May 21, 2024
Merged

Added multi-pop for span. #5588

merged 1 commit into from
May 21, 2024

Conversation

orizi
Copy link
Collaborator

@orizi orizi commented May 19, 2024

Stack:

⚠️ Part of a stack created by spr. Do not merge manually using the UI - doing so may have unexpected results.


This change is Reviewable

@ilyalesokhin-starkware
Copy link
Contributor

crates/cairo-lang-sierra/src/extensions/modules/array.rs line 633 at r1 (raw file):

                                        ty,
                                        count
                                            .to_usize()

why usize here an i16 in the implementation?

Code quote:

                                        count
                                            .to_usize()

@ilyalesokhin-starkware
Copy link
Contributor

crates/cairo-lang-sierra/src/extensions/modules/utils.rs line 116 at r1 (raw file):

/// Returns a fixed type array of the given type and size.
pub fn fixed_array_ty(

Suggestion:

fixed_size_array_ty

@ilyalesokhin-starkware
Copy link
Contributor

crates/cairo-lang-sierra-to-casm/src/invocations/array.rs line 391 at r1 (raw file):

        deref arr_end;
    };
    let popped_size: i16 = libfunc.popped_values * element_size;

why i16?

Code quote:

i16

Copy link
Contributor

@ilyalesokhin-starkware ilyalesokhin-starkware 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 10 files at r1, all commit messages.
Reviewable status: 7 of 10 files reviewed, 5 unresolved discussions (waiting on @orizi)


crates/cairo-lang-sierra-to-casm/src/invocations/array.rs line 448 at r1 (raw file):

    );
    casm_build_extend! {casm_builder,
        const popped_size = popped_size;

Suggestion:

 // Success case
 const popped_size = popped_size;

crates/cairo-lang-sierra-to-casm/src/invocations/array.rs line 486 at r1 (raw file):

        tempvar minus_length = arr_start - arr_end;
        tempvar rc = minus_length + popped_size_plus_1;
        assert rc = *(range_check++);

Why is the negative case
popped_size_plus_1 >= array_length?

shouldn't it be
array_length < popped_size
?

Suggestion:

        /// Prove that popped_size_plus_1 >= array_length        
        tempvar minus_length = arr_start - arr_end;
        tempvar rc = minus_length + popped_size_plus_1;
        assert rc = *(range_check++);

@orizi orizi force-pushed the pr/orizi/multi-pop/de150c40 branch from 2361876 to f73ee30 Compare May 20, 2024 09:10
@orizi orizi force-pushed the pr/orizi/multi-pop/813e40ab branch from eb1dcc6 to 2d0412d Compare May 20, 2024 09:10
Copy link
Collaborator Author

@orizi orizi 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: 4 of 10 files reviewed, 5 unresolved discussions (waiting on @ilyalesokhin-starkware)


crates/cairo-lang-sierra/src/extensions/modules/array.rs line 633 at r1 (raw file):

Previously, ilyalesokhin-starkware wrote…

why usize here an i16 in the implementation?

it is for being repeated - but changing to i16.


crates/cairo-lang-sierra/src/extensions/modules/utils.rs line 116 at r1 (raw file):

/// Returns a fixed type array of the given type and size.
pub fn fixed_array_ty(

Done.


crates/cairo-lang-sierra-to-casm/src/invocations/array.rs line 448 at r1 (raw file):

    );
    casm_build_extend! {casm_builder,
        const popped_size = popped_size;

Done.

Copy link
Collaborator Author

@orizi orizi 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: 4 of 10 files reviewed, 5 unresolved discussions (waiting on @ilyalesokhin-starkware)


crates/cairo-lang-sierra-to-casm/src/invocations/array.rs line 391 at r1 (raw file):

Previously, ilyalesokhin-starkware wrote…

why i16?

since this is the bound for actual type sizes.

Copy link
Collaborator Author

@orizi orizi 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: 4 of 10 files reviewed, 5 unresolved discussions (waiting on @ilyalesokhin-starkware)


crates/cairo-lang-sierra-to-casm/src/invocations/array.rs line 486 at r1 (raw file):

Previously, ilyalesokhin-starkware wrote…

Why is the negative case
popped_size_plus_1 >= array_length?

shouldn't it be
array_length < popped_size
?

Done.

@ilyalesokhin-starkware
Copy link
Contributor

crates/cairo-lang-sierra-to-casm/src/invocations/array.rs line 487 at r2 (raw file):

        jump HasEnoughElements if has_enough_elements != 0;
        // Proving that `arr_start - arr_end + popped_size + 1 >= 0` and therefore
        // `popped_size + 1 >= arr_end - arr_end == arr.len()` ==> `arr.len() < popopped_size`.

Suggestion:

`popped_size + 1 >= arr_end - arr_start

Copy link
Contributor

@ilyalesokhin-starkware ilyalesokhin-starkware 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 10 files at r1, 4 of 4 files at r2, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @orizi)

@orizi orizi force-pushed the pr/orizi/multi-pop/de150c40 branch from f73ee30 to 6da2e64 Compare May 21, 2024 06:22
@orizi orizi force-pushed the pr/orizi/multi-pop/813e40ab branch 2 times, most recently from 55fe474 to 32be7b1 Compare May 21, 2024 06:58
@orizi orizi force-pushed the pr/orizi/multi-pop/de150c40 branch from 6da2e64 to 7b2cd00 Compare May 21, 2024 06:58
Copy link
Collaborator Author

@orizi orizi 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 10 files reviewed, 1 unresolved discussion (waiting on @ilyalesokhin-starkware)


crates/cairo-lang-sierra-to-casm/src/invocations/array.rs line 487 at r2 (raw file):

        jump HasEnoughElements if has_enough_elements != 0;
        // Proving that `arr_start - arr_end + popped_size + 1 >= 0` and therefore
        // `popped_size + 1 >= arr_end - arr_end == arr.len()` ==> `arr.len() < popopped_size`.

Done.

commit-id:813e40ab
@orizi orizi changed the base branch from pr/orizi/multi-pop/de150c40 to main May 21, 2024 07:21
@orizi orizi force-pushed the pr/orizi/multi-pop/813e40ab branch from 32be7b1 to 5f468ad Compare May 21, 2024 07:21
@orizi orizi enabled auto-merge May 21, 2024 07:22
@orizi orizi added this pull request to the merge queue May 21, 2024
Merged via the queue into main with commit 5a34697 May 21, 2024
42 of 43 checks passed
@orizi orizi deleted the pr/orizi/multi-pop/813e40ab branch June 10, 2024 16:01
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