-
Notifications
You must be signed in to change notification settings - Fork 703
feat: use local_into_box #9046
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
base: gilad/12-14-feat_add_unbox_for_symmetry_with_intobox_
Are you sure you want to change the base?
feat: use local_into_box #9046
Conversation
|
Warning This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
This stack of pull requests is managed by Graphite. Learn more about stacking. |
9d7bb23 to
d8fe4c1
Compare
cf3a16f to
cc4db7d
Compare
orizi
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@orizi reviewed 2 of 10 files at r1, all commit messages.
Reviewable status: 2 of 10 files reviewed, 2 unresolved discussions (waiting on @eytan-starkware and @giladchase)
crates/cairo-lang-filesystem/src/flag.rs line 26 at r1 (raw file):
/// /// Default is true. LocalIntoBoxOptimization(bool),
use "sierra_future" flag - should exist after you rebase.
so revert the file.
crates/cairo-lang-sierra-generator/src/local_variables.rs line 41 at r1 (raw file):
pub ap_tracking_configuration: ApTrackingConfiguration, /// Variables that are known to be non-AP-based (expanded to include all aliases). pub non_ap_based_variables: UnorderedHashSet<VariableId>,
why isn't this just included in the set of local variables?
orizi
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 2 of 10 files reviewed, 3 unresolved discussions (waiting on @eytan-starkware and @giladchase)
crates/cairo-lang-sierra-generator/src/function_generator_test_data/boxing line 110 at r1 (raw file):
fn foo() -> Box<MyStruct> { let x = create_struct(); revoke_ap();
why is the revoke required? it should be store_local anyway because it is large - no?
d8fe4c1 to
3783121
Compare
cc4db7d to
9b151ef
Compare
orizi
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@orizi reviewed all commit messages.
Reviewable status: 0 of 10 files reviewed, 3 unresolved discussions (waiting on @eytan-starkware and @giladchase)
3783121 to
e718bfd
Compare
9b151ef to
0316145
Compare
e718bfd to
88dfb36
Compare
0316145 to
eca350a
Compare
88dfb36 to
a916e5c
Compare
eca350a to
5678893
Compare
- usage subject to dedicated db-flag - after tracking local variables in `analyze_ap_changes`, into_box sierra-gen conditionally emits either `into_box` or `local_into_box` if the input is small enough to be worth it.
5678893 to
99620ed
Compare
a916e5c to
39ee7ec
Compare

analyze_ap_changes, into_box sierra-gen conditionally emitseither
into_boxorlocal_into_boxif the input is small enough to be worth it.