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.

Sign up for GitHub

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

Jump to bottom

chore: optimise older opcodes in reverse order #6476

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

chore: optimise older opcodes in reverse order #6476

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
b9a7795
handle expression in reverse order
guipublic Nov 7, 2024
f022806
update comment
guipublic Nov 7, 2024
c7f41e0
Merge branch 'master' into gd/merge_reverse_expression
guipublic Nov 7, 2024
a25af6d
Merge branch 'master' into gd/merge_reverse_expression
TomAFrench Nov 14, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
140 changes: 85 additions & 55 deletions acvm-repo/acvm/src/compiler/optimizers/merge_expressions.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,26 +13,36 @@

use crate::compiler::CircuitSimulator;

pub(crate) struct MergeExpressionsOptimizer {
pub(crate) struct MergeExpressionsOptimizer<F> {
resolved_blocks: HashMap<BlockId, BTreeSet<Witness>>,
modified_gates: HashMap<usize, Opcode<F>>,
deleted_gates: BTreeSet<usize>,
}

impl MergeExpressionsOptimizer {
impl<F: AcirField> MergeExpressionsOptimizer<F> {
pub(crate) fn new() -> Self {
MergeExpressionsOptimizer { resolved_blocks: HashMap::new() }
MergeExpressionsOptimizer {
resolved_blocks: HashMap::new(),
modified_gates: HashMap::new(),
deleted_gates: BTreeSet::new(),
}
}
/// This pass analyzes the circuit and identifies intermediate variables that are
/// only used in two gates. It then merges the gate that produces the
/// intermediate variable into the second one that uses it
/// Note: This pass is only relevant for backends that can handle unlimited width
pub(crate) fn eliminate_intermediate_variable<F: AcirField>(
pub(crate) fn eliminate_intermediate_variable(
&mut self,
circuit: &Circuit<F>,
acir_opcode_positions: Vec<usize>,
) -> (Vec<Opcode<F>>, Vec<usize>) {
// Initialisation

Check warning on line 39 in acvm-repo/acvm/src/compiler/optimizers/merge_expressions.rs

View workflow job for this annotation

GitHub Actions / Code 

Unknown word (Initialisation)
self.modified_gates.clear();
self.deleted_gates.clear();
self.resolved_blocks.clear();

// Keep track, for each witness, of the gates that use it
let circuit_inputs = circuit.circuit_arguments();
self.resolved_blocks = HashMap::new();
let mut used_witness: BTreeMap<Witness, BTreeSet<usize>> = BTreeMap::new();
for (i, opcode) in circuit.opcodes.iter().enumerate() {
let witnesses = self.witness_inputs(opcode);
Expand All @@ -47,57 +57,68 @@
}
}

let mut modified_gates: HashMap<usize, Opcode<F>> = HashMap::new();
let mut new_circuit = Vec::new();
let mut new_acir_opcode_positions = Vec::new();
// For each opcode, try to get a target opcode to merge with
for (i, opcode) in circuit.opcodes.iter().enumerate() {
if !matches!(opcode, Opcode::AssertZero(_)) {
new_circuit.push(opcode.clone());
new_acir_opcode_positions.push(acir_opcode_positions[i]);
continue;
}
let opcode = modified_gates.get(&i).unwrap_or(opcode).clone();
let mut to_keep = true;
let opcode = self.modified_gates.get(&i).unwrap_or(opcode).clone();
let input_witnesses = self.witness_inputs(&opcode);
for w in input_witnesses.clone() {
let empty_gates = BTreeSet::new();
let gates_using_w = used_witness.get(&w).unwrap_or(&empty_gates);
// We only consider witness which are used in exactly two arithmetic gates
if gates_using_w.len() == 2 {
let gates_using_w: Vec<_> = gates_using_w.iter().collect();
let mut b = *gates_using_w[1];
if b == i {
b = *gates_using_w[0];
} else {
// sanity check
assert!(i == *gates_using_w[0]);
}
let second_gate = modified_gates.get(&b).unwrap_or(&circuit.opcodes[b]).clone();
if let (Opcode::AssertZero(expr_define), Opcode::AssertZero(expr_use)) =
(opcode.clone(), second_gate)
{
// We cannot merge an expression into an earlier opcode, because this
// would break the 'execution ordering' of the opcodes
// This case can happen because a previous merge would change an opcode
// and eliminate a witness from it, giving new opportunities for this
// witness to be used in only two expressions
// TODO: the missed optimization for the i>b case can be handled by
// - doing this pass again until there is no change, or
// - merging 'b' into 'i' instead
if i < b {
if let Some(expr) = Self::merge(&expr_use, &expr_define, w) {
modified_gates.insert(b, Opcode::AssertZero(expr));
to_keep = false;
// Update the 'used_witness' map to account for the merge.
for w2 in CircuitSimulator::expr_wit(&expr_define) {
if !circuit_inputs.contains(&w2) {
let mut v = used_witness[&w2].clone();
v.insert(b);
v.remove(&i);
used_witness.insert(w2, v);
let gates_using_w: Vec<_> = gates_using_w
.iter()
.filter(|g| **g != i && !self.deleted_gates.contains(g))
.collect();
if gates_using_w.len() == 2 {
// i is the current opcode (from the iteration)
// b is the (only) other opcode using w
let mut b = *gates_using_w[1];
if b == i {
b = *gates_using_w[0];
} else {
// sanity check
assert!(i == *gates_using_w[0]);
}
// Merge source opcode into target opcode
// by updating modified_gates/deleted_gates/used_witness
let mut merge_opcodes = |target, source| -> bool {
assert!(source < target);
let source_opcode = self.get_opcode(source, circuit);
let target_opcode = self.get_opcode(target, circuit);
if let (
Some(Opcode::AssertZero(expr_use)),
Some(Opcode::AssertZero(expr_define)),
) = (target_opcode, source_opcode)
{
if let Some(expr) =
Self::merge_expression(&expr_use, &expr_define, w)
{
self.modified_gates.insert(source, Opcode::AssertZero(expr));
self.deleted_gates.insert(target);
// Update the 'used_witness' map to account for the merge.
for w2 in CircuitSimulator::expr_wit(&expr_use) {
if !circuit_inputs.contains(&w2) {
let mut v = used_witness[&w2].clone();
v.insert(i);
v.remove(&b);
used_witness.insert(w2, v);
}
}
return true;
}
}
false
};
if i < b {
if merge_opcodes(b, i) {
// We need to stop here and continue with the next opcode
// because the merge invalidates the current opcode.
break;
}
} else if i != b {
// Merge b into i
if merge_opcodes(i, b) {
// We need to stop here and continue with the next opcode
// because the merge invalidates the current opcode.
break;
Expand All @@ -106,20 +127,22 @@
}
}
}
}

if to_keep {
if modified_gates.contains_key(&i) {
new_circuit.push(modified_gates[&i].clone());
} else {
new_circuit.push(opcode.clone());
}
// Construct the new circuit from modified/deleted gates
let mut new_circuit = Vec::new();
let mut new_acir_opcode_positions = Vec::new();
#[allow(clippy::needless_range_loop)]
for i in 0..circuit.opcodes.len() {
if let Some(op) = self.get_opcode(i, circuit) {
new_circuit.push(op);
new_acir_opcode_positions.push(acir_opcode_positions[i]);
}
}
(new_circuit, new_acir_opcode_positions)
}

fn brillig_input_wit<F>(&self, input: &BrilligInputs<F>) -> BTreeSet<Witness> {
fn brillig_input_wit(&self, input: &BrilligInputs<F>) -> BTreeSet<Witness> {
let mut result = BTreeSet::new();
match input {
BrilligInputs::Single(expr) => {
Expand Down Expand Up @@ -152,7 +175,7 @@
}

// Returns the input witnesses used by the opcode
fn witness_inputs<F: AcirField>(&self, opcode: &Opcode<F>) -> BTreeSet<Witness> {
fn witness_inputs(&self, opcode: &Opcode<F>) -> BTreeSet<Witness> {
let mut witnesses = BTreeSet::new();
match opcode {
Opcode::AssertZero(expr) => CircuitSimulator::expr_wit(expr),
Expand Down Expand Up @@ -198,7 +221,7 @@

// Merge 'expr' into 'target' via Gaussian elimination on 'w'
// Returns None if the expressions cannot be merged
fn merge<F: AcirField>(
fn merge_expression(
target: &Expression<F>,
expr: &Expression<F>,
w: Witness,
Expand Down Expand Up @@ -226,6 +249,13 @@
}
None
}

fn get_opcode(&self, g: usize, circuit: &Circuit<F>) -> Option<Opcode<F>> {
if self.deleted_gates.contains(&g) {
return None;
}
Some(self.modified_gates.get(&g).unwrap_or(&circuit.opcodes[g]).clone())
}
}

#[cfg(test)]
Expand Down
2 changes: 1 addition & 1 deletion test_programs/execution_success/regression_6451/src/main.nr
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ fn main(x: Field) {
value += term2;
value.assert_max_bit_size::<1>();

// Regression test for Aztec Packages issue #6451
// Regression test for #6447 (Aztec Packages issue #9488)
let y = unsafe { empty(x + 1) };
let z = y + x + 1;
let z1 = z + y;
Expand Down
Loading