-
Notifications
You must be signed in to change notification settings - Fork 698
(optimization): Reboxing now also works on whole var reboxing #8994
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: eytan_graphite/_optimization_reboxing_also_applied_on_box_of_snapshots
Are you sure you want to change the base?
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. |
65c6092 to
ffde167
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 2 files reviewed, 1 unresolved discussion (waiting on @eytan-starkware, @giladchase, and @ilyalesokhin-starkware)
crates/cairo-lang-lowering/src/optimizations/reboxing.rs line 268 at r1 (raw file):
/// Replaces all usages of old_var with new_var throughout the lowered function. fn replace_variable_usages(lowered: &mut Lowered<'_>, old_var: VariableId, new_var: VariableId) {
can't you use VarRemapper to do all candidate replacements in one go?
ffde167 to
e3b6a1f
Compare
eytan-starkware
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: 0 of 2 files reviewed, 1 unresolved discussion (waiting on @giladchase, @ilyalesokhin-starkware, and @orizi)
crates/cairo-lang-lowering/src/optimizations/reboxing.rs line 268 at r1 (raw file):
Previously, orizi wrote…
can't you use VarRemapper to do all candidate replacements in one go?
Yes, much better
e3b6a1f to
0496ffe
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 2 files at r2, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @eytan-starkware, @giladchase, and @ilyalesokhin-starkware)
crates/cairo-lang-lowering/src/optimizations/reboxing.rs line 205 at r2 (raw file):
// We sort the candidates such that removal of statements can be done in reverse order. stmts_to_remove.sort_by_key(|(block_id, stmt_idx)| (block_id.0, *stmt_idx));
was there any order to the original candidates? or these are basically random?
eytan-starkware
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: all files reviewed, 1 unresolved discussion (waiting on @giladchase, @ilyalesokhin-starkware, and @orizi)
crates/cairo-lang-lowering/src/optimizations/reboxing.rs line 205 at r2 (raw file):
Previously, orizi wrote…
was there any order to the original candidates? or these are basically random?
I think it is safe to assume they were ordered correctly ahead of time, so reversing directly should be safe, but I dont want to add that assumption. I can use unstable sort if performance is an issue, but I expect this to be a small amount of statements to remove
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: all files reviewed, 1 unresolved discussion (waiting on @eytan-starkware, @giladchase, and @ilyalesokhin-starkware)
crates/cairo-lang-lowering/src/optimizations/reboxing.rs line 205 at r2 (raw file):
Previously, eytan-starkware wrote…
I think it is safe to assume they were ordered correctly ahead of time, so reversing directly should be safe, but I dont want to add that assumption. I can use unstable sort if performance is an issue, but I expect this to be a small amount of statements to remove
do stable - and doc that it should be sorted already.
i just don't want it to overly kill large auto-generated codes.
ef335a5 to
630ce29
Compare
0496ffe to
4370c99
Compare
eytan-starkware
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: all files reviewed, 1 unresolved discussion (waiting on @giladchase, @ilyalesokhin-starkware, and @orizi)
crates/cairo-lang-lowering/src/optimizations/reboxing.rs line 205 at r2 (raw file):
Previously, orizi wrote…
do stable - and doc that it should be sorted already.
i just don't want it to overly kill large auto-generated codes.
Done.
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 1 of 1 files at r3, all commit messages.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on @giladchase and @ilyalesokhin-starkware)
4370c99 to
f5eada9
Compare
630ce29 to
1dd5922
Compare
f5eada9 to
7e430d6
Compare
1dd5922 to
2e496a0
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 1 of 1 files at r4, all commit messages.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on @giladchase and @ilyalesokhin-starkware)

Enhances the reboxing optimization to support whole variable reboxing, eliminating unnecessary box/unbox operations.
What changed?
apply_reboxing_candidatefunction to support for theReboxingValue::Unboxedcase, which represents whole variable reboxingreplace_variable_usagesfunction that replaces all usages of the reboxed with the original box variable throughout the lowered functionreplace_into_box_callfunction