-
Notifications
You must be signed in to change notification settings - Fork 651
Adding support to felt add mul and sub in constant propogation #8594
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
Adding support to felt add mul and sub in constant propogation #8594
Conversation
This stack of pull requests is managed by Graphite. Learn more about stacking. |
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: 0 of 13 files reviewed, 1 unresolved discussion (waiting on @ilyalesokhin-starkware and @TomerStarkware)
crates/cairo-lang-lowering/src/lower/test_data/closure line 361 at r1 (raw file):
Return() blk2:
add calls to some inline function to keep the test with meaning.
Code quote:
(v1: core::felt252) <- 1
(v2: core::felt252) <- 2
(v3: core::felt252) <- core::felt252_sub(v1, v2)
End:
Match(match core::felt252_is_zero(v3) {
IsZeroResult::Zero => blk1,
IsZeroResult::NonZero(v4) => blk2,
})
blk1:
Statements:
End:
Return()
blk2:
2472499 to
9eb898a
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 13 files reviewed, 1 unresolved discussion (waiting on @ilyalesokhin-starkware, @orizi, and @TomerStarkware)
crates/cairo-lang-lowering/src/lower/test_data/closure line 361 at r1 (raw file):
Previously, orizi wrote…
add calls to some inline function to keep the test with meaning.
Done.
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 13 files reviewed, 1 unresolved discussion (waiting on @ilyalesokhin-starkware, @orizi, and @TomerStarkware)
crates/cairo-lang-lowering/src/lower/test_data/closure line 361 at r1 (raw file):
Previously, eytan-starkware wrote…
Done.
I used a non-inline function to get the generated code be similar to original one (inline functions would generally get constant propagation activated and back to square one)
9eb898a to
884a555
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 6 of 13 files at r1, 5 of 6 files at r2, all commit messages.
Reviewable status: 11 of 13 files reviewed, 5 unresolved discussions (waiting on @ilyalesokhin-starkware and @TomerStarkware)
crates/cairo-lang-lowering/src/optimizations/test_data/const_folding line 59 at r2 (raw file):
//! > module_code const Pm1: felt252 = 0x800000000000011000000000000000000000000000000000000000000000000;
is this PRIME_MINUS_1?
Suggestion:
const PRIME_MINUS_1: felt252 = -1;
crates/cairo-lang-lowering/src/test_data/tests line 178 at r2 (raw file):
x; } let _y = if 1 == 1 {
prevent test degragation.
crates/cairo-lang-utils/src/bigint.rs line 248 at r2 (raw file):
/// The Cairo prime as a BigInt. #[cfg(feature = "std")] pub static CAIRO_PRIME_BIGINT: LazyLock<BigInt> = LazyLock::new(|| {
do in another PR and use everywhere.
also - possibly just direct usage of Felt252 type should be enough.
crates/cairo-lang-lowering/src/optimizations/const_folding.rs line 513 at r2 (raw file):
if let (Some(lhs), Some(rhs)) = (self.as_int(stmt.inputs[0].var_id), self.as_int(stmt.inputs[1].var_id)) {
if any is known to be 0 - just return the other - same as the felt_sub.
Code quote:
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 525 at r2 (raw file):
{ return Some(self.propagate_zero_and_get_statement(stmt.outputs[0])); }
if any is one return the other - same as felt_sub.
884a555 to
64d82ee
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: 7 of 17 files reviewed, 5 unresolved discussions (waiting on @ilyalesokhin-starkware, @orizi, and @TomerStarkware)
crates/cairo-lang-lowering/src/optimizations/const_folding.rs line 513 at r2 (raw file):
Previously, orizi wrote…
if any is known to be 0 - just return the other - same as the
felt_sub.
Done.
crates/cairo-lang-lowering/src/optimizations/const_folding.rs line 525 at r2 (raw file):
Previously, orizi wrote…
if any is
onereturn the other - same asfelt_sub.
Done.
crates/cairo-lang-lowering/src/optimizations/test_data/const_folding line 59 at r2 (raw file):
Previously, orizi wrote…
is this PRIME_MINUS_1?
Done.
crates/cairo-lang-lowering/src/test_data/tests line 178 at r2 (raw file):
Previously, orizi wrote…
prevent test degragation.
Done.
crates/cairo-lang-utils/src/bigint.rs line 248 at r2 (raw file):
Previously, orizi wrote…
do in another PR and use everywhere.
also - possibly just direct usage of Felt252 type should be enough.
Just moved to wrapping values with felt, which will later provide a div operator as well
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 6 of 9 files at r3, all commit messages.
Reviewable status: 13 of 17 files reviewed, 10 unresolved discussions (waiting on @eytan-starkware, @ilyalesokhin-starkware, and @TomerStarkware)
crates/cairo-lang-lowering/src/optimizations/test_data/const_folding line 59 at r2 (raw file):
Previously, eytan-starkware wrote…
Done.
no it isn't.
crates/cairo-lang-utils/src/lib.rs line 34 at r3 (raw file):
pub mod unordered_hash_set; pub use starknet_types_core::felt::Felt as Felt252;
revert.
use directly where-ever needed.
crates/cairo-lang-lowering/src/optimizations/const_folding.rs line 511 at r3 (raw file):
stmt.outputs[0], false, ));
Suggestion:
let value = Felt252::from(lhs - rhs).to_bigint();
return Some(self.propagate_const_and_get_statement(
value,
stmt.outputs[0],
false,
));crates/cairo-lang-lowering/src/optimizations/const_folding.rs line 529 at r3 (raw file):
if let (Some(lhs), Some(rhs)) = (self.as_int(stmt.inputs[0].var_id), self.as_int(stmt.inputs[1].var_id)) {
Suggestion:
} else if id == self.felt_add {
if let Some(lhs) = self.as_int(stmt.inputs[0].var_id)
&& lhs.is_zero()
{
self.var_info.insert(stmt.outputs[0], VarInfo::Var(stmt.inputs[1]));
} else if let Some(rhs) = self.as_int(stmt.inputs[1].var_id)
&& rhs.is_zero()
{
self.var_info.insert(stmt.outputs[0], VarInfo::Var(stmt.inputs[0]));
} 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 529 at r3 (raw file):
if let (Some(lhs), Some(rhs)) = (self.as_int(stmt.inputs[0].var_id), self.as_int(stmt.inputs[1].var_id)) {
Suggestion:
if let Some(lhs) = self.as_int(stmt.inputs[0].var_id)
&& lhs.is_zero()
{
self.var_info.insert(stmt.outputs[0], VarInfo::Var(stmt.inputs[1]));
} else if let Some(rhs) = self.as_int(stmt.inputs[1].var_id)
&& rhs.is_zero()
{
self.var_info.insert(stmt.outputs[0], VarInfo::Var(stmt.inputs[0]));
} 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 535 at r3 (raw file):
stmt.outputs[0], false, ));
Suggestion:
let value = Felt252::from(lhs + rhs).to_bigint();
return Some(self.propagate_const_and_get_statement(
value,
stmt.outputs[0],
false,
));crates/cairo-lang-lowering/src/optimizations/const_folding.rs line 559 at r3 (raw file):
} if let (Some((lhs_val, lhs_nz)), Some((rhs_val, rhs_nz))) = (lhs, rhs) {
Suggestion:
if let Some(rhs) = self.as_int(stmt.inputs[1].var_id)
&& rhs.is_one()
{
self.var_info.insert(stmt.outputs[0], VarInfo::Var(stmt.inputs[0]));
} else if let Some(lhs) = self.as_int(stmt.inputs[0].var_id)
&& lhs.is_one()
{
self.var_info.insert(stmt.outputs[0], VarInfo::Var(stmt.inputs[1]));
} else if let (Some((lhs_val, lhs_nz)), Some((rhs_val, rhs_nz))) = (lhs, rhs) {crates/cairo-lang-lowering/src/optimizations/const_folding.rs line 566 at r3 (raw file):
stmt.outputs[0], nz_ty, ));
Suggestion:
let value = Felt252::from(lhs_val * rhs_val).to_bigint();
let nz_ty = lhs_nz && rhs_nz;
return Some(self.propagate_const_and_get_statement(
value,
stmt.outputs[0],
nz_ty,
));64d82ee to
4b9630b
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: 11 of 18 files reviewed, 10 unresolved discussions (waiting on @ilyalesokhin-starkware, @orizi, and @TomerStarkware)
crates/cairo-lang-lowering/src/optimizations/test_data/const_folding line 59 at r2 (raw file):
Previously, orizi wrote…
no it isn't.
Now in both locations. Note that it will write -1 in the lowering statements because we are using big ints for constants, and not the felt wrap around number. I think that it is fine though.
crates/cairo-lang-utils/src/lib.rs line 34 at r3 (raw file):
Previously, orizi wrote…
revert.
use directly where-ever needed.
Ok
crates/cairo-lang-lowering/src/optimizations/const_folding.rs line 511 at r3 (raw file):
stmt.outputs[0], false, ));
Done.
crates/cairo-lang-lowering/src/optimizations/const_folding.rs line 529 at r3 (raw file):
if let (Some(lhs), Some(rhs)) = (self.as_int(stmt.inputs[0].var_id), self.as_int(stmt.inputs[1].var_id)) {
Done.
crates/cairo-lang-lowering/src/optimizations/const_folding.rs line 529 at r3 (raw file):
if let (Some(lhs), Some(rhs)) = (self.as_int(stmt.inputs[0].var_id), self.as_int(stmt.inputs[1].var_id)) {
Done.
crates/cairo-lang-lowering/src/optimizations/const_folding.rs line 535 at r3 (raw file):
stmt.outputs[0], false, ));
Done.
crates/cairo-lang-lowering/src/optimizations/const_folding.rs line 559 at r3 (raw file):
} if let (Some((lhs_val, lhs_nz)), Some((rhs_val, rhs_nz))) = (lhs, rhs) {
Done.
crates/cairo-lang-lowering/src/optimizations/const_folding.rs line 566 at r3 (raw file):
stmt.outputs[0], nz_ty, ));
Done.
4b9630b to
bc86fb3
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 7 of 7 files at r4, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @ilyalesokhin-starkware and @TomerStarkware)
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:
complete! all files reviewed, all discussions resolved (waiting on @ilyalesokhin-starkware and @TomerStarkware)

Added constant folding for felt252 arithmetic operations (add, sub, mul) to improve compiler optimization.
What changed?
felt252_addandfelt252_muloperationsfelt252_subconstant folding to handle more casesbigint.rsto support felt252 modular arithmetic:CAIRO_PRIME_BIGINTas a static constantfelt252_mod()function to handle modular arithmetic properly