Skip to content

Commit

Permalink
Try to allow multiple passes with the MergeExpressionsOptimizer
Browse files Browse the repository at this point in the history
  • Loading branch information
aakoshh committed Dec 4, 2024
1 parent 09fb747 commit 97f75ba
Show file tree
Hide file tree
Showing 4 changed files with 34 additions and 11 deletions.
1 change: 1 addition & 0 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion acvm-repo/acvm/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ workspace = true
thiserror.workspace = true
tracing.workspace = true
serde.workspace = true

fxhash.workspace = true
acir.workspace = true
brillig_vm.workspace = true
acvm_blackbox_solver.workspace = true
Expand Down
37 changes: 27 additions & 10 deletions acvm-repo/acvm/src/compiler/transformers/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,10 @@ use super::{
optimizers::MergeExpressionsOptimizer, transform_assert_messages, AcirTransformationMap,
};

/// The `MergeExpressionOptimizer` needs multiple passes to stabilize the output.
/// Testing showed two passes to be enough.
const MAX_OPTIMIZER_PASSES: usize = 10;

/// Applies [`ProofSystemCompiler`][crate::ProofSystemCompiler] specific optimizations to a [`Circuit`].
pub fn transform<F: AcirField>(
acir: Circuit<F>,
Expand Down Expand Up @@ -153,24 +157,37 @@ pub(super) fn transform_internal<F: AcirField>(

let current_witness_index = next_witness_index - 1;

let acir = Circuit {
let mut acir = Circuit {
current_witness_index,
expression_width,
opcodes: transformed_opcodes,
// The transformer does not add new public inputs
..acir
};
let mut merge_optimizer = MergeExpressionsOptimizer::new();

let (opcodes, new_acir_opcode_positions) =
merge_optimizer.eliminate_intermediate_variable(&acir, new_acir_opcode_positions);
// Allow multiple passes until we have stable output.
let mut prev_circuit_hash = fxhash::hash64(&acir);
for _ in 0..MAX_OPTIMIZER_PASSES {
let mut merge_optimizer = MergeExpressionsOptimizer::new();

// n.b. if we do not update current_witness_index after the eliminate_intermediate_variable pass, the real index could be less.
let mut acir = Circuit {
opcodes,
// The optimizer does not add new public inputs
..acir
};
let (next_opcodes, next_acir_opcode_positions) =
merge_optimizer.eliminate_intermediate_variable(&acir, new_acir_opcode_positions);

// n.b. if we do not update current_witness_index after the eliminate_intermediate_variable pass, the real index could be less.
acir = Circuit {
opcodes: next_opcodes,
// The optimizer does not add new public inputs
..acir
};

let next_circuit_hash = fxhash::hash64(&acir);
new_acir_opcode_positions = next_acir_opcode_positions;

if next_circuit_hash == prev_circuit_hash {
break;
}
prev_circuit_hash = next_circuit_hash;
}

// After the elimination of intermediate variables the `current_witness_index` is potentially higher than it needs to be,
// which would cause gaps if we ran the optimization a second time, making it look like new variables were added.
Expand Down
5 changes: 5 additions & 0 deletions tooling/nargo_cli/src/cli/compile_cmd.rs
Original file line number Diff line number Diff line change
Expand Up @@ -413,6 +413,11 @@ mod tests {

let width = get_target_width(package.expression_width, None);

// Figure out if there is an upper bound to repeating the entire optimization step after which the results are stable:
// for _ in 0..2 {
// program_0 = nargo::ops::transform_program(program_0, width);
// }

let program_1 = nargo::ops::transform_program(program_0, width);
let program_2 = nargo::ops::transform_program(program_1.clone(), width);

Expand Down

0 comments on commit 97f75ba

Please sign in to comment.