-
Notifications
You must be signed in to change notification settings - Fork 1
perf: base-field constraint evaluation for F×E accumulation #510
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -804,6 +804,14 @@ pub struct AirWithBuses< | |
| /// Maximum number of bus elements across all interactions. | ||
| /// Used to compute the correct number of alpha powers. | ||
| max_bus_elements: usize, | ||
| /// Number of table-specific (base-field) transition constraints. | ||
| /// LogUp constraints are extension-field and come after these. | ||
| num_base_constraints: usize, | ||
| /// Optional monolithic evaluator for all base-field constraints. | ||
| /// When set, evaluates ALL base constraints in ONE function call instead of N vtable calls. | ||
| #[allow(clippy::type_complexity)] | ||
| monolithic_eval_fn: | ||
| Option<Box<dyn Fn(&crate::table::TableView<F, E>, &mut [FieldElement<F>]) + Send + Sync>>, | ||
| } | ||
|
|
||
| impl< | ||
|
|
@@ -832,6 +840,7 @@ impl< | |
| mut transition_constraints: Vec<Box<dyn TransitionConstraint<F, E>>>, | ||
| ) -> Self { | ||
| let num_interactions = auxiliary_trace_build_data.interactions.len(); | ||
| let num_base_constraints = transition_constraints.len(); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [Medium] Ordering invariant is implicit and unguarded
If a refactor ever inserts LogUp constraints before the table-specific ones (e.g. a prepend instead of an append), Consider adding a debug assertion after LogUp constraints are pushed — verifying that every constraint in |
||
|
|
||
| // Split interactions: committed pairs get term columns, last 1-2 are absorbed | ||
| let (num_committed_pairs, absorbed_count) = split_interactions(num_interactions); | ||
|
|
@@ -896,6 +905,8 @@ impl< | |
| num_precomputed_cols: None, | ||
| name: None, | ||
| max_bus_elements, | ||
| num_base_constraints, | ||
| monolithic_eval_fn: None, | ||
| } | ||
| } | ||
|
|
||
|
|
@@ -933,6 +944,21 @@ impl< | |
| self.name = Some(name.to_string()); | ||
| self | ||
| } | ||
|
|
||
| /// Set a monolithic evaluator for all base-field constraints. | ||
| /// | ||
| /// The closure evaluates ALL base-field constraints in a single function call, | ||
| /// avoiding N vtable calls through `Box<dyn TransitionConstraint>::evaluate_prover()`. | ||
| /// The compiler can inline and optimize across all constraint expressions. | ||
| /// | ||
| /// LogUp constraints (extension-field) are still dispatched individually. | ||
| pub fn with_monolithic_eval<CB>(mut self, f: CB) -> Self | ||
| where | ||
| CB: Fn(&crate::table::TableView<F, E>, &mut [FieldElement<F>]) + Send + Sync + 'static, | ||
| { | ||
| self.monolithic_eval_fn = Some(Box::new(f)); | ||
| self | ||
| } | ||
| } | ||
|
|
||
| impl<F, E, B, PI> crate::traits::AIR for AirWithBuses<F, E, B, PI> | ||
|
|
@@ -990,6 +1016,26 @@ where | |
| &self.context | ||
| } | ||
|
|
||
| fn num_base_transition_constraints(&self) -> usize { | ||
| // All table-specific constraints are base-field. | ||
| // LogUp constraints (batched term + accumulated) are extension-field | ||
| // and are appended after the table-specific ones. | ||
| self.num_base_constraints | ||
| } | ||
|
|
||
| fn eval_all_base_constraints( | ||
| &self, | ||
| step: &crate::table::TableView<Self::Field, Self::FieldExtension>, | ||
| evals: &mut [FieldElement<Self::Field>], | ||
| ) -> bool { | ||
| if let Some(ref f) = self.monolithic_eval_fn { | ||
| f(step, evals); | ||
| true | ||
| } else { | ||
| false | ||
| } | ||
| } | ||
|
|
||
| fn transition_constraints( | ||
| &self, | ||
| ) -> &Vec<Box<dyn TransitionConstraint<Self::Field, Self::FieldExtension>>> { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -282,6 +282,64 @@ pub trait AIR: Send + Sync { | |
| self.context().num_transition_constraints | ||
| } | ||
|
|
||
| /// Number of transition constraints that produce base-field evaluations. | ||
| /// | ||
| /// Constraints `0..num_base` write to the base-field buffer via `evaluate_prover`. | ||
| /// Constraints `num_base..num_total` write to the extension buffer. | ||
| /// Default: 0 (all E×E, backward compatible). | ||
| fn num_base_transition_constraints(&self) -> usize { | ||
| 0 | ||
| } | ||
|
|
||
| /// Evaluate all transition constraints into split base/extension buffers. | ||
| /// | ||
| /// Base-field constraints write to `base_evaluations` (length = `num_base_transition_constraints()`). | ||
| /// Extension-field constraints write to `ext_evaluations` (length = `num_transition_constraints()`). | ||
| /// | ||
| /// When a monolithic evaluator is available (`eval_all_base_constraints` returns true), | ||
| /// all base-field constraints are evaluated in ONE function call instead of N vtable calls. | ||
| /// LogUp constraints (extension-field) always use per-constraint dispatch. | ||
| fn compute_transition_prover( | ||
| &self, | ||
| evaluation_context: &TransitionEvaluationContext<Self::Field, Self::FieldExtension>, | ||
| base_evaluations: &mut [FieldElement<Self::Field>], | ||
| ext_evaluations: &mut [FieldElement<Self::FieldExtension>], | ||
| ) { | ||
| let num_base = base_evaluations.len(); | ||
| for e in base_evaluations.iter_mut() { | ||
| *e = FieldElement::zero(); | ||
| } | ||
| for e in ext_evaluations[num_base..].iter_mut() { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [High] Silent correctness gap: The docstring says If a constraint's Consider either zeroing the full |
||
| *e = FieldElement::zero(); | ||
| } | ||
|
|
||
| // Try monolithic evaluation for base constraints (ONE call for all) | ||
| let step = match evaluation_context { | ||
| TransitionEvaluationContext::Prover { frame, .. } => Some(frame.get_evaluation_step(0)), | ||
| _ => None, | ||
| }; | ||
|
|
||
| let used_monolithic = if let Some(step) = step { | ||
| self.eval_all_base_constraints(step, base_evaluations) | ||
| } else { | ||
| false | ||
| }; | ||
|
|
||
| if used_monolithic { | ||
| // Monolithic handled base constraints. Only dispatch LogUp (extension): | ||
| for c in self.transition_constraints().iter() { | ||
| if c.constraint_idx() >= num_base { | ||
| c.evaluate_prover(evaluation_context, base_evaluations, ext_evaluations); | ||
| } | ||
| } | ||
| } else { | ||
| // Fallback: per-constraint dispatch for ALL constraints | ||
| self.transition_constraints().iter().for_each(|c| { | ||
| c.evaluate_prover(evaluation_context, base_evaluations, ext_evaluations) | ||
| }); | ||
| } | ||
| } | ||
|
|
||
| fn get_periodic_column_values(&self) -> Vec<Vec<FieldElement<Self::Field>>> { | ||
| vec![] | ||
| } | ||
|
|
@@ -306,6 +364,21 @@ pub trait AIR: Send + Sync { | |
| result | ||
| } | ||
|
|
||
| /// Evaluate ALL base-field transition constraints in a single function call. | ||
| /// | ||
| /// Returns `true` if this AIR has a monolithic evaluator (overrides individual dispatch). | ||
| /// When `true`, the evaluator calls this instead of per-constraint `evaluate_prover()`. | ||
| /// LogUp constraints are still dispatched individually (they're in extension field). | ||
| /// | ||
| /// Default: `false` (no monolithic evaluator, use per-constraint dispatch). | ||
| fn eval_all_base_constraints( | ||
| &self, | ||
| _step: &crate::table::TableView<Self::Field, Self::FieldExtension>, | ||
| _evals: &mut [FieldElement<Self::Field>], | ||
| ) -> bool { | ||
| false | ||
| } | ||
|
|
||
| fn transition_constraints( | ||
| &self, | ||
| ) -> &Vec<Box<dyn TransitionConstraint<Self::Field, Self::FieldExtension>>>; | ||
|
|
||
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.
[Medium] ~90 lines of logic duplicated between parallel and non-parallel paths
The
num_base > 0branch (bothis_uniformand non-uniform sub-cases) is copy-pasted nearly verbatim in the#[cfg(not(feature = "parallel"))]path below. This PR doubled the duplication surface compared to the original. Any future correctness fix will need to be applied in two places.Consider extracting the inner accumulation into a shared free function, e.g.:
Both
map_initand the sequential loop can then call it with no duplication.