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

Made arguments to functions in run injected through vm and not code. #6489

Merged
merged 1 commit into from
Oct 23, 2024

Conversation

orizi
Copy link
Collaborator

@orizi orizi commented Oct 15, 2024

No description provided.

Copy link
Collaborator

@piotmag769 piotmag769 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just to make sure I understand it correctly: the header with segments initalization and function call is still appended, just not the args, rights?

Reviewed 2 of 2 files at r1, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @gilbens-starkware and @ilyalesokhin-starkware)

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.

yes - the args (including the gas builtin) are just not a part of the code - but their size is allocated in code, and the VM fills them.

Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @gilbens-starkware and @ilyalesokhin-starkware)

@orizi orizi force-pushed the spr/main/f972f5b1 branch from b6bceae to 1387736 Compare October 20, 2024 08:48
@orizi orizi force-pushed the spr/main/4dc3b5b4 branch from 087c498 to 225228e Compare October 20, 2024 08:48
@lotem-starkware
Copy link

This change is Reviewable

@orizi orizi changed the base branch from spr/main/4dc3b5b4 to main October 20, 2024 11:55
@orizi orizi force-pushed the spr/main/f972f5b1 branch from 1387736 to b904699 Compare October 20, 2024 11:55
@ilyalesokhin-starkware
Copy link
Contributor

crates/cairo-lang-runner/src/lib.rs line 672 at r1 (raw file):

                let offset = ap_offset - used_args;
                for _ in 0..*ty_size {
                    casm_extend!(ctx, [ap + 0] = [ap - offset], ap++;);

can't you put the ap_offset += params_size; before the builtins instead of copying the parameters??

Code quote:

casm_extend!(ctx, [ap + 0] = [ap - offset], ap++;);

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


crates/cairo-lang-runner/src/lib.rs line 672 at r1 (raw file):

Previously, ilyalesokhin-starkware wrote…

can't you put the ap_offset += params_size; before the builtins instead of copying the parameters??

checking if knowing where to inject the params is easy on the other side.
but in general - does it matter here? in reality we do need something like that for the actual runnable format - won't we?

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.

Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @gilbens-starkware)

@ilyalesokhin-starkware
Copy link
Contributor

crates/cairo-lang-runner/src/lib.rs line 651 at r1 (raw file):

        let mut used_args = 0;
        for ty in param_types {
            let (generic_ty, ty_size) = ty;

Suggestion:

        for (generic_ty, ty_size) in param_types {

@ilyalesokhin-starkware
Copy link
Contributor

crates/cairo-lang-runner/src/lib.rs line 671 at r1 (raw file):

            } else {
                let offset = ap_offset - used_args;
                for _ in 0..*ty_size {

do you support a param that is not a felt on an array?
size > 1?

Code quote:

ty_size

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:

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @gilbens-starkware and @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, 2 unresolved discussions (waiting on @gilbens-starkware and @ilyalesokhin-starkware)


crates/cairo-lang-runner/src/lib.rs line 651 at r1 (raw file):

        let mut used_args = 0;
        for ty in param_types {
            let (generic_ty, ty_size) = ty;

Done.


crates/cairo-lang-runner/src/lib.rs line 671 at r1 (raw file):

Previously, ilyalesokhin-starkware wrote…

do you support a param that is not a felt on an array?
size > 1?

yes - structs that hold multiple such elements fully work.

@orizi orizi force-pushed the spr/main/f972f5b1 branch from b904699 to 16f3d2e Compare October 22, 2024 13:33
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.

Reviewable status: 1 of 2 files reviewed, all discussions resolved (waiting on @gilbens-starkware and @piotmag769)

@orizi orizi added this pull request to the merge queue Oct 23, 2024
Merged via the queue into main with commit 68938b7 Oct 23, 2024
86 of 87 checks passed
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.

bug: Cairo runner does not support array arguments larger than 2^15
4 participants