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

Avoid storing zero sized objects on the stack. #4290

Merged
merged 1 commit into from
Oct 25, 2023

Conversation

ilyalesokhin-starkware
Copy link
Contributor

@ilyalesokhin-starkware ilyalesokhin-starkware commented Oct 23, 2023

This change is Reviewable

Copy link
Collaborator

@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.

:lgtm:

Reviewed 2 of 2 files at r1, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @ilyalesokhin-starkware)


crates/cairo-lang-sierra-generator/src/store_variables/state.rs line 50 at r1 (raw file):

    ZeroSizedVar,
    /// The variable was consumed and can no longer be used.
    /// This state is used because there is no efficent way of removing variables

Suggestion:

pub enum VarState {
    /// The variable is a temporary variable with the given type.
    TempVar { ty: sierra::ids::ConcreteTypeId },
    /// The variable is deferred with the given DeferredVariableInfo.
    Deferred { info: DeferredVariableInfo },
    /// The variable is a local variable.
    LocalVar,
    /// The variable is of size zero.
    ZeroSizedVar,
    /// The variable was consumed and can no longer be used.
    /// This state is used because there is no efficent way of removing variables

@ilyalesokhin-starkware ilyalesokhin-starkware force-pushed the ilya/zero_sized_var branch 2 times, most recently from 592c6f3 to 0fe1b9d Compare October 23, 2023 12:14
@ilyalesokhin-starkware ilyalesokhin-starkware changed the title Add ZeroSizedVar. Avoid storing zero sized objects on the stack. Oct 23, 2023
Copy link
Contributor Author

@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: 0 of 62 files reviewed, 1 unresolved discussion (waiting on @orizi)


crates/cairo-lang-sierra-generator/src/store_variables/state.rs line 50 at r1 (raw file):

    ZeroSizedVar,
    /// The variable was consumed and can no longer be used.
    /// This state is used because there is no efficent way of removing variables

Done.

Copy link
Collaborator

@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.

Reviewed 33 of 62 files at r2, all commit messages.
Reviewable status: 33 of 62 files reviewed, 1 unresolved discussion (waiting on @ilyalesokhin-starkware)


crates/cairo-lang-sierra/src/extensions/lib_func.rs line 387 at r3 (raw file):

    SimpleDerefs,
    // The output is a of size 0.
    ZeroSized,

Suggestion:

    SameAsParam { param_idx: usize },
    /// The output value is a part of one of the parameters.
    /// For example, it may be the first element of a struct.
    ///
    /// Information, such as whether the parameter was a temporary or local variable, will be
    /// copied to the output variable.
    PartialParam { param_idx: usize },
    /// The output was allocated as a temporary variable and it is at the top of the stack
    /// (contiguously).
    NewTempVar {
    Deferred(DeferredOutputKind),
    /// All the output cells are of the form `[ap/fp + const]`. For example, `([ap + 1], [fp])`.
    SimpleDerefs,
    /// The output is a of size 0.
    ZeroSized,

Copy link
Collaborator

@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.

Reviewed 27 of 62 files at r2, 2 of 2 files at r3.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @ilyalesokhin-starkware)

Copy link
Contributor Author

@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: 60 of 62 files reviewed, 1 unresolved discussion (waiting on @orizi)


crates/cairo-lang-sierra/src/extensions/lib_func.rs line 387 at r3 (raw file):

    SimpleDerefs,
    // The output is a of size 0.
    ZeroSized,

Done.

Copy link
Collaborator

@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.

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

@ilyalesokhin-starkware ilyalesokhin-starkware added this pull request to the merge queue Oct 25, 2023
Merged via the queue into sierra-minor-update with commit 36aaa56 Oct 25, 2023
@orizi orizi deleted the ilya/zero_sized_var branch November 6, 2023 07:02
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