Simplify expressions.

[Alon-Ti]

No description provided.

[reviewable-StarkWare]

This change is 

[Alon-Ti]

• Added add_intermediate and add_secure_intermediate to eval API. #919
• Add safe simplify for expressions that compares random assignments before and after. #918
• Added test for expression simplifier. #908
• Added eval on exprs. #904
• Separated Base and Extension field expressions. #903
• Added From<G> in add_constraint. #901
• Simplify expressions. #894 👈 (View in Graphite)
• Exported shared code to temp variables in ExprEvaluator. #893
• Added total_ and claimed_sums as formal variables in ExprEvaluator. #892
• State machine constraint string. #889
• Expr formatting. #888
• Formal Expr logup variables. #887
• Initialize logups automatically when first used. #879 : 1 other dependent PR (#883 )
• Abstracted eval.write_fracs to eval.add_to_relations. #876
• Added LogupAtRow functionality to EvalAtRow. #875
• dev

This stack of pull requests is managed by Graphite. Learn more about stacking.

[github-actions]

⚠️ Performance Alert ⚠️

Possible performance regression was detected for benchmark.
Benchmark result of this commit is worse than the previous benchmark result exceeding threshold 2.

Benchmark suite	Current: d53fec3	Previous: cd8b37b	Ratio
iffts/simd ifft/22	13528130 ns/iter (± 132749)	6306399 ns/iter (± 210024)	2.15
iffts/simd ifft/25	131797226 ns/iter (± 849972)	65891527 ns/iter (± 1735211)	2.00
iffts/simd ifft/28	1292042611 ns/iter (± 39669891)	643771030 ns/iter (± 19147376)	2.01
merkle throughput/simd merkle	30022184 ns/iter (± 492981)	13712527 ns/iter (± 579195)	2.19

This comment was automatically generated by workflow using github-action-benchmark.

CC: @shaharsamocha7

[codecov-commenter]

Codecov Report

Attention: Patch coverage is 68.57143% with 22 lines in your changes missing coverage. Please review.

Project coverage is 91.56%. Comparing base (4d64300) to head (e4c2d1d).

Files with missing lines	Patch %	Lines
crates/prover/src/constraint_framework/expr.rs	68.57%	22 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##              dev     #894      +/-   ##
==========================================
- Coverage   91.69%   91.56%   -0.14%     
==========================================
  Files          93       93              
  Lines       13164    13228      +64     
  Branches    13164    13228      +64     
==========================================
+ Hits        12071    12112      +41     
- Misses        980     1003      +23     
  Partials      113      113              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[ohad-starkware]

Reviewed all commit messages.
Reviewable status: 0 of 2 files reviewed, 2 unresolved discussions (waiting on @Alon-Ti and @shaharsamocha7)

---

crates/prover/src/constraint_framework/expr.rs line 222 at r1 (raw file):

            }
        }
        Expr::Mul(a, b) => {

are you covering 1 * 1 => const(1)?

---

crates/prover/src/constraint_framework/expr.rs line 226 at r1 (raw file):

            let b = simplify(*b);
            if let (Expr::Const(a), Expr::Const(b)) = (a.clone(), b.clone()) {
                Expr::Const(a - b)

Suggestion:

a * b

[Alon-Ti]

Reviewable status: 0 of 2 files reviewed, 2 unresolved discussions (waiting on @ohad-starkware and @shaharsamocha7)

---

crates/prover/src/constraint_framework/expr.rs line 222 at r1 (raw file):

Previously, ohad-starkware (Ohad) wrote…

are you covering 1 * 1 => const(1)?

I think so, it would be caught in the first if let, no?

---

crates/prover/src/constraint_framework/expr.rs line 226 at r1 (raw file):

            let b = simplify(*b);
            if let (Expr::Const(a), Expr::Const(b)) = (a.clone(), b.clone()) {
                Expr::Const(a - b)

Done.

[ohad-starkware]

Reviewable status: 0 of 2 files reviewed, all discussions resolved (waiting on @shaharsamocha7)

---

crates/prover/src/constraint_framework/expr.rs line 222 at r1 (raw file):

Previously, Alon-Ti wrote…

I think so, it would be caught in the first if let, no?

right

---

crates/prover/src/constraint_framework/expr.rs line 356 at r2 (raw file):

            .temp_vars
            .iter()
            .map(|(name, expr)| format!("let {} = {};", name, simplify(expr.clone()).format_expr()))

can you find a way to reduce the syntax?
maybe expr.simplify_and_format()
non blocking

Code quote:

 simplify(expr.clone()).format_expr()))

[ohad-starkware]

Reviewable status: 0 of 2 files reviewed, all discussions resolved (waiting on @shaharsamocha7)

[shaharsamocha7]

Reviewed 1 of 2 files at r5, all commit messages.
Reviewable status: 1 of 2 files reviewed, all discussions resolved (waiting on @ohad-starkware)

[shaharsamocha7]

Reviewed 1 of 2 files at r5.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @Alon-Ti)

---

crates/prover/src/constraint_framework/expr.rs line 196 at r5 (raw file):

}

pub fn simplify(expr: Expr) -> Expr {

I think this function should have more tests

Code quote:

pub fn simplify(expr: Expr) -> Expr {

---

crates/prover/src/constraint_framework/expr.rs line 217 at r5 (raw file):

            } else {
                a + b
            }

I think match statement is more readable
Consider to change

Code quote:

            if let (Expr::Const(a), Expr::Const(b)) = (a.clone(), b.clone()) {
                Expr::Const(a + b)
            } else if a == Expr::zero() {
                b
            } else if b == Expr::zero() {
                a
            } else if let Expr::Neg(a) = a {
                if let Expr::Neg(b) = b {
                    -(*a + *b)
                } else {
                    b - *a
                }
            } else if let Expr::Neg(b) = b {
                a - *b
            } else {
                a + b
            }

---

crates/prover/src/constraint_framework/expr.rs line 264 at r5 (raw file):

            let a = simplify(*a);
            match a {
                Expr::Neg(b) => *b,

Is this Neg(Neg(a))? can you comment on that?

Code quote:

Expr::Neg(b) => *b,

---

crates/prover/src/constraint_framework/expr.rs line 264 at r5 (raw file):

            let a = simplify(*a);
            match a {
                Expr::Neg(b) => *b,

What happens if a is a secure column?

Code quote:

            match a {
                Expr::Neg(b) => *b,

---

crates/prover/src/constraint_framework/expr.rs line 267 at r5 (raw file):

                Expr::Const(c) => Expr::Const(-c),
                Expr::Sub(a, b) => Expr::Sub(b, a),
                _ => -a,

I'm a bit worried of this
Will it always be correct?

Code quote:

_ => -a,

[Alon-Ti]

Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @shaharsamocha7)

---

crates/prover/src/constraint_framework/expr.rs line 196 at r5 (raw file):

Previously, shaharsamocha7 wrote…

I think this function should have more tests

Added TODO

---

crates/prover/src/constraint_framework/expr.rs line 264 at r5 (raw file):

Previously, shaharsamocha7 wrote…

Is this Neg(Neg(a))? can you comment on that?

Done.

---

crates/prover/src/constraint_framework/expr.rs line 264 at r5 (raw file):

Previously, shaharsamocha7 wrote…

What happens if a is a secure column?

Neg(SecureColumn(...)). I can simplify Neg(SecureColumn(Neg, Neg, Neg, Neg)) etc, but I don't think we'll have many of these.

---

crates/prover/src/constraint_framework/expr.rs line 267 at r5 (raw file):

Previously, shaharsamocha7 wrote…

I'm a bit worried of this
Will it always be correct?

This just reconstructs Neg(a) if it can't be simplified. Should I change it to explicitly Expr::Neg(Box::new(a))?

[shaharsamocha7]

Reviewed all commit messages.
Reviewable status: 1 of 2 files reviewed, 3 unresolved discussions (waiting on @Alon-Ti)

---

crates/prover/src/constraint_framework/expr.rs line 267 at r5 (raw file):

Previously, Alon-Ti wrote…

This just reconstructs Neg(a) if it can't be simplified. Should I change it to explicitly Expr::Neg(Box::new(a))?

I think it is better yes

---

crates/prover/src/constraint_framework/expr.rs line 210 at r6 (raw file):

                (Expr::Const(M31(0)), _) => b, // 0 + b = b
                (_, Expr::Const(ZERO)) => a,   // a + 0 = a
                // (-a + -b) = -(a + b)

Suggestion:

                (Expr::Const(ZERO), _) => b, // 0 + b = b
                (_, Expr::Const(ZERO)) => a,   // a + 0 = a
                // (-a + -b) = -(a + b)

---

crates/prover/src/constraint_framework/expr.rs line 211 at r6 (raw file):

                (_, Expr::Const(ZERO)) => a,   // a + 0 = a
                // (-a + -b) = -(a + b)
                (Expr::Neg(minus_a), Expr::Neg(minus_b)) => -(*minus_a + *minus_b),

I think you should open Exprs as a, b and not call them minus

Suggestion:

(Expr::Neg(a), Expr::Neg(b)) => -(*a + *b),

[Alon-Ti]

Reviewable status: 1 of 2 files reviewed, 1 unresolved discussion (waiting on @shaharsamocha7)

---

crates/prover/src/constraint_framework/expr.rs line 210 at r6 (raw file):

                (Expr::Const(M31(0)), _) => b, // 0 + b = b
                (_, Expr::Const(ZERO)) => a,   // a + 0 = a
                // (-a + -b) = -(a + b)

Done.

[shaharsamocha7]

Reviewed 1 of 1 files at r7, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @Alon-Ti)

[Alon-Ti]

Merge activity

• Nov 28, 10:24 AM EST: A user merged this pull request with Graphite.