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 add and sub for i*. #3620

Merged
merged 1 commit into from
Jul 25, 2023
Merged

Added add and sub for i*. #3620

merged 1 commit into from
Jul 25, 2023

Conversation

orizi
Copy link
Collaborator

@orizi orizi commented Jul 6, 2023

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

@orizi orizi requested a review from spapinistarkware July 6, 2023 14:41
@orizi orizi force-pushed the pr/orizi/orizi/signed-ints/295e9468 branch from 1b8b4a6 to e028653 Compare July 6, 2023 16:51
@orizi orizi force-pushed the pr/orizi/orizi/signed-ints/a8262aa8 branch from d5bceb0 to 7e47668 Compare July 6, 2023 16:51
@orizi orizi force-pushed the pr/orizi/orizi/signed-ints/295e9468 branch from e028653 to 7b6aa4c Compare July 6, 2023 17:34
@orizi orizi force-pushed the pr/orizi/orizi/signed-ints/a8262aa8 branch 2 times, most recently from 17d6ecb to 473ea6c Compare July 6, 2023 18:02
@orizi orizi force-pushed the pr/orizi/orizi/signed-ints/295e9468 branch from 7b6aa4c to 38e20dd Compare July 6, 2023 18:02
@orizi orizi force-pushed the pr/orizi/orizi/signed-ints/a8262aa8 branch 2 times, most recently from ebea62b to ef5d31b Compare July 9, 2023 04:35
@orizi orizi force-pushed the pr/orizi/orizi/signed-ints/295e9468 branch from 0fc0a34 to e847b7d Compare July 9, 2023 17:44
@orizi orizi force-pushed the pr/orizi/orizi/signed-ints/a8262aa8 branch 2 times, most recently from dbd3927 to de9b6d5 Compare July 10, 2023 03:27
@orizi orizi force-pushed the pr/orizi/orizi/signed-ints/a8262aa8 branch from de9b6d5 to ea5a9fb Compare July 24, 2023 10:19
Copy link
Contributor

@spapinistarkware spapinistarkware 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 16 files at r1.
Reviewable status: 0 of 16 files reviewed, 1 unresolved discussion (waiting on @orizi)


corelib/src/test/integer_test.cairo line 1142 at r2 (raw file):

    assert_eq(@(-121_i8 - -21_i8), @-100_i8, '-121--21=-100');
}

Please check sub overflow and add underflow

@orizi orizi force-pushed the pr/orizi/orizi/signed-ints/a8262aa8 branch from ea5a9fb to c30ee11 Compare July 24, 2023 10:48
@orizi orizi force-pushed the pr/orizi/orizi/signed-ints/295e9468 branch from 119a1fa to 7cd8370 Compare July 24, 2023 10:48
Copy link
Contributor

@spapinistarkware spapinistarkware 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 16 files reviewed, 3 unresolved discussions (waiting on @orizi)


crates/cairo-lang-sierra-to-casm/src/invocations/int/signed.rs line 122 at r3 (raw file):

        jump IsInRange if is_in_range != 0;
        tempvar is_overflow;
        hint TestLessThan {lhs: value, rhs: range_size} into {dst: is_overflow};

value might be up to 2*MAX, but you check here value <= MAX-MIN. This works if MIN=-MAX-1, but this assumption should be added to the documentation somewhere, so we won't try to use it for weird ranges.


crates/cairo-lang-sierra-to-casm/src/invocations/int/signed.rs line 124 at r3 (raw file):

        hint TestLessThan {lhs: value, rhs: range_size} into {dst: is_overflow};
        jump IsOverflow if is_overflow != 0;
    // IsUnderflow:

Remove or indent

Copy link
Contributor

@spapinistarkware spapinistarkware 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 16 files at r1, 8 of 8 files at r2, 1 of 1 files at r3, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @orizi)

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


corelib/src/test/integer_test.cairo line 1142 at r2 (raw file):

Previously, spapinistarkware (Shahar Papini) wrote…

Please check sub overflow and add underflow

Done.

@orizi orizi force-pushed the pr/orizi/orizi/signed-ints/a8262aa8 branch 2 times, most recently from cd5ab66 to e372958 Compare July 24, 2023 11:39
@orizi orizi force-pushed the pr/orizi/orizi/signed-ints/295e9468 branch from 7cd8370 to 67b9d03 Compare July 24, 2023 12:26
@orizi orizi force-pushed the pr/orizi/orizi/signed-ints/a8262aa8 branch from e372958 to 57e7c6b Compare July 24, 2023 12:26
@orizi orizi force-pushed the pr/orizi/orizi/signed-ints/295e9468 branch from 67b9d03 to d7339bc Compare July 24, 2023 12:48
@orizi orizi force-pushed the pr/orizi/orizi/signed-ints/a8262aa8 branch from 57e7c6b to 6e6567f Compare July 24, 2023 12:48
@orizi orizi changed the base branch from pr/orizi/orizi/signed-ints/295e9468 to main July 24, 2023 13:54
@orizi orizi force-pushed the pr/orizi/orizi/signed-ints/a8262aa8 branch from 6e6567f to 6957643 Compare July 24, 2023 13:54
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: 13 of 16 files reviewed, 3 unresolved discussions (waiting on @spapinistarkware)


crates/cairo-lang-sierra-to-casm/src/invocations/int/signed.rs line 122 at r3 (raw file):

Previously, spapinistarkware (Shahar Papini) wrote…

value might be up to 2*MAX, but you check here value <= MAX-MIN. This works if MIN=-MAX-1, but this assumption should be added to the documentation somewhere, so we won't try to use it for weird ranges.

do you mean that this only works for MIN <= 0 && MAX-MIN < 2**128?
because otherwise i'm not sure i understand.
Added asserts for this.


crates/cairo-lang-sierra-to-casm/src/invocations/int/signed.rs line 124 at r3 (raw file):

Previously, spapinistarkware (Shahar Papini) wrote…

Remove or indent

Done.

@orizi orizi force-pushed the pr/orizi/orizi/signed-ints/a8262aa8 branch from 6957643 to 6409ff9 Compare July 24, 2023 15:00
commit-id:a8262aa8
@orizi orizi force-pushed the pr/orizi/orizi/signed-ints/a8262aa8 branch from 6409ff9 to a849524 Compare July 25, 2023 14:16
Copy link
Contributor

@spapinistarkware spapinistarkware 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 2 files at r4, 1 of 2 files at r5, 5 of 5 files at r6, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @orizi)

@orizi orizi enabled auto-merge July 25, 2023 14:19
@orizi orizi added this pull request to the merge queue Jul 25, 2023
Merged via the queue into main with commit d96a302 Jul 25, 2023
@orizi orizi deleted the pr/orizi/orizi/signed-ints/a8262aa8 branch August 10, 2023 06:51
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