Remove constant intermediates #2163
Conversation
pilopt/src/lib.rs
Outdated
| @@ -465,6 +466,52 @@ fn remove_constant_witness_columns<T: FieldElement>(pil_file: &mut Analyzed<T>) | |||
| substitute_polynomial_references(pil_file, constant_polys); | |||
| } | |||
|
|
|||
| /// Identifies intermediate columns that are constrained to a single value, replaces every | |||
| /// reference to this column by the value and deletes the column. | |||
| /// This assumes that intermediate columns are defined before they are used in source order. | |||
There was a problem hiding this comment.
I don't think we can assume that, sorry.
There was a problem hiding this comment.
In that case we could run it until we reach a fixed point? Or build a dependency graph first? But that goes further against the idea of having simple steps.
pilopt/src/lib.rs
Outdated
| let ((name, poly_id), mut definition) = symbol | ||
| .array_elements() | ||
| .zip_eq(definitions.iter().cloned()) | ||
| .next() |
There was a problem hiding this comment.
I think we should either properly iterate or check that it is not an array.
There was a problem hiding this comment.
It's checked line 475. I can add an assert also here for clarity?
pilopt/src/lib.rs
Outdated
| // replace constant intermediate columns by their value | ||
| definition.post_visit_expressions_mut(&mut |e| { | ||
| if let AlgebraicExpression::Reference(AlgebraicReference { poly_id, .. }) = e { | ||
| if let Some((_, value)) = values.get(poly_id) { | ||
| *e = AlgebraicExpression::Number(T::from(value.clone())); | ||
| } | ||
| } | ||
| }); | ||
|
|
||
| // simplify the expression | ||
| let definition = simplify_expression(definition.clone()); |
There was a problem hiding this comment.
Should we really iteratively replace the values right away or rather keep the steps simple and run them multiple times? Do we have that many intermediate columns that reference other intermediate columns?
There was a problem hiding this comment.
The use case here is that when we build the bus accumulator updates:
col intermediate_fingerprint_0 = 0;
col intermediate_fingerprint_1 = 0;
col intermediate_fingerprint_0_1 = std::prelude::challenge(0, 1) * main::intermediate_fingerprint_0 + 11 * std::prelude::challenge(0, 2) * main::intermediate_fingerprint_1 + 0;
col intermediate_fingerprint_1_1 = std::prelude::challenge(0, 2) * main::intermediate_fingerprint_0 + std::prelude::challenge(0, 1) * main::intermediate_fingerprint_1 + 0;All of this should be removed as it's all 0.
We have a lot of intermediates which depend on each other (depending on the number of columns we're sending/receiving), but they typically don't end up being constant. This only happens in some special cases like above.
I think running it twice without memory would work, but writing the case with memory is not that much code. Up to you :)
|
Is this waiting for changes or reviews? |
Co-authored-by: chriseth <[email protected]>
Co-authored-by: chriseth <[email protected]>
See added test
This propagates knowledge about constant intermediates forward in source order. We could run this backwards as well but I think in practice intermediates are defined before usage.