From c7f42a9f17a040189f8ab64ac1eccb1a2d98b28a Mon Sep 17 00:00:00 2001 From: TomAFrench Date: Tue, 19 Nov 2024 23:40:28 +0000 Subject: [PATCH 1/4] chore: remove some unnecessary clones --- .../compiler/optimizers/merge_expressions.rs | 33 +++++++++---------- 1 file changed, 16 insertions(+), 17 deletions(-) diff --git a/noir/noir-repo/acvm-repo/acvm/src/compiler/optimizers/merge_expressions.rs b/noir/noir-repo/acvm-repo/acvm/src/compiler/optimizers/merge_expressions.rs index 56cfa000da2..c5eb3093d83 100644 --- a/noir/noir-repo/acvm-repo/acvm/src/compiler/optimizers/merge_expressions.rs +++ b/noir/noir-repo/acvm-repo/acvm/src/compiler/optimizers/merge_expressions.rs @@ -59,22 +59,23 @@ impl MergeExpressionsOptimizer { let opcode = modified_gates.get(&i).unwrap_or(opcode).clone(); let mut to_keep = true; 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); + for w in input_witnesses { + let gates_using_w = used_witness.entry(w).or_default(); // 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]; + let first = *gates_using_w.first().expect("gates_using_w.len == 2"); + let second = *gates_using_w.last().expect("gates_using_w.len == 2"); + let b = if second == i { + first } else { // sanity check - assert!(i == *gates_using_w[0]); - } - let second_gate = modified_gates.get(&b).unwrap_or(&circuit.opcodes[b]).clone(); + assert!(i == first); + second + }; + + let second_gate = modified_gates.get(&b).unwrap_or(&circuit.opcodes[b]); if let (Opcode::AssertZero(expr_define), Opcode::AssertZero(expr_use)) = - (opcode.clone(), second_gate) + (&opcode, second_gate) { // We cannot merge an expression into an earlier opcode, because this // would break the 'execution ordering' of the opcodes @@ -85,16 +86,14 @@ impl MergeExpressionsOptimizer { // - 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) { + 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) { + 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); + gates_using_w.insert(b); + gates_using_w.remove(&i); } } // We need to stop here and continue with the next opcode From 8cf0cc53eacb8846e635b2b43e15dd337b65aa7e Mon Sep 17 00:00:00 2001 From: TomAFrench Date: Tue, 19 Nov 2024 23:41:42 +0000 Subject: [PATCH 2/4] . --- .../acvm-repo/acvm/src/compiler/optimizers/merge_expressions.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/noir/noir-repo/acvm-repo/acvm/src/compiler/optimizers/merge_expressions.rs b/noir/noir-repo/acvm-repo/acvm/src/compiler/optimizers/merge_expressions.rs index c5eb3093d83..97513623c1e 100644 --- a/noir/noir-repo/acvm-repo/acvm/src/compiler/optimizers/merge_expressions.rs +++ b/noir/noir-repo/acvm-repo/acvm/src/compiler/optimizers/merge_expressions.rs @@ -109,7 +109,7 @@ impl MergeExpressionsOptimizer { if modified_gates.contains_key(&i) { new_circuit.push(modified_gates[&i].clone()); } else { - new_circuit.push(opcode.clone()); + new_circuit.push(opcode); } new_acir_opcode_positions.push(acir_opcode_positions[i]); } From 404b3e691e6ffc96ed4be9db1c7b6abcefd64d41 Mon Sep 17 00:00:00 2001 From: TomAFrench Date: Wed, 20 Nov 2024 00:08:22 +0000 Subject: [PATCH 3/4] . --- .../src/compiler/optimizers/merge_expressions.rs | 13 +++++-------- 1 file changed, 5 insertions(+), 8 deletions(-) diff --git a/noir/noir-repo/acvm-repo/acvm/src/compiler/optimizers/merge_expressions.rs b/noir/noir-repo/acvm-repo/acvm/src/compiler/optimizers/merge_expressions.rs index 97513623c1e..6c8e01549d3 100644 --- a/noir/noir-repo/acvm-repo/acvm/src/compiler/optimizers/merge_expressions.rs +++ b/noir/noir-repo/acvm-repo/acvm/src/compiler/optimizers/merge_expressions.rs @@ -50,10 +50,10 @@ impl MergeExpressionsOptimizer { 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() { + for (i, (opcode, opcode_position)) in circuit.opcodes.iter().zip(acir_opcode_positions).enumerate() { if !matches!(opcode, Opcode::AssertZero(_)) { new_circuit.push(opcode.clone()); - new_acir_opcode_positions.push(acir_opcode_positions[i]); + new_acir_opcode_positions.push(opcode_position); continue; } let opcode = modified_gates.get(&i).unwrap_or(opcode).clone(); @@ -106,12 +106,9 @@ impl MergeExpressionsOptimizer { } if to_keep { - if modified_gates.contains_key(&i) { - new_circuit.push(modified_gates[&i].clone()); - } else { - new_circuit.push(opcode); - } - new_acir_opcode_positions.push(acir_opcode_positions[i]); + let opcode = modified_gates.get(&i).cloned().unwrap_or(opcode); + new_circuit.push(opcode); + new_acir_opcode_positions.push(opcode_position); } } (new_circuit, new_acir_opcode_positions) From f2f8881929c78fae94b228ff3803b512ccc84dbd Mon Sep 17 00:00:00 2001 From: Tom French Date: Wed, 20 Nov 2024 01:55:08 +0000 Subject: [PATCH 4/4] . --- .../src/compiler/optimizers/merge_expressions.rs | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-) diff --git a/noir/noir-repo/acvm-repo/acvm/src/compiler/optimizers/merge_expressions.rs b/noir/noir-repo/acvm-repo/acvm/src/compiler/optimizers/merge_expressions.rs index 6c8e01549d3..0476aa4d574 100644 --- a/noir/noir-repo/acvm-repo/acvm/src/compiler/optimizers/merge_expressions.rs +++ b/noir/noir-repo/acvm-repo/acvm/src/compiler/optimizers/merge_expressions.rs @@ -50,7 +50,9 @@ impl MergeExpressionsOptimizer { 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, opcode_position)) in circuit.opcodes.iter().zip(acir_opcode_positions).enumerate() { + for (i, (opcode, opcode_position)) in + circuit.opcodes.iter().zip(acir_opcode_positions).enumerate() + { if !matches!(opcode, Opcode::AssertZero(_)) { new_circuit.push(opcode.clone()); new_acir_opcode_positions.push(opcode_position); @@ -60,7 +62,9 @@ impl MergeExpressionsOptimizer { let mut to_keep = true; let input_witnesses = self.witness_inputs(&opcode); for w in input_witnesses { - let gates_using_w = used_witness.entry(w).or_default(); + let Some(gates_using_w) = used_witness.get(&w) else { + continue; + }; // We only consider witness which are used in exactly two arithmetic gates if gates_using_w.len() == 2 { let first = *gates_using_w.first().expect("gates_using_w.len == 2"); @@ -92,8 +96,9 @@ impl MergeExpressionsOptimizer { // Update the 'used_witness' map to account for the merge. for w2 in CircuitSimulator::expr_wit(expr_define) { if !circuit_inputs.contains(&w2) { - gates_using_w.insert(b); - gates_using_w.remove(&i); + let v = used_witness.entry(w2).or_default(); + v.insert(b); + v.remove(&i); } } // We need to stop here and continue with the next opcode @@ -106,7 +111,7 @@ impl MergeExpressionsOptimizer { } if to_keep { - let opcode = modified_gates.get(&i).cloned().unwrap_or(opcode); + let opcode = modified_gates.get(&i).cloned().unwrap_or(opcode); new_circuit.push(opcode); new_acir_opcode_positions.push(opcode_position); }