Access to nonconstant fixed columns. #2368
Conversation
executor/src/witgen/jit/compiler.rs
Outdated
| id: column, | ||
| ptype: PolynomialType::Constant, | ||
| }; | ||
| // TODO which size? |
There was a problem hiding this comment.
Does it always work to use the largest one?
There was a problem hiding this comment.
I think it should be fine, if some outer code still decides when to down-size and handles the corner cases of the last row. We do most of the computation assuming we'll pick the largest size, end then downscale and full up the remaining rows.
There was a problem hiding this comment.
But that's not the issue - the question is: Can we pick the largest size and assume it's the same value in the other sizes?
There was a problem hiding this comment.
That's the assumption we currently go with already ;) We could assert that as well.
executor/src/witgen/jit/compiler.rs
Outdated
| id: column, | ||
| ptype: PolynomialType::Constant, | ||
| }; | ||
| // TODO which size? |
There was a problem hiding this comment.
I think it should be fine, if some outer code still decides when to down-size and handles the corner cases of the last row. We do most of the computation assuming we'll pick the largest size, end then downscale and full up the remaining rows.
executor/src/witgen/jit/variable.rs
Outdated
| @@ -16,6 +16,8 @@ pub enum Variable { | |||
| /// An input or output value of a machine call on a certain | |||
| /// identity on a certain row offset. | |||
| MachineCallParam(MachineCallVariable), | |||
| /// A fixed column cell. | |||
| FixedColumn(Cell), | |||
There was a problem hiding this comment.
| FixedColumn(Cell), | |
| FixedCell(Cell), |
No? And also, we should rename Cell -> WitnessCell.
| @@ -456,11 +459,27 @@ impl<'a, T: FieldElement, FixedEval: FixedEvaluator<T>> WitgenInference<'a, T, F | |||
| true | |||
| } | |||
|
|
|||
| /// Record a variable as known. Return true if it was not known before. | |||
| fn record_known(&mut self, variable: Variable) -> bool { | |||
| // We do not record fixed columns as known. | |||
There was a problem hiding this comment.
A consequence of this is that it is not returned by known_variables(), and a consequence of that is that we'll never branch on fixed column values. I don't see where this would be useful, but it's not the most consistent behavior.
There was a problem hiding this comment.
Would it hurt to initialize the known values with all fixed cells?
There was a problem hiding this comment.
The problem is that we don't know which row offsets we will operate on
There was a problem hiding this comment.
Oh and there is #2369 to fix this
| variable | ||
| .try_to_witness_poly_id() | ||
| .try_to_poly_id() |
There was a problem hiding this comment.
Nice, so we do get range constraints on fixed columns too.
…0e956786ecbb89ecd302c089de815bc8ec8b356' into access_to_nonconstant_fixed_columns
chriseth
dismissed
georgwiese’s stale review
The merge-base changed after approval.
chriseth
dismissed
georgwiese’s stale review
The merge-base changed after approval.
Some machines need non-constant fixed column values even outside of fixed lookups. An example for that is the
STEPcolumn in a riscv VM.This PR introduces variables for those columns and implements access in the compiled code.