Skip to content

Commit 3651269

Browse files
TomAFrenchTomAFrench
committed
Merge branch 'master' into tf/pull-acirgen-out-of-ssa
* master: fix: take blackbox function outputs into account when merging expressions (#6532) chore: Add `Instruction::MakeArray` to SSA (#6071) feat(profiler): Reduce memory in Brillig execution flamegraph (#6538) chore: convert some tests to use SSA parser (#6543)
2 parents 81a46d8 + 713df69 commit 3651269

Some content is hidden

Large Commits have some content hidden by default. Use the searchbox below for content that may be hidden.

41 files changed

+896
-627
lines changed

‎acvm-repo/acvm/src/compiler/optimizers/merge_expressions.rs

+60-12
Original file line numberDiff line numberDiff line change
@@ -153,15 +153,18 @@ impl MergeExpressionsOptimizer {
153153

154154
// Returns the input witnesses used by the opcode
155155
fn witness_inputs<F: AcirField>(&self, opcode: &Opcode<F>) -> BTreeSet<Witness> {
156-
let mut witnesses = BTreeSet::new();
157156
match opcode {
158157
Opcode::AssertZero(expr) => CircuitSimulator::expr_wit(expr),
159-
Opcode::BlackBoxFuncCall(bb_func) => bb_func.get_input_witnesses(),
158+
Opcode::BlackBoxFuncCall(bb_func) => {
159+
let mut witnesses = bb_func.get_input_witnesses();
160+
witnesses.extend(bb_func.get_outputs_vec());
161+
162+
witnesses
163+
}
160164
Opcode::Directive(Directive::ToLeRadix { a, .. }) => CircuitSimulator::expr_wit(a),
161165
Opcode::MemoryOp { block_id: _, op, predicate } => {
162166
//index et value, et predicate
163-
let mut witnesses = BTreeSet::new();
164-
witnesses.extend(CircuitSimulator::expr_wit(&op.index));
167+
let mut witnesses = CircuitSimulator::expr_wit(&op.index);
165168
witnesses.extend(CircuitSimulator::expr_wit(&op.value));
166169
if let Some(p) = predicate {
167170
witnesses.extend(CircuitSimulator::expr_wit(p));
@@ -173,6 +176,7 @@ impl MergeExpressionsOptimizer {
173176
init.iter().cloned().collect()
174177
}
175178
Opcode::BrilligCall { inputs, outputs, .. } => {
179+
let mut witnesses = BTreeSet::new();
176180
for i in inputs {
177181
witnesses.extend(self.brillig_input_wit(i));
178182
}
@@ -182,12 +186,9 @@ impl MergeExpressionsOptimizer {
182186
witnesses
183187
}
184188
Opcode::Call { id: _, inputs, outputs, predicate } => {
185-
for i in inputs {
186-
witnesses.insert(*i);
187-
}
188-
for i in outputs {
189-
witnesses.insert(*i);
190-
}
189+
let mut witnesses: BTreeSet<Witness> = BTreeSet::from_iter(inputs.iter().copied());
190+
witnesses.extend(outputs);
191+
191192
if let Some(p) = predicate {
192193
witnesses.extend(CircuitSimulator::expr_wit(p));
193194
}
@@ -235,15 +236,15 @@ mod tests {
235236
acir_field::AcirField,
236237
circuit::{
237238
brillig::{BrilligFunctionId, BrilligOutputs},
238-
opcodes::FunctionInput,
239+
opcodes::{BlackBoxFuncCall, FunctionInput},
239240
Circuit, ExpressionWidth, Opcode, PublicInputs,
240241
},
241242
native_types::{Expression, Witness},
242243
FieldElement,
243244
};
244245
use std::collections::BTreeSet;
245246

246-
fn check_circuit(circuit: Circuit<FieldElement>) {
247+
fn check_circuit(circuit: Circuit<FieldElement>) -> Circuit<FieldElement> {
247248
assert!(CircuitSimulator::default().check_circuit(&circuit));
248249
let mut merge_optimizer = MergeExpressionsOptimizer::new();
249250
let acir_opcode_positions = vec![0; 20];
@@ -253,6 +254,7 @@ mod tests {
253254
optimized_circuit.opcodes = opcodes;
254255
// check that the circuit is still valid after optimization
255256
assert!(CircuitSimulator::default().check_circuit(&optimized_circuit));
257+
optimized_circuit
256258
}
257259

258260
#[test]
@@ -352,4 +354,50 @@ mod tests {
352354
};
353355
check_circuit(circuit);
354356
}
357+
358+
#[test]
359+
fn takes_blackbox_opcode_outputs_into_account() {
360+
// Regression test for https://github.com/noir-lang/noir/issues/6527
361+
// Previously we would not track the usage of witness 4 in the output of the blackbox function.
362+
// We would then merge the final two opcodes losing the check that the brillig call must match
363+
// with `_0 ^ _1`.
364+
365+
let circuit: Circuit<FieldElement> = Circuit {
366+
current_witness_index: 7,
367+
opcodes: vec![
368+
Opcode::BrilligCall {
369+
id: BrilligFunctionId(0),
370+
inputs: Vec::new(),
371+
outputs: vec![BrilligOutputs::Simple(Witness(3))],
372+
predicate: None,
373+
},
374+
Opcode::BlackBoxFuncCall(BlackBoxFuncCall::AND {
375+
lhs: FunctionInput::witness(Witness(0), 8),
376+
rhs: FunctionInput::witness(Witness(1), 8),
377+
output: Witness(4),
378+
}),
379+
Opcode::AssertZero(Expression {
380+
linear_combinations: vec![
381+
(FieldElement::one(), Witness(3)),
382+
(-FieldElement::one(), Witness(4)),
383+
],
384+
..Default::default()
385+
}),
386+
Opcode::AssertZero(Expression {
387+
linear_combinations: vec![
388+
(-FieldElement::one(), Witness(2)),
389+
(FieldElement::one(), Witness(4)),
390+
],
391+
..Default::default()
392+
}),
393+
],
394+
expression_width: ExpressionWidth::Bounded { width: 4 },
395+
private_parameters: BTreeSet::from([Witness(0), Witness(1)]),
396+
return_values: PublicInputs(BTreeSet::from([Witness(2)])),
397+
..Default::default()
398+
};
399+
400+
let new_circuit = check_circuit(circuit.clone());
401+
assert_eq!(circuit, new_circuit);
402+
}
355403
}

‎compiler/noirc_evaluator/src/acir/mod.rs

+7-19
Original file line numberDiff line numberDiff line change
@@ -778,6 +778,12 @@ impl<'a> Context<'a> {
778778
Instruction::IfElse { .. } => {
779779
unreachable!("IfElse instruction remaining in acir-gen")
780780
}
781+
Instruction::MakeArray { elements, typ: _ } => {
782+
let elements = elements.iter().map(|element| self.convert_value(*element, dfg));
783+
let value = AcirValue::Array(elements.collect());
784+
let result = dfg.instruction_results(instruction_id)[0];
785+
self.ssa_values.insert(result, value);
786+
}
781787
}
782788

783789
self.acir_context.set_call_stack(CallStack::new());
@@ -1568,7 +1574,7 @@ impl<'a> Context<'a> {
15681574
if !already_initialized {
15691575
let value = &dfg[array];
15701576
match value {
1571-
Value::Array { .. } | Value::Instruction { .. } => {
1577+
Value::Instruction { .. } => {
15721578
let value = self.convert_value(array, dfg);
15731579
let array_typ = dfg.type_of_value(array);
15741580
let len = if !array_typ.contains_slice_element() {
@@ -1611,13 +1617,6 @@ impl<'a> Context<'a> {
16111617
match array_typ {
16121618
Type::Array(_, _) | Type::Slice(_) => {
16131619
match &dfg[array_id] {
1614-
Value::Array { array, .. } => {
1615-
for (i, value) in array.iter().enumerate() {
1616-
flat_elem_type_sizes.push(
1617-
self.flattened_slice_size(*value, dfg) + flat_elem_type_sizes[i],
1618-
);
1619-
}
1620-
}
16211620
Value::Instruction { .. } | Value::Param { .. } => {
16221621
// An instruction representing the slice means it has been processed previously during ACIR gen.
16231622
// Use the previously defined result of an array operation to fetch the internal type information.
@@ -1750,13 +1749,6 @@ impl<'a> Context<'a> {
17501749
fn flattened_slice_size(&mut self, array_id: ValueId, dfg: &DataFlowGraph) -> usize {
17511750
let mut size = 0;
17521751
match &dfg[array_id] {
1753-
Value::Array { array, .. } => {
1754-
// The array is going to be the flattened outer array
1755-
// Flattened slice size from SSA value does not need to be multiplied by the len
1756-
for value in array {
1757-
size += self.flattened_slice_size(*value, dfg);
1758-
}
1759-
}
17601752
Value::NumericConstant { .. } => {
17611753
size += 1;
17621754
}
@@ -1920,10 +1912,6 @@ impl<'a> Context<'a> {
19201912
Value::NumericConstant { constant, typ } => {
19211913
AcirValue::Var(self.acir_context.add_constant(*constant), typ.into())
19221914
}
1923-
Value::Array { array, .. } => {
1924-
let elements = array.iter().map(|element| self.convert_value(*element, dfg));
1925-
AcirValue::Array(elements.collect())
1926-
}
19271915
Value::Intrinsic(..) => todo!(),
19281916
Value::Function(function_id) => {
19291917
// This conversion is for debugging support only, to allow the

‎compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_block.rs

+40-47
Original file line numberDiff line numberDiff line change
@@ -160,13 +160,9 @@ impl<'block> BrilligBlock<'block> {
160160
);
161161
}
162162
TerminatorInstruction::Return { return_values, .. } => {
163-
let return_registers: Vec<_> = return_values
164-
.iter()
165-
.map(|value_id| {
166-
let return_variable = self.convert_ssa_value(*value_id, dfg);
167-
return_variable.extract_register()
168-
})
169-
.collect();
163+
let return_registers = vecmap(return_values, |value_id| {
164+
self.convert_ssa_value(*value_id, dfg).extract_register()
165+
});
170166
self.brillig_context.codegen_return(&return_registers);
171167
}
172168
}
@@ -763,6 +759,43 @@ impl<'block> BrilligBlock<'block> {
763759
Instruction::IfElse { .. } => {
764760
unreachable!("IfElse instructions should not be possible in brillig")
765761
}
762+
Instruction::MakeArray { elements: array, typ } => {
763+
let value_id = dfg.instruction_results(instruction_id)[0];
764+
if !self.variables.is_allocated(&value_id) {
765+
let new_variable = self.variables.define_variable(
766+
self.function_context,
767+
self.brillig_context,
768+
value_id,
769+
dfg,
770+
);
771+
772+
// Initialize the variable
773+
match new_variable {
774+
BrilligVariable::BrilligArray(brillig_array) => {
775+
self.brillig_context.codegen_initialize_array(brillig_array);
776+
}
777+
BrilligVariable::BrilligVector(vector) => {
778+
let size = self
779+
.brillig_context
780+
.make_usize_constant_instruction(array.len().into());
781+
self.brillig_context.codegen_initialize_vector(vector, size, None);
782+
self.brillig_context.deallocate_single_addr(size);
783+
}
784+
_ => unreachable!(
785+
"ICE: Cannot initialize array value created as {new_variable:?}"
786+
),
787+
};
788+
789+
// Write the items
790+
let items_pointer = self
791+
.brillig_context
792+
.codegen_make_array_or_vector_items_pointer(new_variable);
793+
794+
self.initialize_constant_array(array, typ, dfg, items_pointer);
795+
796+
self.brillig_context.deallocate_register(items_pointer);
797+
}
798+
}
766799
};
767800

768801
let dead_variables = self
@@ -1500,46 +1533,6 @@ impl<'block> BrilligBlock<'block> {
15001533
new_variable
15011534
}
15021535
}
1503-
Value::Array { array, typ } => {
1504-
if self.variables.is_allocated(&value_id) {
1505-
self.variables.get_allocation(self.function_context, value_id, dfg)
1506-
} else {
1507-
let new_variable = self.variables.define_variable(
1508-
self.function_context,
1509-
self.brillig_context,
1510-
value_id,
1511-
dfg,
1512-
);
1513-
1514-
// Initialize the variable
1515-
match new_variable {
1516-
BrilligVariable::BrilligArray(brillig_array) => {
1517-
self.brillig_context.codegen_initialize_array(brillig_array);
1518-
}
1519-
BrilligVariable::BrilligVector(vector) => {
1520-
let size = self
1521-
.brillig_context
1522-
.make_usize_constant_instruction(array.len().into());
1523-
self.brillig_context.codegen_initialize_vector(vector, size, None);
1524-
self.brillig_context.deallocate_single_addr(size);
1525-
}
1526-
_ => unreachable!(
1527-
"ICE: Cannot initialize array value created as {new_variable:?}"
1528-
),
1529-
};
1530-
1531-
// Write the items
1532-
let items_pointer = self
1533-
.brillig_context
1534-
.codegen_make_array_or_vector_items_pointer(new_variable);
1535-
1536-
self.initialize_constant_array(array, typ, dfg, items_pointer);
1537-
1538-
self.brillig_context.deallocate_register(items_pointer);
1539-
1540-
new_variable
1541-
}
1542-
}
15431536
Value::Function(_) => {
15441537
// For the debugger instrumentation we want to allow passing
15451538
// around values representing function pointers, even though

‎compiler/noirc_evaluator/src/brillig/brillig_gen/constant_allocation.rs

+2-3
Original file line numberDiff line numberDiff line change
@@ -89,8 +89,7 @@ impl ConstantAllocation {
8989
}
9090
if let Some(terminator_instruction) = block.terminator() {
9191
terminator_instruction.for_each_value(|value_id| {
92-
let variables = collect_variables_of_value(value_id, &func.dfg);
93-
for variable in variables {
92+
if let Some(variable) = collect_variables_of_value(value_id, &func.dfg) {
9493
record_if_constant(block_id, variable, InstructionLocation::Terminator);
9594
}
9695
});
@@ -166,7 +165,7 @@ impl ConstantAllocation {
166165
}
167166

168167
pub(crate) fn is_constant_value(id: ValueId, dfg: &DataFlowGraph) -> bool {
169-
matches!(&dfg[dfg.resolve(id)], Value::NumericConstant { .. } | Value::Array { .. })
168+
matches!(&dfg[dfg.resolve(id)], Value::NumericConstant { .. })
170169
}
171170

172171
/// For a given function, finds all the blocks that are within loops

‎compiler/noirc_evaluator/src/brillig/brillig_gen/variable_liveness.rs

+7-20
Original file line numberDiff line numberDiff line change
@@ -45,32 +45,19 @@ fn find_back_edges(
4545
}
4646

4747
/// Collects the underlying variables inside a value id. It might be more than one, for example in constant arrays that are constructed with multiple vars.
48-
pub(crate) fn collect_variables_of_value(value_id: ValueId, dfg: &DataFlowGraph) -> Vec<ValueId> {
48+
pub(crate) fn collect_variables_of_value(
49+
value_id: ValueId,
50+
dfg: &DataFlowGraph,
51+
) -> Option<ValueId> {
4952
let value_id = dfg.resolve(value_id);
5053
let value = &dfg[value_id];
5154

5255
match value {
53-
Value::Instruction { .. } | Value::Param { .. } => {
54-
vec![value_id]
55-
}
56-
// Literal arrays are constants, but might use variable values to initialize.
57-
Value::Array { array, .. } => {
58-
let mut value_ids = vec![value_id];
59-
60-
array.iter().for_each(|item_id| {
61-
let underlying_ids = collect_variables_of_value(*item_id, dfg);
62-
value_ids.extend(underlying_ids);
63-
});
64-
65-
value_ids
66-
}
67-
Value::NumericConstant { .. } => {
68-
vec![value_id]
56+
Value::Instruction { .. } | Value::Param { .. } | Value::NumericConstant { .. } => {
57+
Some(value_id)
6958
}
7059
// Functions are not variables in a defunctionalized SSA. Only constant function values should appear.
71-
Value::ForeignFunction(_) | Value::Function(_) | Value::Intrinsic(..) => {
72-
vec![]
73-
}
60+
Value::ForeignFunction(_) | Value::Function(_) | Value::Intrinsic(..) => None,
7461
}
7562
}
7663

‎compiler/noirc_evaluator/src/brillig/brillig_ir/codegen_stack.rs

+2
Original file line numberDiff line numberDiff line change
@@ -47,13 +47,15 @@ impl<F: AcirField + DebugToString, Registers: RegisterAllocator> BrilligContext<
4747
let destinations_of_temp = movements_map.remove(first_source).unwrap();
4848
movements_map.insert(temp_register, destinations_of_temp);
4949
}
50+
5051
// After removing loops we should have an DAG with each node having only one ancestor (but could have multiple successors)
5152
// Now we should be able to move the registers just by performing a DFS on the movements map
5253
let heads: Vec<_> = movements_map
5354
.keys()
5455
.filter(|source| !destinations_set.contains(source))
5556
.copied()
5657
.collect();
58+
5759
for head in heads {
5860
self.perform_movements(&movements_map, head);
5961
}

‎compiler/noirc_evaluator/src/ssa/checks/check_for_underconstrained_values.rs

+3-3
Original file line numberDiff line numberDiff line change
@@ -191,7 +191,8 @@ impl Context {
191191
| Instruction::Load { .. }
192192
| Instruction::Not(..)
193193
| Instruction::Store { .. }
194-
| Instruction::Truncate { .. } => {
194+
| Instruction::Truncate { .. }
195+
| Instruction::MakeArray { .. } => {
195196
self.value_sets.push(instruction_arguments_and_results);
196197
}
197198

@@ -247,8 +248,7 @@ impl Context {
247248
Value::ForeignFunction(..) => {
248249
panic!("Should not be able to reach foreign function from non-brillig functions, {func_id} in function {}", function.name());
249250
}
250-
Value::Array { .. }
251-
| Value::Instruction { .. }
251+
Value::Instruction { .. }
252252
| Value::NumericConstant { .. }
253253
| Value::Param { .. } => {
254254
panic!("At the point we are running disconnect there shouldn't be any other values as arguments")

0 commit comments

Comments
 (0)