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

chore: remove clones in optimizer/transformer code #3037

Merged
merged 4 commits into from
Oct 9, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
2 changes: 1 addition & 1 deletion acvm-repo/acvm/src/compiler/optimizers/general.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
native_types::{Expression, Witness},
FieldElement,
};
use indexmap::IndexMap;

Check warning on line 5 in acvm-repo/acvm/src/compiler/optimizers/general.rs

View workflow job for this annotation

GitHub Actions / Spellcheck / Spellcheck

Unknown word (indexmap)

/// The `GeneralOptimizer` processes all [`Expression`]s to:
/// - remove any zero-coefficient terms.
Expand Down Expand Up @@ -31,7 +31,7 @@
let mut hash_map: IndexMap<(Witness, Witness), FieldElement> = IndexMap::new();

// Canonicalize the ordering of the multiplication, lets just order by variable name
for (scale, w_l, w_r) in gate.mul_terms.clone().into_iter() {
for (scale, w_l, w_r) in gate.mul_terms.into_iter() {
let mut pair = [w_l, w_r];
// Sort using rust sort algorithm
pair.sort();
Expand Down
20 changes: 11 additions & 9 deletions acvm-repo/acvm/src/compiler/optimizers/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -23,20 +23,22 @@ pub fn optimize(acir: Circuit) -> (Circuit, AcirTransformationMap) {
/// Applies [`ProofSystemCompiler`][crate::ProofSystemCompiler] independent optimizations to a [`Circuit`].
pub(super) fn optimize_internal(acir: Circuit) -> (Circuit, AcirTransformationMap) {
// General optimizer pass
let mut opcodes: Vec<Opcode> = Vec::new();
for opcode in acir.opcodes {
match opcode {
Opcode::Arithmetic(arith_expr) => {
opcodes.push(Opcode::Arithmetic(GeneralOptimizer::optimize(arith_expr)));
let opcodes: Vec<Opcode> = acir
.opcodes
.into_iter()
.map(|opcode| {
if let Opcode::Arithmetic(arith_expr) = opcode {
Opcode::Arithmetic(GeneralOptimizer::optimize(arith_expr))
} else {
opcode
}
other_opcode => opcodes.push(other_opcode),
};
}
})
.collect();
let acir = Circuit { opcodes, ..acir };

// Track original acir opcode positions throughout the transformation passes of the compilation
// by applying the modifications done to the circuit opcodes and also to the opcode_positions (delete and insert)
let acir_opcode_positions = acir.opcodes.iter().enumerate().map(|(i, _)| i).collect();
let acir_opcode_positions = (0..acir.opcodes.len()).collect();

// Unused memory optimization pass
let memory_optimizer = UnusedMemoryOptimizer::new(acir);
Expand Down
17 changes: 5 additions & 12 deletions acvm-repo/acvm/src/compiler/optimizers/redundant_range.rs
Original file line number Diff line number Diff line change
Expand Up @@ -74,13 +74,13 @@ impl RangeOptimizer {

let mut new_order_list = Vec::with_capacity(order_list.len());
let mut optimized_opcodes = Vec::with_capacity(self.circuit.opcodes.len());
for (idx, opcode) in self.circuit.opcodes.iter().enumerate() {
let (witness, num_bits) = match extract_range_opcode(opcode) {
for (idx, opcode) in self.circuit.opcodes.into_iter().enumerate() {
let (witness, num_bits) = match extract_range_opcode(&opcode) {
Some(range_opcode) => range_opcode,
None => {
// If its not the range opcode, add it to the opcode
// list and continue;
optimized_opcodes.push(opcode.clone());
optimized_opcodes.push(opcode);
new_order_list.push(order_list[idx]);
continue;
}
Expand All @@ -101,18 +101,11 @@ impl RangeOptimizer {
if is_lowest_bit_size {
already_seen_witness.insert(witness);
new_order_list.push(order_list[idx]);
optimized_opcodes.push(opcode.clone());
optimized_opcodes.push(opcode);
}
}

(
Circuit {
current_witness_index: self.circuit.current_witness_index,
opcodes: optimized_opcodes,
..self.circuit
},
new_order_list,
)
(Circuit { opcodes: optimized_opcodes, ..self.circuit }, new_order_list)
}
}

Expand Down
6 changes: 3 additions & 3 deletions acvm-repo/acvm/src/compiler/optimizers/unused_memory.rs
Original file line number Diff line number Diff line change
Expand Up @@ -42,16 +42,16 @@ impl UnusedMemoryOptimizer {
) -> (Circuit, Vec<usize>) {
let mut new_order_list = Vec::with_capacity(order_list.len());
let mut optimized_opcodes = Vec::with_capacity(self.circuit.opcodes.len());
for (idx, opcode) in self.circuit.opcodes.iter().enumerate() {
for (idx, opcode) in self.circuit.opcodes.into_iter().enumerate() {
match opcode {
Opcode::MemoryInit { block_id, .. }
if self.unused_memory_initializations.contains(block_id) =>
if self.unused_memory_initializations.contains(&block_id) =>
{
// Drop opcode
}
_ => {
new_order_list.push(order_list[idx]);
optimized_opcodes.push(opcode.clone());
optimized_opcodes.push(opcode);
}
}
}
Expand Down
22 changes: 11 additions & 11 deletions acvm-repo/acvm/src/compiler/transformers/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,15 +3,15 @@
native_types::{Expression, Witness},
FieldElement,
};
use indexmap::IndexMap;

Check warning on line 6 in acvm-repo/acvm/src/compiler/transformers/mod.rs

View workflow job for this annotation

GitHub Actions / Spellcheck / Spellcheck

Unknown word (indexmap)

use crate::Language;

mod csat;

Check warning on line 10 in acvm-repo/acvm/src/compiler/transformers/mod.rs

View workflow job for this annotation

GitHub Actions / Spellcheck / Spellcheck

Unknown word (csat)
mod fallback;
mod r1cs;

pub(crate) use csat::CSatTransformer;

Check warning on line 14 in acvm-repo/acvm/src/compiler/transformers/mod.rs

View workflow job for this annotation

GitHub Actions / Spellcheck / Spellcheck

Unknown word (csat)
pub(crate) use fallback::FallbackTransformer;
pub(crate) use r1cs::R1CSTransformer;

Expand Down Expand Up @@ -55,15 +55,15 @@
return Ok((transformer.transform(), transformation_map));
}
crate::Language::PLONKCSat { width } => {
let mut csat = CSatTransformer::new(*width);

Check warning on line 58 in acvm-repo/acvm/src/compiler/transformers/mod.rs

View workflow job for this annotation

GitHub Actions / Spellcheck / Spellcheck

Unknown word (csat)
for value in acir.circuit_arguments() {
csat.mark_solvable(value);

Check warning on line 60 in acvm-repo/acvm/src/compiler/transformers/mod.rs

View workflow job for this annotation

GitHub Actions / Spellcheck / Spellcheck

Unknown word (csat)
}
csat

Check warning on line 62 in acvm-repo/acvm/src/compiler/transformers/mod.rs

View workflow job for this annotation

GitHub Actions / Spellcheck / Spellcheck

Unknown word (csat)
}
};

// TODO: the code below is only for CSAT transformer

Check warning on line 66 in acvm-repo/acvm/src/compiler/transformers/mod.rs

View workflow job for this annotation

GitHub Actions / Spellcheck / Spellcheck

Unknown word (CSAT)
// TODO it may be possible to refactor it in a way that we do not need to return early from the r1cs
// TODO or at the very least, we could put all of it inside of CSatOptimizer pass

Expand All @@ -76,13 +76,13 @@
// maps a normalized expression to the intermediate variable which represents the expression, along with its 'norm'
// the 'norm' is simply the value of the first non zero coefficient in the expression, taken from the linear terms, or quadratic terms if there is none.
let mut intermediate_variables: IndexMap<Expression, (FieldElement, Witness)> = IndexMap::new();
for (index, opcode) in acir.opcodes.iter().enumerate() {
for (index, opcode) in acir.opcodes.into_iter().enumerate() {
match opcode {
Opcode::Arithmetic(arith_expr) => {
let len = intermediate_variables.len();

let arith_expr = transformer.transform(
arith_expr.clone(),
arith_expr,
&mut intermediate_variables,
&mut next_witness_index,
);
Expand All @@ -104,7 +104,7 @@
transformed_opcodes.push(Opcode::Arithmetic(opcode));
}
}
Opcode::BlackBoxFuncCall(func) => {
Opcode::BlackBoxFuncCall(ref func) => {
match func {
acir::circuit::opcodes::BlackBoxFuncCall::AND { output, .. }
| acir::circuit::opcodes::BlackBoxFuncCall::XOR { output, .. } => {
Expand Down Expand Up @@ -146,9 +146,9 @@
}

new_acir_opcode_positions.push(acir_opcode_positions[index]);
transformed_opcodes.push(opcode.clone());
transformed_opcodes.push(opcode);
}
Opcode::Directive(directive) => {
Opcode::Directive(ref directive) => {
match directive {
Directive::Quotient(quotient_directive) => {
transformer.mark_solvable(quotient_directive.q);
Expand All @@ -166,14 +166,14 @@
}
}
new_acir_opcode_positions.push(acir_opcode_positions[index]);
transformed_opcodes.push(opcode.clone());
transformed_opcodes.push(opcode);
}
Opcode::MemoryInit { .. } => {
// `MemoryInit` does not write values to the `WitnessMap`
new_acir_opcode_positions.push(acir_opcode_positions[index]);
transformed_opcodes.push(opcode.clone());
transformed_opcodes.push(opcode);
}
Opcode::MemoryOp { op, .. } => {
Opcode::MemoryOp { ref op, .. } => {
for (_, witness1, witness2) in &op.value.mul_terms {
transformer.mark_solvable(*witness1);
transformer.mark_solvable(*witness2);
Expand All @@ -182,9 +182,9 @@
transformer.mark_solvable(*witness);
}
new_acir_opcode_positions.push(acir_opcode_positions[index]);
transformed_opcodes.push(opcode.clone());
transformed_opcodes.push(opcode);
}
Opcode::Brillig(brillig) => {
Opcode::Brillig(ref brillig) => {
for output in &brillig.outputs {
match output {
BrilligOutputs::Simple(w) => transformer.mark_solvable(*w),
Expand All @@ -196,7 +196,7 @@
}
}
new_acir_opcode_positions.push(acir_opcode_positions[index]);
transformed_opcodes.push(opcode.clone());
transformed_opcodes.push(opcode);
}
}
}
Expand Down
Loading