-
Notifications
You must be signed in to change notification settings - Fork 655
(optimization): felt252_div constant propogation support #8604
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
Conversation
This stack of pull requests is managed by Graphite. Learn more about stacking. |
4b9630b to
bc86fb3
Compare
4982bd9 to
ad3f92b
Compare
TomerStarkware
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.
@TomerStarkware reviewed 3 of 3 files at r1, all commit messages.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on @ilyalesokhin-starkware and @orizi)
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, 6 unresolved discussions (waiting on @ilyalesokhin-starkware)
crates/cairo-lang-lowering/src/optimizations/const_folding.rs line 556 at r1 (raw file):
// There is no need to check if divisor is 0 and throw error, because casting to NonZero // will add a panic that will always be thrown. TODO(eytan-starkware): Make // sure that NonZero on a const zero leads to a compile error.
It does.
shorten comment.
Suggestion:
// Note that divisor is never 0, due to NonZero type always being the divisor.crates/cairo-lang-lowering/src/optimizations/const_folding.rs line 560 at r1 (raw file):
&& rhs.is_one() { // Check if dividing by 1 (returns the original value)
Suggestion:
// Returns the original value when dividing by 1.crates/cairo-lang-lowering/src/optimizations/const_folding.rs line 566 at r1 (raw file):
&& lhs.is_zero() { // Check if 0 is being divided (returns 0)
Suggestion:
// If the value is 0, result is 0 regardless of the divisor.crates/cairo-lang-lowering/src/optimizations/const_folding.rs line 569 at r1 (raw file):
Some(self.propagate_zero_and_get_statement(stmt.outputs[0])) } else if let (Some(lhs), Some(rhs)) = (self.as_int(stmt.inputs[0].var_id), self.as_int(stmt.inputs[1].var_id))
maybe better now when let-chains works - no need for an extra tuple structuring and destructuring.
Suggestion:
} else if let Some(lhs) = self.as_int(stmt.inputs[0].var_id)
&& let Some(rhs)) = self.as_int(stmt.inputs[1].var_id)crates/cairo-lang-lowering/src/optimizations/const_folding.rs line 570 at r1 (raw file):
} else if let (Some(lhs), Some(rhs)) = (self.as_int(stmt.inputs[0].var_id), self.as_int(stmt.inputs[1].var_id)) && !rhs.is_zero()
Suggestion:
} else if let (Some(lhs), Some(rhs)) =
(self.as_int(stmt.inputs[0].var_id), self.as_int(stmt.inputs[1].var_id))crates/cairo-lang-lowering/src/optimizations/const_folding.rs line 578 at r1 (raw file):
let rhs_felt = Felt252::from(rhs); // For non-zero divisor, use field_div; the libfunc should handle zero checks let rhs_nonzero = rhs_felt.try_into().expect("Non-zero divisor");
if you want to do that - you can do it as part of the initial let-chain.
&& let Some(rhs_nonzero) = Felt252::from(rhs).try_into() {
Code quote:
let rhs_nonzero = rhs_felt.try_into().expect("Non-zero divisor");
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, 8 unresolved discussions (waiting on @eytan-starkware and @ilyalesokhin-starkware)
crates/cairo-lang-lowering/src/optimizations/test_data/const_folding line 6514 at r1 (raw file):
//! > module_code use core::felt252;
remove all around - already in prelude.
Code quote:
use core::felt252;
crates/cairo-lang-lowering/src/optimizations/test_data/const_folding line 6578 at r1 (raw file):
Statements: (v1: core::zeroable::NonZero::<core::felt252>) <- NonZero(1) (v2: core::felt252) <- core::felt252_div(v0, v1)
nothing happened.
Code quote:
(v2: core::felt252) <- core::felt252_div(v0, v1)

What changed?
felt252_divlibfunc with three optimization cases: