From 78a561eb0737c26b383200e6e37a0a6bd9e6a640 Mon Sep 17 00:00:00 2001 From: chriseth Date: Mon, 1 Jul 2024 18:08:58 +0000 Subject: [PATCH 1/7] Change column map to work efficiently with slices of column IDs. --- .../src/witgen/data_structures/column_map.rs | 27 +++++++++++++++---- executor/src/witgen/mod.rs | 16 ++++++----- executor/src/witgen/processor.rs | 1 + executor/src/witgen/rows.rs | 6 ++++- 4 files changed, 38 insertions(+), 12 deletions(-) diff --git a/executor/src/witgen/data_structures/column_map.rs b/executor/src/witgen/data_structures/column_map.rs index 576082b03..37c139ab2 100644 --- a/executor/src/witgen/data_structures/column_map.rs +++ b/executor/src/witgen/data_structures/column_map.rs @@ -1,6 +1,6 @@ use std::{ marker::PhantomData, - ops::{Index, IndexMut}, + ops::{Index, IndexMut, Range}, }; use powdr_ast::analyzed::{PolyID, PolynomialType}; @@ -30,26 +30,36 @@ pub type FixedColumnMap = ColumnMap; /// A Map indexed by polynomial ID, for a specific polynomial type (e.g. fixed or witness). /// For performance reasons, it uses a Vec internally and assumes that the polynomial IDs /// are contiguous. +/// If the IDs are not contiguous, accessing the columns between the minimal and maximal column ID +/// will not lead to an error. #[derive(Clone)] pub struct ColumnMap { values: Vec, + /// The first column ID in the map. + // min_column_id: u64, + // /// The number of columns in the map. + // column_count: u64, _ptype: PhantomData, } impl ColumnMap { /// Create a new ColumnMap with the given initial value and size. - pub fn new(initial_value: V, size: usize) -> Self { + pub fn new(initial_value: V, column_range: Range) -> Self { + assert_eq!(column_range.start, 0); ColumnMap { - values: vec![initial_value; size], + values: vec![initial_value; column_range.end - column_range.start], _ptype: PhantomData, } } } impl ColumnMap { - pub fn from(values: impl Iterator) -> Self { + pub fn from(column_id_range: Range, values: impl Iterator) -> Self { + assert_eq!(column_id_range.start, 0); + let values: Vec<_> = values.collect(); + assert_eq!(values.len(), column_id_range.end); ColumnMap { - values: values.collect(), + values, _ptype: PhantomData, } } @@ -86,10 +96,12 @@ impl ColumnMap { self.keys().zip(self.values) } + // TODO check pub fn values(&self) -> impl Iterator { self.values.iter() } + // TODO check pub fn values_into_iter(self) -> impl Iterator { self.values.into_iter() } @@ -98,9 +110,14 @@ impl ColumnMap { self.values.iter_mut() } + // TODO remove in the long term pub fn len(&self) -> usize { self.values.len() } + + pub fn column_id_range(&self) -> Range { + 0..self.values.len() + } } impl Default for ColumnMap { diff --git a/executor/src/witgen/mod.rs b/executor/src/witgen/mod.rs index 6d8d72fb5..08702f667 100644 --- a/executor/src/witgen/mod.rs +++ b/executor/src/witgen/mod.rs @@ -296,7 +296,9 @@ impl<'a, T: FieldElement> FixedData<'a, T> { .collect::>(); let witness_cols = - WitnessColumnMap::from(analyzed.committed_polys_in_source_order().iter().flat_map( + WitnessColumnMap::from( + 0..analyzed.commitment_count(), + analyzed.committed_polys_in_source_order().iter().flat_map( |(poly, value)| { poly.array_elements() .map(|(name, poly_id)| { @@ -333,13 +335,15 @@ impl<'a, T: FieldElement> FixedData<'a, T> { ); } - let fixed_cols = - FixedColumnMap::from(fixed_col_values.iter().map(|(n, v)| FixedColumn::new(n, v))); + let fixed_cols = FixedColumnMap::from( + 0..fixed_col_values.len(), + fixed_col_values.iter().map(|(n, v)| FixedColumn::new(n, v)), + ); // The global range constraints are not set yet. let global_range_constraints = GlobalConstraints { - witness_constraints: WitnessColumnMap::new(None, witness_cols.len()), - fixed_constraints: FixedColumnMap::new(None, fixed_cols.len()), + witness_constraints: WitnessColumnMap::new(None, 0..witness_cols.len()), + fixed_constraints: FixedColumnMap::new(None, 0..fixed_cols.len()), }; FixedData { @@ -382,7 +386,7 @@ impl<'a, T: FieldElement> FixedData<'a, T> { } fn witness_map_with(&self, initial_value: V) -> WitnessColumnMap { - WitnessColumnMap::new(initial_value, self.witness_cols.len()) + WitnessColumnMap::new(initial_value, 0..self.witness_cols.len()) } fn column_name(&self, poly_id: &PolyID) -> &str { diff --git a/executor/src/witgen/processor.rs b/executor/src/witgen/processor.rs index 446ad00f9..7d84d75ce 100644 --- a/executor/src/witgen/processor.rs +++ b/executor/src/witgen/processor.rs @@ -96,6 +96,7 @@ impl<'a, 'b, 'c, T: FieldElement, Q: QueryCallback> Processor<'a, 'b, 'c, T, witness_cols: &'c HashSet, ) -> Self { let is_relevant_witness = WitnessColumnMap::from( + fixed_data.witness_cols.column_id_range(), fixed_data .witness_cols .keys() diff --git a/executor/src/witgen/rows.rs b/executor/src/witgen/rows.rs index f30d5e385..865c885c6 100644 --- a/executor/src/witgen/rows.rs +++ b/executor/src/witgen/rows.rs @@ -256,6 +256,7 @@ impl Row { // TODO and we could copy in the external witnesses later on // TODO we should really only have a subset of the columns. let values = WitnessColumnMap::from( + fixed_data.witness_cols.column_id_range(), fixed_data .global_range_constraints() .witness_constraints @@ -321,7 +322,10 @@ impl Row { impl From> for WitnessColumnMap { /// Builds a map from polynomial ID to value. Unknown values are set to zero. fn from(row: Row) -> Self { - WitnessColumnMap::from(row.values.values_into_iter().map(|c| c.unwrap_or_zero())) + WitnessColumnMap::from( + row.values.column_id_range(), + row.values.values_into_iter().map(|c| c.unwrap_or_zero()), + ) } } From aec1e249811f07fe3cffed48180610a9693f1e2a Mon Sep 17 00:00:00 2001 From: chriseth Date: Mon, 1 Jul 2024 20:35:24 +0000 Subject: [PATCH 2/7] enable ranges --- .../src/witgen/data_structures/column_map.rs | 41 ++++++++++--------- executor/src/witgen/global_constraints.rs | 2 +- 2 files changed, 23 insertions(+), 20 deletions(-) diff --git a/executor/src/witgen/data_structures/column_map.rs b/executor/src/witgen/data_structures/column_map.rs index 37c139ab2..dc0789649 100644 --- a/executor/src/witgen/data_structures/column_map.rs +++ b/executor/src/witgen/data_structures/column_map.rs @@ -35,19 +35,17 @@ pub type FixedColumnMap = ColumnMap; #[derive(Clone)] pub struct ColumnMap { values: Vec, - /// The first column ID in the map. - // min_column_id: u64, - // /// The number of columns in the map. - // column_count: u64, + /// The range of column IDs in the vector. + column_id_range: Range, _ptype: PhantomData, } impl ColumnMap { /// Create a new ColumnMap with the given initial value and size. - pub fn new(initial_value: V, column_range: Range) -> Self { - assert_eq!(column_range.start, 0); + pub fn new(initial_value: V, column_id_range: Range) -> Self { ColumnMap { - values: vec![initial_value; column_range.end - column_range.start], + values: vec![initial_value; column_id_range.len()], + column_id_range, _ptype: PhantomData, } } @@ -55,34 +53,38 @@ impl ColumnMap { impl ColumnMap { pub fn from(column_id_range: Range, values: impl Iterator) -> Self { - assert_eq!(column_id_range.start, 0); let values: Vec<_> = values.collect(); - assert_eq!(values.len(), column_id_range.end); + assert_eq!(values.len(), column_id_range.len()); ColumnMap { values, + column_id_range, _ptype: PhantomData, } } /// Creates a ColumnMap from an iterator over PolyIDs and values. - pub fn from_indexed(items: impl Iterator, len: usize) -> Self + pub fn from_indexed( + column_id_range: Range, + items: impl Iterator, + ) -> Self where V: Default, { - let mut values: Vec = (0..len).map(|_| V::default()).collect(); + let mut values: Vec = (0..column_id_range.len()).map(|_| V::default()).collect(); for (poly, value) in items { - values[poly.id as usize] = value; + values[poly.id as usize - column_id_range.start] = value; debug_assert_eq!(poly.ptype, T::P_TYPE); } ColumnMap { values, + column_id_range, _ptype: PhantomData, } } pub fn keys(&self) -> impl Iterator { - (0..self.values.len()).map(move |i| PolyID { + self.column_id_range.clone().into_iter().map(|i| PolyID { id: i as u64, ptype: T::P_TYPE, }) @@ -96,12 +98,12 @@ impl ColumnMap { self.keys().zip(self.values) } - // TODO check + // TODO check if usage assumes keys pub fn values(&self) -> impl Iterator { self.values.iter() } - // TODO check + // TODO check if usage assumes keys pub fn values_into_iter(self) -> impl Iterator { self.values.into_iter() } @@ -116,7 +118,7 @@ impl ColumnMap { } pub fn column_id_range(&self) -> Range { - 0..self.values.len() + self.column_id_range.clone() } } @@ -124,6 +126,7 @@ impl Default for ColumnMap { fn default() -> Self { ColumnMap { values: Vec::new(), + column_id_range: Default::default(), _ptype: PhantomData, } } @@ -131,7 +134,7 @@ impl Default for ColumnMap { impl PartialEq for ColumnMap { fn eq(&self, other: &Self) -> bool { - self.values == other.values + self.column_id_range == other.column_id_range && self.values == other.values } } @@ -141,7 +144,7 @@ impl Index<&PolyID> for ColumnMap { #[inline] fn index(&self, poly_id: &PolyID) -> &Self::Output { debug_assert!(poly_id.ptype == T::P_TYPE); - &self.values[poly_id.id as usize] + &self.values[poly_id.id as usize - self.column_id_range.start] } } @@ -149,6 +152,6 @@ impl IndexMut<&PolyID> for ColumnMap { #[inline] fn index_mut(&mut self, poly_id: &PolyID) -> &mut Self::Output { debug_assert!(poly_id.ptype == T::P_TYPE); - &mut self.values[poly_id.id as usize] + &mut self.values[poly_id.id as usize - self.column_id_range.start] } } diff --git a/executor/src/witgen/global_constraints.rs b/executor/src/witgen/global_constraints.rs index 37a48d393..0520f5e00 100644 --- a/executor/src/witgen/global_constraints.rs +++ b/executor/src/witgen/global_constraints.rs @@ -126,8 +126,8 @@ pub fn set_global_constraints<'a, T: FieldElement>( } } let fixed_constraints = FixedColumnMap::from_indexed( + 0..fixed_data.fixed_cols.len(), known_constraints.iter().map(|(p, c)| (*p, Some(c.clone()))), - fixed_data.fixed_cols.len(), ); let mut retained_identities = vec![]; From 77a61074d5755d4a17fb126591b743a27b7130fc Mon Sep 17 00:00:00 2001 From: chriseth Date: Mon, 1 Jul 2024 20:39:13 +0000 Subject: [PATCH 3/7] remove len --- executor/src/witgen/block_processor.rs | 7 +------ executor/src/witgen/data_structures/column_map.rs | 5 ----- executor/src/witgen/global_constraints.rs | 2 +- executor/src/witgen/mod.rs | 6 +++--- 4 files changed, 5 insertions(+), 15 deletions(-) diff --git a/executor/src/witgen/block_processor.rs b/executor/src/witgen/block_processor.rs index 6b41cbb13..73861a961 100644 --- a/executor/src/witgen/block_processor.rs +++ b/executor/src/witgen/block_processor.rs @@ -162,12 +162,7 @@ mod tests { let mut fixed_lookup = FixedLookup::new(fixed_data.global_range_constraints().clone()); let mut machines = []; - let columns = (0..fixed_data.witness_cols.len()) - .map(move |i| PolyID { - id: i as u64, - ptype: PolynomialType::Committed, - }) - .collect(); + let columns = fixed_data.witness_cols.keys().collect(); let data = FinalizableData::with_initial_rows_in_progress( &columns, (0..fixed_data.degree) diff --git a/executor/src/witgen/data_structures/column_map.rs b/executor/src/witgen/data_structures/column_map.rs index dc0789649..c51c60995 100644 --- a/executor/src/witgen/data_structures/column_map.rs +++ b/executor/src/witgen/data_structures/column_map.rs @@ -112,11 +112,6 @@ impl ColumnMap { self.values.iter_mut() } - // TODO remove in the long term - pub fn len(&self) -> usize { - self.values.len() - } - pub fn column_id_range(&self) -> Range { self.column_id_range.clone() } diff --git a/executor/src/witgen/global_constraints.rs b/executor/src/witgen/global_constraints.rs index 0520f5e00..c0225242c 100644 --- a/executor/src/witgen/global_constraints.rs +++ b/executor/src/witgen/global_constraints.rs @@ -126,7 +126,7 @@ pub fn set_global_constraints<'a, T: FieldElement>( } } let fixed_constraints = FixedColumnMap::from_indexed( - 0..fixed_data.fixed_cols.len(), + fixed_data.fixed_cols.column_id_range(), known_constraints.iter().map(|(p, c)| (*p, Some(c.clone()))), ); diff --git a/executor/src/witgen/mod.rs b/executor/src/witgen/mod.rs index 08702f667..d6d0c05f4 100644 --- a/executor/src/witgen/mod.rs +++ b/executor/src/witgen/mod.rs @@ -342,8 +342,8 @@ impl<'a, T: FieldElement> FixedData<'a, T> { // The global range constraints are not set yet. let global_range_constraints = GlobalConstraints { - witness_constraints: WitnessColumnMap::new(None, 0..witness_cols.len()), - fixed_constraints: FixedColumnMap::new(None, 0..fixed_cols.len()), + witness_constraints: WitnessColumnMap::new(None, witness_cols.column_id_range()), + fixed_constraints: FixedColumnMap::new(None, fixed_cols.column_id_range()), }; FixedData { @@ -386,7 +386,7 @@ impl<'a, T: FieldElement> FixedData<'a, T> { } fn witness_map_with(&self, initial_value: V) -> WitnessColumnMap { - WitnessColumnMap::new(initial_value, 0..self.witness_cols.len()) + WitnessColumnMap::new(initial_value, self.witness_cols.column_id_range()) } fn column_name(&self, poly_id: &PolyID) -> &str { From e906eb08ccde458394747a4675db74a3fa9755e2 Mon Sep 17 00:00:00 2001 From: chriseth Date: Mon, 1 Jul 2024 20:50:18 +0000 Subject: [PATCH 4/7] some more cleanup --- executor/src/witgen/block_processor.rs | 2 +- executor/src/witgen/data_structures/column_map.rs | 7 +++++-- executor/src/witgen/rows.rs | 4 ++++ 3 files changed, 10 insertions(+), 3 deletions(-) diff --git a/executor/src/witgen/block_processor.rs b/executor/src/witgen/block_processor.rs index 73861a961..fd1954790 100644 --- a/executor/src/witgen/block_processor.rs +++ b/executor/src/witgen/block_processor.rs @@ -116,7 +116,7 @@ impl<'a, 'b, 'c, T: FieldElement, Q: QueryCallback> BlockProcessor<'a, 'b, 'c mod tests { use std::collections::BTreeMap; - use powdr_ast::analyzed::{PolyID, PolynomialType}; + use powdr_ast::analyzed::PolyID; use powdr_number::{FieldElement, GoldilocksField}; use powdr_pil_analyzer::analyze_string; diff --git a/executor/src/witgen/data_structures/column_map.rs b/executor/src/witgen/data_structures/column_map.rs index c51c60995..a811f179a 100644 --- a/executor/src/witgen/data_structures/column_map.rs +++ b/executor/src/witgen/data_structures/column_map.rs @@ -52,6 +52,8 @@ impl ColumnMap { } impl ColumnMap { + /// Creates a new ColumnMap from an iterator over values, corresponding + /// to the keys in the given range, in the iteration order. pub fn from(column_id_range: Range, values: impl Iterator) -> Self { let values: Vec<_> = values.collect(); assert_eq!(values.len(), column_id_range.len()); @@ -98,16 +100,17 @@ impl ColumnMap { self.keys().zip(self.values) } - // TODO check if usage assumes keys + /// Returns an iterator over the values, in the order of the keys. pub fn values(&self) -> impl Iterator { self.values.iter() } - // TODO check if usage assumes keys + /// Returns an iterator over the values, in the order of the keys. pub fn values_into_iter(self) -> impl Iterator { self.values.into_iter() } + /// Returns a mutating iterator over the values, in the order of the keys. pub fn values_iter_mut(&mut self) -> impl Iterator { self.values.iter_mut() } diff --git a/executor/src/witgen/rows.rs b/executor/src/witgen/rows.rs index 865c885c6..54116f863 100644 --- a/executor/src/witgen/rows.rs +++ b/executor/src/witgen/rows.rs @@ -201,6 +201,10 @@ impl Row { /// Merges two rows, updating the first. /// Range constraints from the second row are ignored. pub fn merge_with(&mut self, other: &Row) -> Result<(), ()> { + debug_assert_eq!( + self.values.column_id_range(), + other.values.column_id_range() + ); // First check for conflicts, otherwise we would have to roll back changes. if self .values From c3742a6405848ceebed181ed08bb8f1b7b79b677 Mon Sep 17 00:00:00 2001 From: chriseth Date: Mon, 1 Jul 2024 22:19:02 +0000 Subject: [PATCH 5/7] Use only subset of columns. --- executor/src/witgen/block_processor.rs | 3 +- .../src/witgen/data_structures/column_map.rs | 4 ++ executor/src/witgen/generator.rs | 6 ++- executor/src/witgen/machines/block_machine.rs | 19 +++++++- executor/src/witgen/rows.rs | 46 +++++++++++++------ executor/src/witgen/vm_processor.rs | 7 ++- 6 files changed, 65 insertions(+), 20 deletions(-) diff --git a/executor/src/witgen/block_processor.rs b/executor/src/witgen/block_processor.rs index fd1954790..9d3b7b59a 100644 --- a/executor/src/witgen/block_processor.rs +++ b/executor/src/witgen/block_processor.rs @@ -165,8 +165,7 @@ mod tests { let columns = fixed_data.witness_cols.keys().collect(); let data = FinalizableData::with_initial_rows_in_progress( &columns, - (0..fixed_data.degree) - .map(|i| Row::fresh(&fixed_data, RowIndex::from_degree(i, fixed_data.degree))), + (0..fixed_data.degree).map(|i| Row::fresh(&fixed_data, fixed_data.witness_cols.keys())), ); let mut mutable_state = MutableState { diff --git a/executor/src/witgen/data_structures/column_map.rs b/executor/src/witgen/data_structures/column_map.rs index a811f179a..6943a65fb 100644 --- a/executor/src/witgen/data_structures/column_map.rs +++ b/executor/src/witgen/data_structures/column_map.rs @@ -100,6 +100,10 @@ impl ColumnMap { self.keys().zip(self.values) } + pub fn iter_mut(&mut self) -> impl Iterator { + self.keys().zip(self.values.iter_mut()) + } + /// Returns an iterator over the values, in the order of the keys. pub fn values(&self) -> impl Iterator { self.values.iter() diff --git a/executor/src/witgen/generator.rs b/executor/src/witgen/generator.rs index e595e986b..2cd697fc2 100644 --- a/executor/src/witgen/generator.rs +++ b/executor/src/witgen/generator.rs @@ -111,6 +111,7 @@ impl<'a, T: FieldElement> Generator<'a, T> { witnesses: HashSet, latch: Option>, ) -> Self { + // TODO restrict columns to those in witnesses let data = FinalizableData::new(&witnesses); Self { connecting_identities: connecting_identities.clone(), @@ -165,14 +166,15 @@ impl<'a, T: FieldElement> Generator<'a, T> { // Note that using `BlockProcessor` instead of `VmProcessor` is more convenient here because // it does not assert that the row is "complete" afterwards (i.e., that all identities // are satisfied assuming 0 for unknown values). + let row = Row::fresh(self.fixed_data, self.witnesses.iter().cloned()); let data = FinalizableData::with_initial_rows_in_progress( &self.witnesses, [ - Row::fresh( + row.clone().with_external_witness_values( self.fixed_data, RowIndex::from_i64(-1, self.fixed_data.degree), ), - Row::fresh( + row.with_external_witness_values( self.fixed_data, RowIndex::from_i64(0, self.fixed_data.degree), ), diff --git a/executor/src/witgen/machines/block_machine.rs b/executor/src/witgen/machines/block_machine.rs index 9894e6305..e95541f36 100644 --- a/executor/src/witgen/machines/block_machine.rs +++ b/executor/src/witgen/machines/block_machine.rs @@ -1,6 +1,7 @@ use std::collections::{BTreeMap, BTreeSet, HashMap, HashSet}; use std::fmt::Display; use std::iter; +use std::ops::Range; use super::{EvalResult, FixedData, FixedLookup}; @@ -107,6 +108,8 @@ pub struct BlockMachine<'a, T: FieldElement> { connection_type: ConnectionType, /// The internal identities identities: Vec<&'a Identity>, + /// A prototypical row with global range constraints set, but uninitialized otherwise. + basic_row: Row, /// The data of the machine. data: FinalizableData, /// The index of the first row that has not been finalized yet. @@ -152,9 +155,16 @@ impl<'a, T: FieldElement> BlockMachine<'a, T> { // In `take_witness_col_values()`, this block will be removed and its values will be used to // construct the "default" block used to fill up unused rows. let start_index = RowIndex::from_i64(-(block_size as i64), fixed_data.degree); + // compute the min and max of the witness col ids + + let basic_row = Row::fresh(fixed_data, witness_cols.iter().cloned()); let data = FinalizableData::with_initial_rows_in_progress( witness_cols, - (0..block_size).map(|i| Row::fresh(fixed_data, start_index + i)), + (0..block_size).map(|i| { + basic_row + .clone() + .with_external_witness_values(fixed_data, start_index + i) + }), ); Some(BlockMachine { name, @@ -163,6 +173,7 @@ impl<'a, T: FieldElement> BlockMachine<'a, T> { connecting_identities: connecting_identities.clone(), connection_type: is_permutation, identities: identities.to_vec(), + basic_row, data, first_in_progress_row: block_size, witness_cols: witness_cols.clone(), @@ -560,7 +571,11 @@ impl<'a, T: FieldElement> BlockMachine<'a, T> { // and the first row of the next block. let block = FinalizableData::with_initial_rows_in_progress( &self.witness_cols, - (0..(self.block_size + 2)).map(|i| Row::fresh(self.fixed_data, row_offset + i)), + (0..(self.block_size + 2)).map(|i| { + self.basic_row + .clone() + .with_external_witness_values(self.fixed_data, row_offset + i) + }), ); let mut processor = BlockProcessor::new( row_offset, diff --git a/executor/src/witgen/rows.rs b/executor/src/witgen/rows.rs index 54116f863..af867686f 100644 --- a/executor/src/witgen/rows.rs +++ b/executor/src/witgen/rows.rs @@ -1,11 +1,13 @@ use std::{ collections::HashSet, fmt::Display, - ops::{Add, Sub}, + ops::{Add, Range, Sub}, }; use itertools::Itertools; -use powdr_ast::analyzed::{AlgebraicExpression as Expression, AlgebraicReference, PolyID}; +use powdr_ast::analyzed::{ + AlgebraicExpression as Expression, AlgebraicReference, PolyID, PolynomialType, +}; use powdr_number::{DegreeType, FieldElement}; use crate::witgen::Constraint; @@ -255,22 +257,24 @@ impl Row { impl Row { /// Creates a "fresh" row, i.e., one that is empty but initialized with the global range constraints. - pub fn fresh(fixed_data: &FixedData<'_, T>, row: RowIndex) -> Row { - // TODO this instance could be computed exactly once (per column set) and then cloned. - // TODO and we could copy in the external witnesses later on - // TODO we should really only have a subset of the columns. + pub fn fresh(fixed_data: &FixedData<'_, T>, columns: impl Iterator) -> Row { + let column_id_range = columns + .map(|poly_id| poly_id.id as usize) + .minmax() + .into_option() + .map(|(min, max)| min..(max - 1)) + .unwrap_or_default(); + let values = WitnessColumnMap::from( - fixed_data.witness_cols.column_id_range(), + column_id_range.clone(), fixed_data .global_range_constraints() .witness_constraints .iter() - .map(|(poly_id, rc)| { - if let Some(external_witness) = - fixed_data.external_witness(row.into(), &poly_id) - { - CellValue::Known(external_witness) - } else if let Some(rc) = rc { + // TODO maybe the Range should be over PolyID directly instead of usize? + .filter(|(poly_id, _)| column_id_range.contains(&(poly_id.id as usize))) + .map(|(_, rc)| { + if let Some(rc) = rc { CellValue::RangeConstraint(rc.clone()) } else { CellValue::Unknown @@ -280,6 +284,22 @@ impl Row { Self { values } } + /// Adds the externally-provided witness values for the given row. + pub fn with_external_witness_values( + mut self, + fixed_data: &FixedData<'_, T>, + row: RowIndex, + ) -> Self { + // TODO we should store which witness cols have external data at some point higher up in the call chain + let row = DegreeType::from(row); + for (poly_id, cell) in self.values.iter_mut() { + if let Some(external_witness) = fixed_data.external_witness(row, &poly_id) { + *cell = CellValue::Known(external_witness); + } + } + self + } + /// Builds a string representing the current row pub fn render( &self, diff --git a/executor/src/witgen/vm_processor.rs b/executor/src/witgen/vm_processor.rs index f18f5b75d..471df4cfc 100644 --- a/executor/src/witgen/vm_processor.rs +++ b/executor/src/witgen/vm_processor.rs @@ -54,6 +54,8 @@ pub struct VmProcessor<'a, 'b, 'c, T: FieldElement, Q: QueryCallback> { /// The subset of identities that does not contain a reference to the next row /// (precomputed once for performance reasons) identities_without_next_ref: Vec<&'a Identity>, + /// A prototypical row that contains global range constraints but otherwise on values. + basic_row: Row, last_report: DegreeType, last_report_time: Instant, processor: Processor<'a, 'b, 'c, T, Q>, @@ -82,12 +84,15 @@ impl<'a, 'b, 'c, T: FieldElement, Q: QueryCallback> VmProcessor<'a, 'b, 'c, T .unwrap(), ); + let basic_row = Row::fresh(fixed_data, witnesses.iter().cloned()); + VmProcessor { row_offset: row_offset.into(), witnesses: witnesses.clone(), fixed_data, identities_with_next_ref: identities_with_next, identities_without_next_ref: identities_without_next, + basic_row, last_report: 0, last_report_time: Instant::now(), processor, @@ -229,7 +234,7 @@ impl<'a, 'b, 'c, T: FieldElement, Q: QueryCallback> VmProcessor<'a, 'b, 'c, T if row_index == self.processor.len() as DegreeType - 1 { self.processor.set_row( self.processor.len(), - Row::fresh( + self.basic_row.clone().with_external_witness_values( self.fixed_data, RowIndex::from_degree(row_index, self.fixed_data.degree) + 1, ), From c32e17926b1714d82e77c462b6596ae11c9aa0e2 Mon Sep 17 00:00:00 2001 From: chriseth Date: Tue, 2 Jul 2024 10:26:34 +0000 Subject: [PATCH 6/7] use subset of columns. --- executor/src/witgen/block_processor.rs | 5 +++-- .../src/witgen/data_structures/column_map.rs | 10 ++++++++++ executor/src/witgen/rows.rs | 20 +++++++++++-------- 3 files changed, 25 insertions(+), 10 deletions(-) diff --git a/executor/src/witgen/block_processor.rs b/executor/src/witgen/block_processor.rs index 9d3b7b59a..cee688904 100644 --- a/executor/src/witgen/block_processor.rs +++ b/executor/src/witgen/block_processor.rs @@ -114,7 +114,7 @@ impl<'a, 'b, 'c, T: FieldElement, Q: QueryCallback> BlockProcessor<'a, 'b, 'c #[cfg(test)] mod tests { - use std::collections::BTreeMap; + use std::{collections::BTreeMap, iter::repeat}; use powdr_ast::analyzed::PolyID; use powdr_number::{FieldElement, GoldilocksField}; @@ -163,9 +163,10 @@ mod tests { let mut machines = []; let columns = fixed_data.witness_cols.keys().collect(); + let basic_row = Row::fresh(&fixed_data, fixed_data.witness_cols.keys()); let data = FinalizableData::with_initial_rows_in_progress( &columns, - (0..fixed_data.degree).map(|i| Row::fresh(&fixed_data, fixed_data.witness_cols.keys())), + repeat(basic_row).take(fixed_data.degree as usize), ); let mut mutable_state = MutableState { diff --git a/executor/src/witgen/data_structures/column_map.rs b/executor/src/witgen/data_structures/column_map.rs index 6943a65fb..cafd2fdcf 100644 --- a/executor/src/witgen/data_structures/column_map.rs +++ b/executor/src/witgen/data_structures/column_map.rs @@ -122,6 +122,16 @@ impl ColumnMap { pub fn column_id_range(&self) -> Range { self.column_id_range.clone() } + + #[inline] + pub fn get(&self, poly_id: &PolyID) -> Option<&V> { + debug_assert!(poly_id.ptype == T::P_TYPE); + if self.column_id_range.contains(&(poly_id.id as usize)) { + Some(&self.values[poly_id.id as usize - self.column_id_range.start]) + } else { + None + } + } } impl Default for ColumnMap { diff --git a/executor/src/witgen/rows.rs b/executor/src/witgen/rows.rs index af867686f..570a6b180 100644 --- a/executor/src/witgen/rows.rs +++ b/executor/src/witgen/rows.rs @@ -122,12 +122,12 @@ enum CellValue { } impl CellValue { - pub fn is_known(&self) -> bool { + fn is_known(&self) -> bool { matches!(self, CellValue::Known(_)) } /// Returns the value if known, otherwise zero. - pub fn unwrap_or_zero(&self) -> T { + fn unwrap_or_zero(&self) -> T { match self { CellValue::Known(v) => *v, _ => Default::default(), @@ -135,7 +135,7 @@ impl CellValue { } /// Returns Some(value) if known, otherwise None. - pub fn value(&self) -> Option { + fn value(&self) -> Option { match self { CellValue::Known(v) => Some(*v), _ => None, @@ -146,7 +146,7 @@ impl CellValue { /// /// # Panics /// Panics if the update is not an improvement. - pub fn apply_update(&mut self, c: &Constraint) { + fn apply_update(&mut self, c: &Constraint) { match (&self, c) { (CellValue::Known(_), _) => { // Note that this is a problem even if the value that was set is the same, @@ -186,18 +186,19 @@ pub struct Row { impl Row { pub fn value_or_zero(&self, poly_id: &PolyID) -> T { + // This should not return zero for columns outside the set of columns, because they might be known. self.values[poly_id].unwrap_or_zero() } pub fn value(&self, poly_id: &PolyID) -> Option { - self.values[poly_id].value() + self.values.get(poly_id).and_then(|v| v.value()) } pub fn range_constraint(&self, poly_id: &PolyID) -> Option> { - match &self.values[poly_id] { + self.values.get(poly_id).and_then(|v| match v { CellValue::RangeConstraint(c) => Some(c.clone()), _ => None, - } + }) } /// Merges two rows, updating the first. @@ -262,8 +263,9 @@ impl Row { .map(|poly_id| poly_id.id as usize) .minmax() .into_option() - .map(|(min, max)| min..(max - 1)) + .map(|(min, max)| min..(max + 1)) .unwrap_or_default(); + println!("Creating row with columns: {:?}", column_id_range); let values = WitnessColumnMap::from( column_id_range.clone(), @@ -486,6 +488,7 @@ impl<'row, 'a, T: FieldElement> RowPair<'row, 'a, T> { pub fn get_value(&self, poly: &AlgebraicReference) -> Option { let row = self.get_row(poly.next); if self.unknown_strategy == UnknownStrategy::Zero { + // TODO this should panic if the column is out of range. Some(row.value_or_zero(&poly.poly_id)) } else { row.value(&poly.poly_id) @@ -507,6 +510,7 @@ impl<'row, 'a, T: FieldElement> RowPair<'row, 'a, T> { impl WitnessColumnEvaluator for RowPair<'_, '_, T> { fn value<'b>(&self, poly: &'b AlgebraicReference) -> AffineResult<&'b AlgebraicReference, T> { + // TODO this should not panic if the column is out of range. Ok(match self.get_value(poly) { Some(v) => v.into(), None => AffineExpression::from_variable_id(poly), From c02168342a650036760e2f0bd789ce7d0ce33244 Mon Sep 17 00:00:00 2001 From: chriseth Date: Wed, 3 Jul 2024 18:02:38 +0000 Subject: [PATCH 7/7] Clippy. --- executor/src/witgen/data_structures/column_map.rs | 2 +- executor/src/witgen/machines/block_machine.rs | 2 +- executor/src/witgen/rows.rs | 5 ++--- 3 files changed, 4 insertions(+), 5 deletions(-) diff --git a/executor/src/witgen/data_structures/column_map.rs b/executor/src/witgen/data_structures/column_map.rs index cafd2fdcf..986fe15b7 100644 --- a/executor/src/witgen/data_structures/column_map.rs +++ b/executor/src/witgen/data_structures/column_map.rs @@ -86,7 +86,7 @@ impl ColumnMap { } pub fn keys(&self) -> impl Iterator { - self.column_id_range.clone().into_iter().map(|i| PolyID { + self.column_id_range.clone().map(|i| PolyID { id: i as u64, ptype: T::P_TYPE, }) diff --git a/executor/src/witgen/machines/block_machine.rs b/executor/src/witgen/machines/block_machine.rs index e95541f36..ba9611a7f 100644 --- a/executor/src/witgen/machines/block_machine.rs +++ b/executor/src/witgen/machines/block_machine.rs @@ -1,7 +1,7 @@ use std::collections::{BTreeMap, BTreeSet, HashMap, HashSet}; use std::fmt::Display; use std::iter; -use std::ops::Range; + use super::{EvalResult, FixedData, FixedLookup}; diff --git a/executor/src/witgen/rows.rs b/executor/src/witgen/rows.rs index 570a6b180..214fe8275 100644 --- a/executor/src/witgen/rows.rs +++ b/executor/src/witgen/rows.rs @@ -1,12 +1,12 @@ use std::{ collections::HashSet, fmt::Display, - ops::{Add, Range, Sub}, + ops::{Add, Sub}, }; use itertools::Itertools; use powdr_ast::analyzed::{ - AlgebraicExpression as Expression, AlgebraicReference, PolyID, PolynomialType, + AlgebraicExpression as Expression, AlgebraicReference, PolyID, }; use powdr_number::{DegreeType, FieldElement}; @@ -265,7 +265,6 @@ impl Row { .into_option() .map(|(min, max)| min..(max + 1)) .unwrap_or_default(); - println!("Creating row with columns: {:?}", column_id_range); let values = WitnessColumnMap::from( column_id_range.clone(),