Skip to content

Commit

Permalink
Assert that block machines are stackable (#2307)
Browse files Browse the repository at this point in the history
Fixes a TODO that makes block machine fail hard if a block machine is
not stackable. This way, we'll know when this happens, instead of
silently resorting to run-time solving.
  • Loading branch information
georgwiese authored Jan 9, 2025
1 parent e328eb9 commit 9c6315e
Show file tree
Hide file tree
Showing 4 changed files with 22 additions and 11 deletions.
18 changes: 11 additions & 7 deletions executor/src/witgen/jit/block_machine_processor.rs
Original file line number Diff line number Diff line change
Expand Up @@ -68,7 +68,7 @@ impl<'a, T: FieldElement> BlockMachineProcessor<'a, T> {
// Solve for the block witness.
// Fails if any machine call cannot be completed.
match self.solve_block(can_process, &mut witgen, connection.right) {
Ok(()) => Ok(witgen.code()),
Ok(()) => Ok(witgen.finish()),
Err(e) => {
log::debug!("\nCode generation failed for connection:\n {connection}");
let known_args_str = known_args
Expand All @@ -80,7 +80,7 @@ impl<'a, T: FieldElement> BlockMachineProcessor<'a, T> {
log::debug!("Error:\n {e}");
log::debug!(
"The following code was generated so far:\n{}",
format_code(witgen.code().as_slice())
format_code(witgen.code())
);
Err(format!("Code generation failed: {e}\nRun with RUST_LOG=debug to see the code generated so far."))
}
Expand Down Expand Up @@ -131,10 +131,14 @@ impl<'a, T: FieldElement> BlockMachineProcessor<'a, T> {
}
}

// TODO: Fail hard (or return a different error), as this should never
// happen for valid block machines. Currently fails in:
// powdr-pipeline::powdr_std arith256_memory_large_test
self.check_block_shape(witgen)?;
if let Err(e) = self.check_block_shape(witgen) {
// Fail hard, as this should never happen for a correctly detected block machine.
log::debug!(
"The following code was generated so far:\n{}",
format_code(witgen.code())
);
panic!("{e}");
}
self.check_incomplete_machine_calls(&complete)?;

Ok(())
Expand Down Expand Up @@ -192,7 +196,7 @@ impl<'a, T: FieldElement> BlockMachineProcessor<'a, T> {
block_list,
values[self.block_size + 1]
);
log::debug!("Column {column_name} is not stackable:\n{column_str}");
log::error!("Column {column_name} is not stackable:\n{column_str}");
}

stackable
Expand Down
2 changes: 1 addition & 1 deletion executor/src/witgen/jit/single_step_processor.rs
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,7 @@ impl<'a, T: FieldElement> SingleStepProcessor<'a, T> {

let missing_identities = self.machine_parts.identities.len() - complete.len();
let code = if unknown_witnesses.is_empty() && missing_identities == 0 {
witgen.code()
witgen.finish()
} else {
let Some(most_constrained_var) = witgen
.known_variables()
Expand Down
5 changes: 4 additions & 1 deletion executor/src/witgen/jit/symbolic_expression.rs
Original file line number Diff line number Diff line change
Expand Up @@ -262,7 +262,10 @@ impl<T: FieldElement, V: Clone> SymbolicExpression<T, V> {

/// Integer division, i.e. convert field elements to unsigned integer and divide.
pub fn integer_div(&self, rhs: &Self) -> Self {
if rhs.is_known_one() {
if let (SymbolicExpression::Concrete(a), SymbolicExpression::Concrete(b)) = (self, rhs) {
assert!(b != &T::from(0));
SymbolicExpression::Concrete(*a / *b)
} else if rhs.is_known_one() {
self.clone()
} else {
SymbolicExpression::BinaryOperation(
Expand Down
8 changes: 6 additions & 2 deletions executor/src/witgen/jit/witgen_inference.rs
Original file line number Diff line number Diff line change
Expand Up @@ -88,10 +88,14 @@ impl<'a, T: FieldElement, FixedEval: FixedEvaluator<T>> WitgenInference<'a, T, F
}
}

pub fn code(self) -> Vec<Effect<T, Variable>> {
pub fn finish(self) -> Vec<Effect<T, Variable>> {
self.code
}

pub fn code(&self) -> &[Effect<T, Variable>] {
&self.code
}

pub fn known_variables(&self) -> &HashSet<Variable> {
&self.known_variables
}
Expand Down Expand Up @@ -627,7 +631,7 @@ mod test {
}
assert!(counter < 10000, "Solving took more than 10000 rounds.");
}
format_code(&witgen.code())
format_code(witgen.code())
}

#[test]
Expand Down

1 comment on commit 9c6315e

@github-actions
Copy link

Choose a reason for hiding this comment

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

⚠️ Performance Alert ⚠️

Possible performance regression was detected for benchmark 'Benchmarks'.
Benchmark result of this commit is worse than the previous benchmark result exceeding threshold 1.20.

Benchmark suite Current: 9c6315e Previous: 0be5a43 Ratio
jit-benchmark/sqrt_879882356 18222 ns/iter (± 36) 2591 ns/iter (± 1) 7.03
jit-benchmark/sqrt_1882356 14531 ns/iter (± 19) 2075 ns/iter (± 1) 7.00
jit-benchmark/sqrt_1187956 13678 ns/iter (± 17) 2051 ns/iter (± 1) 6.67
jit-benchmark/sqrt_56 7120 ns/iter (± 21) 1229 ns/iter (± 2) 5.79
jit-benchmark/sort_33 385442 ns/iter (± 486) 71716 ns/iter (± 32) 5.37
jit-benchmark/sort_100 1505315 ns/iter (± 1457) 268321 ns/iter (± 236) 5.61
jit-benchmark/sort_300 6501512 ns/iter (± 3621) 1030191 ns/iter (± 1259) 6.31
jit-benchmark/sort_900 34255506 ns/iter (± 29041) 4300269 ns/iter (± 5480) 7.97
jit-benchmark/sort_2700 223133968 ns/iter (± 192781) 20688055 ns/iter (± 75629) 10.79

This comment was automatically generated by workflow using github-action-benchmark.

Please sign in to comment.