Parallelize Lagrange constraints evaluation #317
Conversation
There was a problem hiding this comment.
Great job! Thank you! I left some comments and improvement suggestions inline.
Overall, I'm not sure I like creating a separate constraint evaluator specifically for LogUp-GKR, but I think once the inline comments are addressed, we can merge this structure to see how it impacts the overall proof generation performance. Then, we can think of a way to refactor it.
| fn evaluate_fragment_full<T: TraceLde<E>>( | ||
| &self, | ||
| trace: &T, | ||
| domain: &StarkDomain<A::BaseField>, | ||
| fragment: &mut EvaluationTableFragment<E>, | ||
| trans_constraints_divisors: &LagrangeKernelTransitionConstraintsDivisor<E>, | ||
| boundary_divisors_inv: &[E], | ||
| s_col_constraint_divisor: &[E::BaseField], | ||
| ) { |
There was a problem hiding this comment.
Related to the above 2 comments, I'm wondering if it would be better to move trans_constraints_divisors, boundary_divisors_inv, and s_col_constraint_divisor into self.logup_gkr_constraints_evaluator. This way, we won't need to pass them as parameters here and it would also encapsulate LogUp-GKR-related code better.
There was a problem hiding this comment.
I agree. In the current implementation, it would make sense to have LogUpGkrConstraintsDivisors be part of
of LogUpGkrConstraintEvaluator. The only issue with making this work is that requires a StarkDomain for its creation and I couldn't come up with a clean way to make it work.
There was a problem hiding this comment.
This is because when we create a new evaluator, we don't pass the domain into the constructor, right? I think we can probably refactor this. Let's create an issue for this.
Al-Kindi-0
force-pushed
the
al-parallel-lagrange-constraint-eval-2
branch
from
d48e5a4 to
dc4d876
Compare
Builds on #315 and supersedes #316 .
The PR proposes a new constraint evaluator that is specific to when LogUp-GKR is enabled.
This has also the advantage of closing #306 .