Skip to content
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

Enable Bus in tests #2289

Merged
merged 11 commits into from
Dec 30, 2024
10 changes: 10 additions & 0 deletions ast/src/parsed/asm.rs
Original file line number Diff line number Diff line change
Expand Up @@ -191,6 +191,16 @@ impl SymbolPath {
pub fn into_parts(self) -> impl DoubleEndedIterator + ExactSizeIterator<Item = Part> {
self.parts.into_iter()
}

pub fn is_std(&self) -> bool {
self.parts
.first()
.map(|p| match p {
Part::Named(n) => n == "std",
Part::Super => false,
})
.unwrap_or(false)
}
}

impl FromStr for SymbolPath {
Expand Down
12 changes: 4 additions & 8 deletions executor/src/witgen/jit/witgen_inference.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5,8 +5,8 @@ use itertools::Itertools;
use powdr_ast::analyzed::{
AlgebraicBinaryOperation, AlgebraicBinaryOperator, AlgebraicExpression as Expression,
AlgebraicReference, AlgebraicUnaryOperation, AlgebraicUnaryOperator, Identity, LookupIdentity,
PermutationIdentity, PhantomBusInteractionIdentity, PhantomLookupIdentity,
PhantomPermutationIdentity, PolynomialIdentity, PolynomialType,
PermutationIdentity, PhantomLookupIdentity, PhantomPermutationIdentity, PolynomialIdentity,
PolynomialType,
};
use powdr_number::FieldElement;

Expand Down Expand Up @@ -86,12 +86,8 @@ impl<'a, T: FieldElement, FixedEval: FixedEvaluator<T>> WitgenInference<'a, T, F
&left.expressions,
row_offset,
),
Identity::PhantomBusInteraction(PhantomBusInteractionIdentity {
id,
multiplicity,
tuple,
..
}) => self.process_call(can_process, *id, multiplicity, &tuple.0, row_offset),
// TODO(bus_interaction)
Identity::PhantomBusInteraction(_) => ProcessResult::empty(),
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bug: Witgen filters out Bus interactions, so processing a call here leads to witgen not being able to find the called machine by the identity ID. This is also not trivial, because we'd probably want to use the receiving bus interaction.

Right now, all bus interactions also come with a phantom lookup / permutation, so they can safely be ignored. // TODO(bus_interaction) mark the places that need to be adjusted once this is removed as part of #2184.

Identity::Connect(_) => ProcessResult::empty(),
};
self.ingest_effects(result)
Expand Down
2 changes: 1 addition & 1 deletion executor/src/witgen/machines/block_machine.rs
Original file line number Diff line number Diff line change
Expand Up @@ -445,7 +445,7 @@ impl<'a, T: FieldElement> BlockMachine<'a, T> {

let updates = updates.report_side_effect();

let global_latch_row_index = self.data.len() - 1 - self.block_size + self.latch_row;
let global_latch_row_index = self.data.len() - self.block_size + self.latch_row;
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bug: This -1 is just wrong.

self.multiplicity_counter
.increment_at_row(identity_id, global_latch_row_index);

Expand Down
4 changes: 3 additions & 1 deletion executor/src/witgen/machines/dynamic_machine.rs
Original file line number Diff line number Diff line change
Expand Up @@ -96,7 +96,9 @@ impl<'a, T: FieldElement> Machine<'a, T> for DynamicMachine<'a, T> {
self.data.extend(updated_data.block);
self.publics.extend(updated_data.publics);

let latch_row = self.data.len() - 1;
// The block we just added contains the first row of the next block,
// so the latch row is the second-to-last row.
let latch_row = self.data.len() - 2;
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bug: See comment.

self.multiplicity_counter
.increment_at_row(identity_id, latch_row);

Expand Down
8 changes: 5 additions & 3 deletions linker/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -360,9 +360,11 @@ fn namespaced_expression(namespace: String, mut expr: Expression) -> Expression
expr.visit_expressions_mut(
&mut |expr| {
if let Expression::Reference(_, refs) = expr {
refs.path = SymbolPath::from_parts(
once(Part::Named(namespace.clone())).chain(refs.path.clone().into_parts()),
);
if !refs.path.is_std() {
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bug: We used to namespace std namespaces as well. So, if there was a lookup from main into main_sub, using something in std::* in its expressions, it would turn into main::std::*.

refs.path = SymbolPath::from_parts(
once(Part::Named(namespace.clone())).chain(refs.path.clone().into_parts()),
);
}
}
ControlFlow::Continue::<(), _>(())
},
Expand Down
43 changes: 34 additions & 9 deletions pipeline/src/test_util.rs
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
use powdr_ast::analyzed::Analyzed;
use powdr_linker::{DegreeMode, LinkerMode, LinkerParams};
use powdr_number::{
BabyBearField, BigInt, Bn254Field, FieldElement, GoldilocksField, KoalaBearField,
};
Expand All @@ -22,9 +23,17 @@ pub fn resolve_test_file(file_name: &str) -> PathBuf {
/// Makes a new pipeline for the given file. All steps until witness generation are
/// already computed, so that the test can branch off from there, without having to re-compute
/// these steps.
pub fn make_simple_prepared_pipeline<T: FieldElement>(file_name: &str) -> Pipeline<T> {
pub fn make_simple_prepared_pipeline<T: FieldElement>(
file_name: &str,
linker_mode: LinkerMode,
) -> Pipeline<T> {
let linker_params = LinkerParams {
mode: linker_mode,
degree_mode: DegreeMode::Vadcop,
};
let mut pipeline = Pipeline::default()
.with_tmp_output()
.with_linker_params(linker_params)
.from_file(resolve_test_file(file_name));
pipeline.compute_witness().unwrap();
pipeline
Expand All @@ -37,9 +46,15 @@ pub fn make_prepared_pipeline<T: FieldElement>(
file_name: &str,
inputs: Vec<T>,
external_witness_values: Vec<(String, Vec<T>)>,
linker_mode: LinkerMode,
) -> Pipeline<T> {
let linker_params = LinkerParams {
mode: linker_mode,
degree_mode: DegreeMode::Vadcop,
};
let mut pipeline = Pipeline::default()
.with_tmp_output()
.with_linker_params(linker_params)
.from_file(resolve_test_file(file_name))
.with_prover_inputs(inputs)
.add_external_witness_values(external_witness_values);
Expand All @@ -62,27 +77,37 @@ pub fn regular_test_small_field(file_name: &str, inputs: &[i32]) {
/// Tests witness generation, mock prover, pilcom and plonky3 with BabyBear.
pub fn regular_test_bb(file_name: &str, inputs: &[i32]) {
let inputs_bb = inputs.iter().map(|x| BabyBearField::from(*x)).collect();
let pipeline_bb = make_prepared_pipeline(file_name, inputs_bb, vec![]);
// LinkerMode::Native because the bus is not implemented for small fields
let pipeline_bb = make_prepared_pipeline(file_name, inputs_bb, vec![], LinkerMode::Native);
test_mock_backend(pipeline_bb.clone());
test_plonky3_pipeline(pipeline_bb);
}

/// Tests witness generation, mock prover, pilcom and plonky3 with BabyBear and KoalaBear.
pub fn regular_test_kb(file_name: &str, inputs: &[i32]) {
let inputs_kb = inputs.iter().map(|x| KoalaBearField::from(*x)).collect();
let pipeline_kb = make_prepared_pipeline(file_name, inputs_kb, vec![]);
// LinkerMode::Native because the bus is not implemented for small fields
let pipeline_kb = make_prepared_pipeline(file_name, inputs_kb, vec![], LinkerMode::Native);
test_mock_backend(pipeline_kb.clone());
test_plonky3_pipeline(pipeline_kb);
}

/// Tests witness generation, mock prover, pilcom and plonky3 with Goldilocks.
pub fn regular_test_gl(file_name: &str, inputs: &[i32]) {
let inputs_gl = inputs.iter().map(|x| GoldilocksField::from(*x)).collect();
let pipeline_gl = make_prepared_pipeline(file_name, inputs_gl, vec![]);

test_mock_backend(pipeline_gl.clone());
run_pilcom_with_backend_variant(pipeline_gl.clone(), BackendVariant::Composite).unwrap();
test_plonky3_pipeline(pipeline_gl);
let inputs_gl = inputs
.iter()
.map(|x| GoldilocksField::from(*x))
.collect::<Vec<_>>();

let pipeline_gl_native =
make_prepared_pipeline(file_name, inputs_gl.clone(), vec![], LinkerMode::Native);
test_mock_backend(pipeline_gl_native.clone());
run_pilcom_with_backend_variant(pipeline_gl_native, BackendVariant::Composite).unwrap();

let pipeline_gl_bus =
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that this means we now run steps such as constant generation, witgen, etc twice, once for each linker mode.

make_prepared_pipeline(file_name, inputs_gl.clone(), vec![], LinkerMode::Bus);
test_mock_backend(pipeline_gl_bus.clone());
test_plonky3_pipeline(pipeline_gl_bus);
}

pub fn test_pilcom(pipeline: Pipeline<GoldilocksField>) {
Expand Down
Loading
Loading