diff --git a/compiler/noirc_evaluator/src/acir/arrays.rs b/compiler/noirc_evaluator/src/acir/arrays.rs index 418653b5aef..a1a227b2279 100644 --- a/compiler/noirc_evaluator/src/acir/arrays.rs +++ b/compiler/noirc_evaluator/src/acir/arrays.rs @@ -377,15 +377,15 @@ impl Context<'_> { /// We need to properly setup the inputs for array operations in ACIR. /// From the original SSA values we compute the following AcirVars: - /// - new_index is the index of the array. ACIR memory operations work with a flat memory, so we fully flattened the specified index + /// - `index_var` is the index of the array. ACIR memory operations work with a flat memory, so we fully flattened the specified index /// in case we have a nested array. The index for SSA array operations only represents the flattened index of the current array. /// Thus internal array element type sizes need to be computed to accurately transform the index. /// - /// - predicate_index is offset, or the index if the predicate is true + /// - If the predicate is known to be true or the array access is guaranteed to be safe, we can directly return `index_var` + /// Otherwise, `predicate_index` is a fallback offset set by [Self::predicated_index]. /// - /// - new_value is the optional value when the operation is an array_set - /// When there is a predicate, it is predicate*value + (1-predicate)*dummy, where dummy is the value of the array at the requested index. - /// It is a dummy value because in the case of a false predicate, the value stored at the requested index will be itself. + /// - `new_value` is the optional value when the operation is an array_set. + /// The value used in an array_set is also dependent upon the predicate and is set in [Self::predicated_store_value] fn convert_array_operation_inputs( &mut self, array_id: ValueId, @@ -400,42 +400,57 @@ impl Context<'_> { let index_var = self.convert_numeric_value(index, dfg)?; let index_var = self.get_flattened_index(&array_typ, array_id, index_var, dfg)?; - let predicate_index = if dfg.is_safe_index(index, array_id) { - index_var - } else { - // index*predicate + (1-predicate)*offset - let offset = self.acir_context.add_constant(offset); - let sub = self.acir_context.sub_var(index_var, offset)?; - let pred = self.acir_context.mul_var(sub, self.current_side_effects_enabled_var)?; - self.acir_context.add_var(pred, offset)? - }; + // Side-effects are always enabled so we do not need to do any predication + if self.acir_context.is_constant_one(&self.current_side_effects_enabled_var) { + let store_value = store_value.map(|store| self.convert_value(store, dfg)); + return Ok((index_var, store_value)); + } - let new_value = if let Some(store) = store_value { - let store_value = self.convert_value(store, dfg); - if self.acir_context.is_constant_one(&self.current_side_effects_enabled_var) { - Some(store_value) - } else { - let store_type = dfg.type_of_value(store); + let predicate_index = self.predicated_index(index_var, index, array_id, dfg, offset)?; - let mut dummy_predicate_index = predicate_index; - // We must setup the dummy value to match the type of the value we wish to store - let dummy = - self.array_get_value(&store_type, block_id, &mut dummy_predicate_index)?; + // Handle the predicated store value + let new_value = store_value + .map(|store| self.predicated_store_value(store, dfg, block_id, predicate_index)) + .transpose()?; - Some(self.convert_array_set_store_value(&store_value, &dummy)?) - } - } else { - None - }; + Ok((predicate_index, new_value)) + } - let new_index = if self.acir_context.is_constant_one(&self.current_side_effects_enabled_var) - { - index_var + /// Computes the predicated index for an array access. + /// If the index is always safe, it is returned directly. + /// Otherwise, we compute `predicate * index + (1 - predicate) * offset`. + fn predicated_index( + &mut self, + index_var: AcirVar, + index: ValueId, + array_id: ValueId, + dfg: &DataFlowGraph, + offset: usize, + ) -> Result { + if dfg.is_safe_index(index, array_id) { + Ok(index_var) } else { - predicate_index - }; + let offset = self.acir_context.add_constant(offset); + let sub = self.acir_context.sub_var(index_var, offset)?; + let pred = self.acir_context.mul_var(sub, self.current_side_effects_enabled_var)?; + self.acir_context.add_var(pred, offset) + } + } - Ok((new_index, new_value)) + /// When there is a predicate, the store value is predicate*value + (1-predicate)*dummy, where dummy is the value of the array at the requested index. + /// It is a dummy value because in the case of a false predicate, the value stored at the requested index will be itself. + fn predicated_store_value( + &mut self, + store: ValueId, + dfg: &DataFlowGraph, + block_id: BlockId, + mut dummy_predicate_index: AcirVar, + ) -> Result { + let store_value = self.convert_value(store, dfg); + let store_type = dfg.type_of_value(store); + // We must setup the dummy value to match the type of the value we wish to store + let dummy = self.array_get_value(&store_type, block_id, &mut dummy_predicate_index)?; + self.convert_array_set_store_value(&store_value, &dummy) } fn convert_array_set_store_value( diff --git a/compiler/noirc_evaluator/src/acir/call.rs b/compiler/noirc_evaluator/src/acir/call.rs index 611b5e4af48..cc7a6cbaccb 100644 --- a/compiler/noirc_evaluator/src/acir/call.rs +++ b/compiler/noirc_evaluator/src/acir/call.rs @@ -3,7 +3,9 @@ use acvm::{AcirField, FieldElement}; use iter_extended::vecmap; use crate::acir::AcirVar; +use crate::brillig::brillig_gen::brillig_fn::FunctionContext as BrilligFunctionContext; use crate::brillig::brillig_gen::gen_brillig_for; +use crate::brillig::brillig_ir::artifact::BrilligParameter; use crate::errors::{RuntimeError, SsaReport}; use crate::ssa::ir::instruction::Hint; use crate::ssa::ir::value::Value; @@ -170,6 +172,40 @@ impl Context<'_> { self.handle_ssa_call_outputs(result_ids, output_values, dfg) } + pub(super) fn gen_brillig_parameters( + &self, + values: &[ValueId], + dfg: &DataFlowGraph, + ) -> Vec { + values + .iter() + .map(|&value_id| { + let typ = dfg.type_of_value(value_id); + if let Type::Slice(item_types) = typ { + let len = match self + .ssa_values + .get(&value_id) + .expect("ICE: Unknown slice input to brillig") + { + AcirValue::DynamicArray(AcirDynamicArray { len, .. }) => *len, + AcirValue::Array(array) => array.len(), + _ => unreachable!("ICE: Slice value is not an array"), + }; + + BrilligParameter::Slice( + item_types + .iter() + .map(BrilligFunctionContext::ssa_type_to_parameter) + .collect(), + len / item_types.len(), + ) + } else { + BrilligFunctionContext::ssa_type_to_parameter(&typ) + } + }) + .collect() + } + /// Returns a vector of `AcirVar`s constrained to be result of the function call. /// /// The function being called is required to be intrinsic. diff --git a/compiler/noirc_evaluator/src/acir/mod.rs b/compiler/noirc_evaluator/src/acir/mod.rs index 47ab91c19ea..f30dee857fb 100644 --- a/compiler/noirc_evaluator/src/acir/mod.rs +++ b/compiler/noirc_evaluator/src/acir/mod.rs @@ -27,11 +27,8 @@ pub(crate) mod ssa; mod tests; mod types; +use crate::brillig::Brillig; use crate::brillig::brillig_gen::gen_brillig_for; -use crate::brillig::{ - Brillig, brillig_gen::brillig_fn::FunctionContext as BrilligFunctionContext, - brillig_ir::artifact::BrilligParameter, -}; use crate::errors::{InternalError, InternalWarning, RuntimeError, SsaReport}; use crate::ssa::{ function_builder::data_bus::DataBus, @@ -587,40 +584,6 @@ impl<'a> Context<'a> { Ok(warnings) } - fn gen_brillig_parameters( - &self, - values: &[ValueId], - dfg: &DataFlowGraph, - ) -> Vec { - values - .iter() - .map(|&value_id| { - let typ = dfg.type_of_value(value_id); - if let Type::Slice(item_types) = typ { - let len = match self - .ssa_values - .get(&value_id) - .expect("ICE: Unknown slice input to brillig") - { - AcirValue::DynamicArray(AcirDynamicArray { len, .. }) => *len, - AcirValue::Array(array) => array.len(), - _ => unreachable!("ICE: Slice value is not an array"), - }; - - BrilligParameter::Slice( - item_types - .iter() - .map(BrilligFunctionContext::ssa_type_to_parameter) - .collect(), - len / item_types.len(), - ) - } else { - BrilligFunctionContext::ssa_type_to_parameter(&typ) - } - }) - .collect() - } - /// Remember the result of an instruction returning a single value fn define_result( &mut self, diff --git a/compiler/noirc_evaluator/src/acir/tests/arrays.rs b/compiler/noirc_evaluator/src/acir/tests/arrays.rs new file mode 100644 index 00000000000..f45df4b61d8 --- /dev/null +++ b/compiler/noirc_evaluator/src/acir/tests/arrays.rs @@ -0,0 +1,269 @@ +use acvm::{acir::circuit::Opcode, assert_circuit_snapshot}; + +use crate::acir::tests::ssa_to_acir_program; + +#[test] +fn does_not_generate_memory_blocks_without_dynamic_accesses() { + let src = " + acir(inline) fn main f0 { + b0(v0: [Field; 2]): + v2, v3 = call as_slice(v0) -> (u32, [Field]) + call f1(u32 2, v3) + v7 = array_get v0, index u32 0 -> Field + constrain v7 == Field 0 + return + } + + brillig(inline) fn foo f1 { + b0(v0: u32, v1: [Field]): + return + } + "; + let program = ssa_to_acir_program(src); + + // Check that no memory opcodes were emitted. + assert_eq!(program.functions.len(), 1); + for opcode in &program.functions[0].opcodes { + assert!(!matches!(opcode, Opcode::MemoryInit { .. })); + } +} + +#[test] +fn constant_array_access_out_of_bounds() { + let src = " + acir(inline) fn main f0 { + b0(): + v2 = make_array [Field 0, Field 1] : [Field; 2] + v4 = array_get v2, index u32 5 -> Field + constrain v4 == Field 0 + return + } + "; + let program = ssa_to_acir_program(src); + + // We expect a constant array access that is out of bounds (OOB) to be deferred to the runtime. + // This means memory checks will be laid down and array access OOB checks will be handled there. + assert_circuit_snapshot!(program, @r" + func 0 + current witness: w3 + private parameters: [] + public parameters: [] + return values: [] + EXPR [ (-1, w0) 0 ] + EXPR [ (-1, w1) 1 ] + INIT (id: 0, len: 2, witnesses: [w0, w1]) + EXPR [ (-1, w2) 5 ] + MEM (id: 0, read at: EXPR [ (1, w2) 0 ], value: EXPR [ (1, w3) 0 ]) + EXPR [ (1, w3) 0 ] + "); +} + +#[test] +fn constant_array_access_in_bounds() { + let src = " + acir(inline) fn main f0 { + b0(): + v2 = make_array [Field 0, Field 1] : [Field; 2] + v4 = array_get v2, index u32 0 -> Field + constrain v4 == Field 0 + return + } + "; + let program = ssa_to_acir_program(src); + + // We know the circuit above to be trivially true + assert_eq!(program.functions.len(), 1); + assert_eq!(program.functions[0].opcodes.len(), 0); +} + +#[test] +fn generates_memory_op_for_dynamic_read() { + let src = " + acir(inline) fn main f0 { + b0(v0: [Field; 3], v1: u32): + v2 = array_get v0, index v1 -> Field + constrain v2 == Field 10 + return + } + "; + + let program = ssa_to_acir_program(src); + + assert_circuit_snapshot!(program, @r" + func 0 + current witness: w4 + private parameters: [w0, w1, w2, w3] + public parameters: [] + return values: [] + INIT (id: 0, len: 3, witnesses: [w0, w1, w2]) + MEM (id: 0, read at: EXPR [ (1, w3) 0 ], value: EXPR [ (1, w4) 0 ]) + EXPR [ (1, w4) -10 ] + "); +} + +#[test] +fn generates_memory_op_for_dynamic_write() { + let src = " + acir(inline) fn main f0 { + b0(v0: [Field; 3], v1: u32): + v2 = array_set v0, index v1, value Field 10 + return v2 + } + "; + let program = ssa_to_acir_program(src); + + // All logic after the write is expected as we generate new witnesses for return values + assert_circuit_snapshot!(program, @r" + func 0 + current witness: w13 + private parameters: [w0, w1, w2, w3] + public parameters: [] + return values: [w4, w5, w6] + INIT (id: 1, len: 3, witnesses: [w0, w1, w2]) + EXPR [ (-1, w7) 10 ] + MEM (id: 1, write EXPR [ (1, w7) 0 ] at: EXPR [ (1, w3) 0 ]) + EXPR [ (-1, w8) 0 ] + MEM (id: 1, read at: EXPR [ (1, w8) 0 ], value: EXPR [ (1, w9) 0 ]) + EXPR [ (-1, w10) 1 ] + MEM (id: 1, read at: EXPR [ (1, w10) 0 ], value: EXPR [ (1, w11) 0 ]) + EXPR [ (-1, w12) 2 ] + MEM (id: 1, read at: EXPR [ (1, w12) 0 ], value: EXPR [ (1, w13) 0 ]) + EXPR [ (1, w4) (-1, w9) 0 ] + EXPR [ (1, w5) (-1, w11) 0 ] + EXPR [ (1, w6) (-1, w13) 0 ] + "); +} + +#[test] +fn generates_predicated_index_for_dynamic_read() { + let src = " + acir(inline) fn main f0 { + b0(v0: [Field; 3], v1: u32, predicate: bool): + enable_side_effects predicate + v3 = array_get v0, index v1 -> Field + constrain v3 == Field 10 + return + } + "; + let program = ssa_to_acir_program(src); + + // w0, w1, w2 represents the array + // So w3 represents our index and w4 is our predicate + // We can see that before the read we have `EXPR [ (1, w3, w4) (-1, w5) 0 ]` + // As the index is zero this is a simplified version of `index*predicate + (1-predicate)*offset` + // w5 is then used as the index which we use to read from the memory block + assert_circuit_snapshot!(program, @r" + func 0 + current witness: w6 + private parameters: [w0, w1, w2, w3, w4] + public parameters: [] + return values: [] + INIT (id: 0, len: 3, witnesses: [w0, w1, w2]) + BLACKBOX::RANGE [w3]:32 bits [] + BLACKBOX::RANGE [w4]:1 bits [] + EXPR [ (1, w3, w4) (-1, w5) 0 ] + MEM (id: 0, read at: EXPR [ (1, w5) 0 ], value: EXPR [ (1, w6) 0 ]) + EXPR [ (1, w6) -10 ] + "); +} + +#[test] +fn generates_predicated_index_and_dummy_value_for_dynamic_write() { + let src = " + acir(inline) fn main f0 { + b0(v0: [Field; 3], v1: u32, predicate: bool): + enable_side_effects predicate + v3 = array_set v0, index v1, value Field 10 + return v3 + } + "; + let program = ssa_to_acir_program(src); + + // Similar to the `generates_predicated_index_for_dynamic_read` test we can + // see how `EXPR [ (1, w3, w4) (-1, w8) 0 ]` forms our predicated index. + // However, now we also have extra logic for generating a dummy value. + // The original value we want to write is `Field 10` and our predicate is `w4`. + // We read the value at the predicated index into `w9`. This is our dummy value. + // We can then see how we form our new store value with: + // `EXPR [ (-1, w4, w9) (10, w4) (1, w9) (-1, w10) 0 ]` -> (predicate*value + (1-predicate)*dummy) + // `(10, w4)` -> predicate*value + // `(-1, w4, w9)` -> (-predicate * dummy) + // `(1, w9)` -> dummy + // As expected, we then store `w10` at the predicated index `w8`. + assert_circuit_snapshot!(program, @r" + func 0 + current witness: w16 + private parameters: [w0, w1, w2, w3, w4] + public parameters: [] + return values: [w5, w6, w7] + INIT (id: 0, len: 3, witnesses: [w0, w1, w2]) + BLACKBOX::RANGE [w3]:32 bits [] + BLACKBOX::RANGE [w4]:1 bits [] + EXPR [ (1, w3, w4) (-1, w8) 0 ] + MEM (id: 0, read at: EXPR [ (1, w8) 0 ], value: EXPR [ (1, w9) 0 ]) + INIT (id: 1, len: 3, witnesses: [w0, w1, w2]) + EXPR [ (-1, w4, w9) (10, w4) (1, w9) (-1, w10) 0 ] + MEM (id: 1, write EXPR [ (1, w10) 0 ] at: EXPR [ (1, w8) 0 ]) + EXPR [ (-1, w11) 0 ] + MEM (id: 1, read at: EXPR [ (1, w11) 0 ], value: EXPR [ (1, w12) 0 ]) + EXPR [ (-1, w13) 1 ] + MEM (id: 1, read at: EXPR [ (1, w13) 0 ], value: EXPR [ (1, w14) 0 ]) + EXPR [ (-1, w15) 2 ] + MEM (id: 1, read at: EXPR [ (1, w15) 0 ], value: EXPR [ (1, w16) 0 ]) + EXPR [ (1, w5) (-1, w12) 0 ] + EXPR [ (1, w6) (-1, w14) 0 ] + EXPR [ (1, w7) (-1, w16) 0 ] + "); +} + +#[test] +fn zero_length_array_constant() { + let src = " + acir(inline) fn main f0 { + b0(): + v0 = make_array [] : [Field; 0] + v2 = array_get v0, index u32 0 -> Field + constrain v2 == Field 0 + return + } + "; + let program = ssa_to_acir_program(src); + + // As we have a constant array the constraint we insert will be simplified down. + // We expect ever expression to equal zero when executed. Thus, this circuit will always fail. + assert_circuit_snapshot!(program, @r" + func 0 + current witness: w0 + private parameters: [] + public parameters: [] + return values: [] + EXPR [ 1 ] + "); +} + +#[test] +fn zero_length_array_dynamic_predicate() { + let src = " + acir(inline) fn main f0 { + b0(predicate: bool): + enable_side_effects predicate + v0 = make_array [] : [Field; 0] + v2 = array_get v0, index u32 0 -> Field + constrain v2 == Field 0 + return + } + "; + let program = ssa_to_acir_program(src); + + // Similar to the `zero_length_array_constant` test we inserted an always failing constraint + // when an array access is attempted on a zero length array. + // However, we must gate it by the predicate in case the branch is inactive. + assert_circuit_snapshot!(program, @r" + func 0 + current witness: w0 + private parameters: [w0] + public parameters: [] + return values: [] + EXPR [ (1, w0) 0 ] + "); +} diff --git a/compiler/noirc_evaluator/src/acir/tests/mod.rs b/compiler/noirc_evaluator/src/acir/tests/mod.rs index 3a43b6394e4..49620a12b2b 100644 --- a/compiler/noirc_evaluator/src/acir/tests/mod.rs +++ b/compiler/noirc_evaluator/src/acir/tests/mod.rs @@ -23,6 +23,7 @@ use crate::{ }; use proptest::prelude::*; +mod arrays; mod brillig_call; mod call; mod intrinsics; @@ -98,91 +99,6 @@ fn unchecked_mul_should_not_have_range_check() { "); } -#[test] -fn does_not_generate_memory_blocks_without_dynamic_accesses() { - let src = " - acir(inline) fn main f0 { - b0(v0: [Field; 2]): - v2, v3 = call as_slice(v0) -> (u32, [Field]) - call f1(u32 2, v3) - v7 = array_get v0, index u32 0 -> Field - constrain v7 == Field 0 - return - } - - brillig(inline) fn foo f1 { - b0(v0: u32, v1: [Field]): - return - } - "; - let program = ssa_to_acir_program(src); - println!("{program}"); - - // Check that no memory opcodes were emitted. - assert_circuit_snapshot!(program, @r" - func 0 - current witness: w1 - private parameters: [w0, w1] - public parameters: [] - return values: [] - BRILLIG CALL func 0: inputs: [EXPR [ 2 ], [EXPR [ (1, w0) 0 ], EXPR [ (1, w1) 0 ]]], outputs: [] - EXPR [ (1, w0) 0 ] - - unconstrained func 0 - 0: @2 = const u32 1 - 1: @1 = const u32 32839 - 2: @0 = const u32 3 - 3: sp[3] = const u32 3 - 4: sp[4] = const u32 0 - 5: @32836 = calldata copy [sp[4]; sp[3]] - 6: @32836 = cast @32836 to u32 - 7: sp[1] = @32836 - 8: sp[2] = const u32 32837 - 9: sp[4] = const u32 2 - 10: sp[6] = const u32 3 - 11: sp[5] = u32 add sp[4], sp[6] - 12: sp[3] = @1 - 13: @1 = u32 add @1, sp[5] - 14: sp[3] = indirect const u32 1 - 15: sp[5] = u32 add sp[3], @2 - 16: store sp[4] at sp[5] - 17: sp[5] = u32 add sp[5], @2 - 18: store sp[4] at sp[5] - 19: sp[6] = const u32 3 - 20: sp[5] = u32 add sp[3], sp[6] - 21: @32771 = sp[2] - 22: @32772 = sp[5] - 23: @32773 = sp[4] - 24: call 31 - 25: sp[2] = sp[3] - 26: call 42 - 27: call 43 - 28: sp[1] = const u32 32839 - 29: sp[2] = const u32 0 - 30: stop &[sp[1]; sp[2]] - 31: @32775 = u32 add @32771, @32773 - 32: @32776 = @32771 - 33: @32777 = @32772 - 34: @32778 = u32 eq @32776, @32775 - 35: jump if @32778 to 41 - 36: @32774 = load @32776 - 37: store @32774 at @32777 - 38: @32776 = u32 add @32776, @2 - 39: @32777 = u32 add @32777, @2 - 40: jump to 34 - 41: return - 42: return - 43: call 45 - 44: return - 45: @32772 = const u32 30720 - 46: @32771 = u32 lt @0, @32772 - 47: jump if @32771 to 50 - 48: @1 = indirect const u64 15764276373176857197 - 49: trap &[@1; @2] - 50: return - "); -} - #[test] fn properly_constrains_quotient_when_truncating_fields() { let src = "