diff --git a/Cargo.toml b/Cargo.toml index 5ecf465ef05..7345bdab230 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -61,4 +61,4 @@ tower = "0.4" url = "2.2.0" wasm-bindgen = { version = "=0.2.86", features = ["serde-serialize"] } wasm-bindgen-test = "0.3.33" -base64 = "0.21.2" +base64 = "0.21.2" \ No newline at end of file diff --git a/crates/nargo_cli/tests/execution_success/slice_dynamic_index/Nargo.toml b/crates/nargo_cli/tests/execution_success/slice_dynamic_index/Nargo.toml new file mode 100644 index 00000000000..08322784151 --- /dev/null +++ b/crates/nargo_cli/tests/execution_success/slice_dynamic_index/Nargo.toml @@ -0,0 +1,7 @@ +[package] +name = "slice_dynamic_index" +type = "bin" +authors = [""] +compiler_version = "0.10.3" + +[dependencies] \ No newline at end of file diff --git a/crates/nargo_cli/tests/execution_success/slice_dynamic_index/Prover.toml b/crates/nargo_cli/tests/execution_success/slice_dynamic_index/Prover.toml new file mode 100644 index 00000000000..0e5dfd5638d --- /dev/null +++ b/crates/nargo_cli/tests/execution_success/slice_dynamic_index/Prover.toml @@ -0,0 +1 @@ +x = "5" diff --git a/crates/nargo_cli/tests/execution_success/slice_dynamic_index/src/main.nr b/crates/nargo_cli/tests/execution_success/slice_dynamic_index/src/main.nr new file mode 100644 index 00000000000..48229a0ced3 --- /dev/null +++ b/crates/nargo_cli/tests/execution_success/slice_dynamic_index/src/main.nr @@ -0,0 +1,222 @@ +fn main(x : Field) { + // The parameters to this function must come directly from witness values (inputs to main). + regression_dynamic_slice_index(x - 1, x - 4); +} + +fn regression_dynamic_slice_index(x: Field, y: Field) { + let mut slice = []; + for i in 0..5 { + slice = slice.push_back(i); + } + assert(slice.len() == 5); + + dynamic_slice_index_set_if(slice, x, y); + dynamic_slice_index_set_else(slice, x, y); + dynamic_slice_index_set_nested_if_else_else(slice, x, y); + dynamic_slice_index_set_nested_if_else_if(slice, x, y + 1); + dynamic_slice_index_if(slice, x); + dynamic_array_index_if([0, 1, 2, 3, 4], x); + dynamic_slice_index_else(slice, x); + + dynamic_slice_merge_if(slice, x); + dynamic_slice_merge_else(slice, x); +} + +fn dynamic_slice_index_set_if(mut slice: [Field], x: Field, y: Field) { + assert(slice[x] == 4); + assert(slice[y] == 1); + slice[y] = 0; + assert(slice[x] == 4); + assert(slice[1] == 0); + if x as u32 < 10 { + assert(slice[x] == 4); + slice[x] = slice[x] - 2; + slice[x - 1] = slice[x]; + } else { + slice[x] = 0; + } + assert(slice[3] == 2); + assert(slice[4] == 2); +} + +fn dynamic_slice_index_set_else(mut slice: [Field], x: Field, y: Field) { + assert(slice[x] == 4); + assert(slice[y] == 1); + slice[y] = 0; + assert(slice[x] == 4); + assert(slice[1] == 0); + if x as u32 > 10 { + assert(slice[x] == 4); + slice[x] = slice[x] - 2; + slice[x - 1] = slice[x]; + } else { + slice[x] = 0; + } + assert(slice[4] == 0); +} + +// This tests the case of missing a store instruction in the else branch +// of merging slices +fn dynamic_slice_index_if(mut slice: [Field], x: Field) { + if x as u32 < 10 { + assert(slice[x] == 4); + slice[x] = slice[x] - 2; + } else { + assert(slice[x] == 0); + } + assert(slice[4] == 2); +} + +fn dynamic_array_index_if(mut array: [Field; 5], x: Field) { + if x as u32 < 10 { + assert(array[x] == 4); + array[x] = array[x] - 2; + } else { + assert(array[x] == 0); + } + assert(array[4] == 2); +} + +// This tests the case of missing a store instruction in the then branch +// of merging slices +fn dynamic_slice_index_else(mut slice: [Field], x: Field) { + if x as u32 > 10 { + assert(slice[x] == 0); + } else { + assert(slice[x] == 4); + slice[x] = slice[x] - 2; + } + assert(slice[4] == 2); +} + + +fn dynamic_slice_merge_if(mut slice: [Field], x: Field) { + if x as u32 < 10 { + assert(slice[x] == 4); + slice[x] = slice[x] - 2; + + slice = slice.push_back(10); + // Having an array set here checks whether we appropriately + // handle a slice length that is not yet resolving to a constant + // during flattening + slice[x] = 10; + assert(slice[slice.len() - 1] == 10); + assert(slice.len() == 6); + + slice[x] = 20; + slice[x] = slice[x] + 10; + + slice = slice.push_front(11); + assert(slice[0] == 11); + assert(slice.len() == 7); + assert(slice[5] == 30); + + slice = slice.push_front(12); + assert(slice[0] == 12); + assert(slice.len() == 8); + assert(slice[6] == 30); + + let (popped_slice, last_elem) = slice.pop_back(); + assert(last_elem == 10); + assert(popped_slice.len() == 7); + + let (first_elem, rest_of_slice) = popped_slice.pop_front(); + assert(first_elem == 12); + assert(rest_of_slice.len() == 6); + + // TODO(#2462): SliceInsert and SliceRemove with a dynamic index are not yet implemented in ACIR gen + slice = rest_of_slice.insert(2, 20); + assert(slice[2] == 20); + assert(slice[6] == 30); + assert(slice.len() == 7); + + // TODO(#2462): SliceInsert and SliceRemove with a dynamic index are not yet implemented in ACIR gen + let (removed_slice, removed_elem) = slice.remove(3); + // The deconstructed tuple assigns to the slice but is not seen outside of the if statement + // without a direct assignment + slice = removed_slice; + + assert(removed_elem == 1); + assert(slice.len() == 6); + } else { + assert(slice[x] == 0); + slice = slice.push_back(20); + } + + assert(slice.len() == 6); + assert(slice[slice.len() - 1] == 30); +} + +fn dynamic_slice_merge_else(mut slice: [Field], x: Field) { + if x as u32 > 10 { + assert(slice[x] == 0); + slice[x] = 2; + } else { + assert(slice[x] == 4); + slice[x] = slice[x] - 2; + slice = slice.push_back(10); + } + assert(slice.len() == 6); + assert(slice[slice.len() - 1] == 10); + + slice = slice.push_back(20); + assert(slice.len() == 7); + assert(slice[slice.len() - 1] == 20); +} + +fn dynamic_slice_index_set_nested_if_else_else(mut slice: [Field], x: Field, y: Field) { + assert(slice[x] == 4); + assert(slice[y] == 1); + slice[y] = 0; + assert(slice[x] == 4); + assert(slice[1] == 0); + if x as u32 < 10 { + slice[x] = slice[x] - 2; + if y != 1 { + slice[x] = slice[x] + 20; + } else { + if x == 5 { + // We should not hit this case + assert(slice[x] == 22); + } else { + slice[x] = 10; + slice = slice.push_back(15); + assert(slice.len() == 6); + } + assert(slice[4] == 10); + } + } else { + slice[x] = 0; + } + assert(slice[4] == 10); + assert(slice.len() == 6); + assert(slice[slice.len() - 1] == 15); + + slice = slice.push_back(20); + assert(slice.len() == 7); + assert(slice[slice.len() - 1] == 20); +} + +fn dynamic_slice_index_set_nested_if_else_if(mut slice: [Field], x: Field, y: Field) { + assert(slice[x] == 4); + assert(slice[y] == 2); + slice[y] = 0; + assert(slice[x] == 4); + assert(slice[2] == 0); + if x as u32 < 10 { + slice[x] = slice[x] - 2; + // TODO: this panics as we have a load for the slice in flattening + if y == 1 { + slice[x] = slice[x] + 20; + } else { + if x == 4 { + slice[x] = 5; + } + assert(slice[4] == 5); + } + } else { + slice[x] = 0; + } + assert(slice[4] == 5); +} + diff --git a/crates/nargo_cli/tests/execution_success/slices/src/main.nr b/crates/nargo_cli/tests/execution_success/slices/src/main.nr index 8fbf0a19fc5..26cf173a253 100644 --- a/crates/nargo_cli/tests/execution_success/slices/src/main.nr +++ b/crates/nargo_cli/tests/execution_success/slices/src/main.nr @@ -1,5 +1,6 @@ use dep::std::slice; use dep::std; + fn main(x : Field, y : pub Field) { let mut slice = [0; 2]; assert(slice[0] == 0); @@ -155,4 +156,4 @@ fn merge_slices_mutate_in_loop(x: Field, y: Field) -> [Field] { slice = slice.push_back(x); } slice -} +} \ No newline at end of file diff --git a/crates/noirc_evaluator/src/brillig/brillig_gen/brillig_block.rs b/crates/noirc_evaluator/src/brillig/brillig_gen/brillig_block.rs index a62c8e72498..2e31f618e8f 100644 --- a/crates/noirc_evaluator/src/brillig/brillig_gen/brillig_block.rs +++ b/crates/noirc_evaluator/src/brillig/brillig_gen/brillig_block.rs @@ -492,7 +492,7 @@ impl<'block> BrilligBlock<'block> { destination_variable, ); } - Instruction::ArraySet { array, index, value } => { + Instruction::ArraySet { array, index, value, .. } => { let source_variable = self.convert_ssa_value(*array, dfg); let index_register = self.convert_ssa_register_value(*index, dfg); let value_variable = self.convert_ssa_value(*value, dfg); diff --git a/crates/noirc_evaluator/src/errors.rs b/crates/noirc_evaluator/src/errors.rs index d7d04d521c5..da2a09d3093 100644 --- a/crates/noirc_evaluator/src/errors.rs +++ b/crates/noirc_evaluator/src/errors.rs @@ -116,7 +116,8 @@ impl RuntimeError { } _ => { let message = self.to_string(); - let location = self.call_stack().back().expect("Expected RuntimeError to have a location"); + let location = + self.call_stack().back().expect("Expected RuntimeError to have a location"); Diagnostic::simple_error(message, String::new(), location.span) } diff --git a/crates/noirc_evaluator/src/ssa/acir_gen/acir_ir/acir_variable.rs b/crates/noirc_evaluator/src/ssa/acir_gen/acir_ir/acir_variable.rs index 6bf9a9d1085..30dc0e7e8f2 100644 --- a/crates/noirc_evaluator/src/ssa/acir_gen/acir_ir/acir_variable.rs +++ b/crates/noirc_evaluator/src/ssa/acir_gen/acir_ir/acir_variable.rs @@ -240,7 +240,7 @@ impl AcirContext { } /// Converts an [`AcirVar`] to an [`Expression`] - fn var_to_expression(&self, var: AcirVar) -> Result { + pub(crate) fn var_to_expression(&self, var: AcirVar) -> Result { let var_data = match self.vars.get(&var) { Some(var_data) => var_data, None => { diff --git a/crates/noirc_evaluator/src/ssa/acir_gen/mod.rs b/crates/noirc_evaluator/src/ssa/acir_gen/mod.rs index b69376c573c..092fb1e966b 100644 --- a/crates/noirc_evaluator/src/ssa/acir_gen/mod.rs +++ b/crates/noirc_evaluator/src/ssa/acir_gen/mod.rs @@ -30,6 +30,7 @@ use acvm::{ FieldElement, }; use fxhash::FxHashMap as HashMap; +use im::Vector; use iter_extended::{try_vecmap, vecmap}; use noirc_frontend::Distinctness; @@ -428,6 +429,22 @@ impl Context { // assert_eq!(result_ids.len(), outputs.len()); for (result, output) in result_ids.iter().zip(outputs) { + match &output { + // We need to make sure we initialize arrays returned from intrinsic calls + // or else they will fail if accessed with a dynamic index + AcirValue::Array(values) => { + let block_id = self.block_id(result); + let values = vecmap(values, |v| v.clone()); + + self.initialize_array(block_id, values.len(), Some(&values))?; + } + AcirValue::DynamicArray(_) => { + unreachable!("The output from an intrinsic call is expected to be a single value or an array but got {output:?}"); + } + AcirValue::Var(_, _) => { + // Do nothing + } + } self.ssa_values.insert(*result, output); } } @@ -454,25 +471,8 @@ impl Context { let acir_var = self.convert_numeric_value(*condition, dfg)?; self.current_side_effects_enabled_var = acir_var; } - Instruction::ArrayGet { array, index } => { - self.handle_array_operation( - instruction_id, - *array, - *index, - None, - dfg, - last_array_uses, - )?; - } - Instruction::ArraySet { array, index, value } => { - self.handle_array_operation( - instruction_id, - *array, - *index, - Some(*value), - dfg, - last_array_uses, - )?; + Instruction::ArrayGet { .. } | Instruction::ArraySet { .. } => { + self.handle_array_operation(instruction_id, dfg, last_array_uses)?; } Instruction::Allocate => { unreachable!("Expected all allocate instructions to be removed before acir_gen") @@ -523,72 +523,94 @@ impl Context { fn handle_array_operation( &mut self, instruction: InstructionId, - array: ValueId, - index: ValueId, - store_value: Option, dfg: &DataFlowGraph, last_array_uses: &HashMap, ) -> Result<(), RuntimeError> { - let index_const = dfg.get_numeric_constant(index); - - match self.convert_value(array, dfg) { - AcirValue::Var(acir_var, _) => { - return Err(RuntimeError::InternalError(InternalError::UnExpected { - expected: "an array value".to_string(), - found: format!("{acir_var:?}"), + // Pass the instruction between array methods rather than the internal fields themselves + let (array, index, store_value) = match dfg[instruction] { + Instruction::ArrayGet { array, index } => (array, index, None), + Instruction::ArraySet { array, index, value, .. } => (array, index, Some(value)), + _ => { + return Err(InternalError::UnExpected { + expected: "Instruction should be an ArrayGet or ArraySet".to_owned(), + found: format!("Instead got {:?}", dfg[instruction]), call_stack: self.acir_context.get_call_stack(), - })) + } + .into()) } - AcirValue::Array(array) => { - if let Some(index_const) = index_const { - let array_size = array.len(); - let index = match index_const.try_to_u64() { - Some(index_const) => index_const as usize, - None => { - let call_stack = self.acir_context.get_call_stack(); - return Err(RuntimeError::TypeConversion { - from: "array index".to_string(), - into: "u64".to_string(), - call_stack, - }); - } - }; - if index >= array_size { - // Ignore the error if side effects are disabled. - if self.acir_context.is_constant_one(&self.current_side_effects_enabled_var) - { - let call_stack = self.acir_context.get_call_stack(); - return Err(RuntimeError::IndexOutOfBounds { - index, - array_size, - call_stack, - }); - } - let result_type = - dfg.type_of_value(dfg.instruction_results(instruction)[0]); - let value = self.create_default_value(&result_type)?; - self.define_result(dfg, instruction, value); - return Ok(()); + }; + + let index_const = dfg.get_numeric_constant(index); + + match dfg.type_of_value(array) { + Type::Array(_, _) => { + match self.convert_value(array, dfg) { + AcirValue::Var(acir_var, _) => { + return Err(RuntimeError::InternalError(InternalError::UnExpected { + expected: "an array value".to_string(), + found: format!("{acir_var:?}"), + call_stack: self.acir_context.get_call_stack(), + })) } + AcirValue::Array(array) => { + if let Some(index_const) = index_const { + let array_size = array.len(); + let index = match index_const.try_to_u64() { + Some(index_const) => index_const as usize, + None => { + let call_stack = self.acir_context.get_call_stack(); + return Err(RuntimeError::TypeConversion { + from: "array index".to_string(), + into: "u64".to_string(), + call_stack, + }); + } + }; + if index >= array_size { + // Ignore the error if side effects are disabled. + if self + .acir_context + .is_constant_one(&self.current_side_effects_enabled_var) + { + let call_stack = self.acir_context.get_call_stack(); + return Err(RuntimeError::IndexOutOfBounds { + index, + array_size, + call_stack, + }); + } + let result_type = + dfg.type_of_value(dfg.instruction_results(instruction)[0]); + let value = self.create_default_value(&result_type)?; + self.define_result(dfg, instruction, value); + return Ok(()); + } - let value = match store_value { - Some(store_value) => { - let store_value = self.convert_value(store_value, dfg); - AcirValue::Array(array.update(index, store_value)) - } - None => array[index].clone(), - }; + let value = match store_value { + Some(store_value) => { + let store_value = self.convert_value(store_value, dfg); + AcirValue::Array(array.update(index, store_value)) + } + None => array[index].clone(), + }; - self.define_result(dfg, instruction, value); - return Ok(()); + self.define_result(dfg, instruction, value); + return Ok(()); + } + } + AcirValue::DynamicArray(_) => (), } } - AcirValue::DynamicArray(_) => (), + Type::Slice(_) => { + // Do nothing we only want dynamic checks here + } + _ => unreachable!("ICE: expected array or slice type"), } + let resolved_array = dfg.resolve(array); let map_array = last_array_uses.get(&resolved_array) == Some(&instruction); - if let Some(store) = store_value { - self.array_set(instruction, array, index, store, dfg, map_array)?; + if store_value.is_some() { + self.array_set(instruction, dfg, map_array)?; } else { self.array_get(instruction, array, index, dfg)?; } @@ -617,7 +639,7 @@ impl Context { return Err(RuntimeError::UnInitialized { name: "array".to_string(), call_stack: self.acir_context.get_call_stack(), - }) + }); } } } @@ -627,6 +649,16 @@ impl Context { let typ = match dfg.type_of_value(array) { Type::Array(typ, _) => { if typ.len() != 1 { + // TODO(#2461) + unimplemented!( + "Non-const array indices is not implemented for non-homogenous array" + ); + } + typ[0].clone() + } + Type::Slice(typ) => { + if typ.len() != 1 { + // TODO(#2461) unimplemented!( "Non-const array indices is not implemented for non-homogenous array" ); @@ -646,12 +678,21 @@ impl Context { fn array_set( &mut self, instruction: InstructionId, - array: ValueId, - index: ValueId, - store_value: ValueId, dfg: &DataFlowGraph, map_array: bool, ) -> Result<(), InternalError> { + // Pass the instruction between array methods rather than the internal fields themselves + let (array, index, store_value, length) = match dfg[instruction] { + Instruction::ArraySet { array, index, value, length } => (array, index, value, length), + _ => { + return Err(InternalError::UnExpected { + expected: "Instruction should be an ArraySet".to_owned(), + found: format!("Instead got {:?}", dfg[instruction]), + call_stack: self.acir_context.get_call_stack(), + }) + } + }; + // Fetch the internal SSA ID for the array let array = dfg.resolve(array); @@ -662,6 +703,15 @@ impl Context { // the SSA IR. let len = match dfg.type_of_value(array) { Type::Array(_, len) => len, + Type::Slice(_) => { + let length = length + .expect("ICE: array set on slice must have a length associated with the call"); + let length_acir_var = self.convert_value(length, dfg).into_var()?; + let len = self.acir_context.var_to_expression(length_acir_var)?.to_const(); + let len = len + .expect("ICE: slice length should be fully tracked and constant by ACIR gen"); + len.to_u128() as usize + } _ => unreachable!("ICE - expected an array"), }; @@ -1117,10 +1167,180 @@ impl Context { }; Ok(vec![AcirValue::Var(self.acir_context.add_constant(len), AcirType::field())]) } + Intrinsic::SlicePushBack => { + let slice_length = self.convert_value(arguments[0], dfg).into_var()?; + let slice = self.convert_value(arguments[1], dfg); + // TODO(#2461): make sure that we have handled nested struct inputs + let element = self.convert_value(arguments[2], dfg); + + let one = self.acir_context.add_constant(FieldElement::one()); + let new_slice_length = self.acir_context.add_var(slice_length, one)?; + + let mut new_slice = Vector::new(); + self.slice_intrinsic_input(&mut new_slice, slice)?; + new_slice.push_back(element); + + Ok(vec![ + AcirValue::Var(new_slice_length, AcirType::field()), + AcirValue::Array(new_slice), + ]) + } + Intrinsic::SlicePushFront => { + let slice_length = self.convert_value(arguments[0], dfg).into_var()?; + let slice = self.convert_value(arguments[1], dfg); + // TODO(#2461): make sure that we have handled nested struct inputs + let element = self.convert_value(arguments[2], dfg); + + let one = self.acir_context.add_constant(FieldElement::one()); + let new_slice_length = self.acir_context.add_var(slice_length, one)?; + + let mut new_slice = Vector::new(); + self.slice_intrinsic_input(&mut new_slice, slice)?; + new_slice.push_front(element); + + Ok(vec![ + AcirValue::Var(new_slice_length, AcirType::field()), + AcirValue::Array(new_slice), + ]) + } + Intrinsic::SlicePopBack => { + let slice_length = self.convert_value(arguments[0], dfg).into_var()?; + let slice = self.convert_value(arguments[1], dfg); + + let one = self.acir_context.add_constant(FieldElement::one()); + let new_slice_length = self.acir_context.sub_var(slice_length, one)?; + + let mut new_slice = Vector::new(); + self.slice_intrinsic_input(&mut new_slice, slice)?; + // TODO(#2461): make sure that we have handled nested struct inputs + let elem = new_slice + .pop_back() + .expect("There are no elements in this slice to be removed"); + + Ok(vec![ + AcirValue::Var(new_slice_length, AcirType::field()), + AcirValue::Array(new_slice), + elem, + ]) + } + Intrinsic::SlicePopFront => { + let slice_length = self.convert_value(arguments[0], dfg).into_var()?; + let slice = self.convert_value(arguments[1], dfg); + + let one = self.acir_context.add_constant(FieldElement::one()); + let new_slice_length = self.acir_context.sub_var(slice_length, one)?; + + let mut new_slice = Vector::new(); + self.slice_intrinsic_input(&mut new_slice, slice)?; + // TODO(#2461): make sure that we have handled nested struct inputs + let elem = new_slice + .pop_front() + .expect("There are no elements in this slice to be removed"); + + Ok(vec![ + elem, + AcirValue::Var(new_slice_length, AcirType::field()), + AcirValue::Array(new_slice), + ]) + } + Intrinsic::SliceInsert => { + // Slice insert with a constant index + let slice_length = self.convert_value(arguments[0], dfg).into_var()?; + let slice = self.convert_value(arguments[1], dfg); + let index = self.convert_value(arguments[2], dfg).into_var()?; + let element = self.convert_value(arguments[3], dfg); + + let one = self.acir_context.add_constant(FieldElement::one()); + let new_slice_length = self.acir_context.add_var(slice_length, one)?; + + // TODO(#2462): Slice insert is a little less obvious on how to implement due to the case + // of having a dynamic index + // The slice insert logic will need a more involved codegen + let index = self.acir_context.var_to_expression(index)?.to_const(); + let index = index + .expect("ICE: slice length should be fully tracked and constant by ACIR gen"); + let index = index.to_u128() as usize; + + let mut new_slice = Vector::new(); + self.slice_intrinsic_input(&mut new_slice, slice)?; + // TODO(#2461): make sure that we have handled nested struct inputs + new_slice.insert(index, element); + + Ok(vec![ + AcirValue::Var(new_slice_length, AcirType::field()), + AcirValue::Array(new_slice), + ]) + } + Intrinsic::SliceRemove => { + // Slice insert with a constant index + let slice_length = self.convert_value(arguments[0], dfg).into_var()?; + let slice = self.convert_value(arguments[1], dfg); + let index = self.convert_value(arguments[2], dfg).into_var()?; + + let one = self.acir_context.add_constant(FieldElement::one()); + let new_slice_length = self.acir_context.sub_var(slice_length, one)?; + + // TODO(#2462): allow slice remove with a constant index + // Slice remove is a little less obvious on how to implement due to the case + // of having a dynamic index + // The slice remove logic will need a more involved codegen + let index = self.acir_context.var_to_expression(index)?.to_const(); + let index = index + .expect("ICE: slice length should be fully tracked and constant by ACIR gen"); + let index = index.to_u128() as usize; + + let mut new_slice = Vector::new(); + self.slice_intrinsic_input(&mut new_slice, slice)?; + // TODO(#2461): make sure that we have handled nested struct inputs + let removed_elem = new_slice.remove(index); + + Ok(vec![ + AcirValue::Var(new_slice_length, AcirType::field()), + AcirValue::Array(new_slice), + removed_elem, + ]) + } _ => todo!("expected a black box function"), } } + fn slice_intrinsic_input( + &mut self, + old_slice: &mut Vector, + input: AcirValue, + ) -> Result<(), RuntimeError> { + match input { + AcirValue::Var(_, _) => { + old_slice.push_back(input); + } + AcirValue::Array(vars) => { + for var in vars { + self.slice_intrinsic_input(old_slice, var)?; + } + } + AcirValue::DynamicArray(AcirDynamicArray { block_id, len }) => { + for i in 0..len { + // We generate witnesses corresponding to the array values + let index = AcirValue::Var( + self.acir_context.add_constant(FieldElement::from(i as u128)), + AcirType::NumericType(NumericType::NativeField), + ); + + let index_var = index.into_var()?; + let value_read_var = + self.acir_context.read_from_memory(block_id, &index_var)?; + let value_read = AcirValue::Var( + value_read_var, + AcirType::NumericType(NumericType::NativeField), + ); + + old_slice.push_back(value_read); + } + } + } + Ok(()) + } + /// Given an array value, return the numerical type of its element. /// Panics if the given value is not an array or has a non-numeric element type. fn array_element_type(dfg: &DataFlowGraph, value: ValueId) -> AcirType { diff --git a/crates/noirc_evaluator/src/ssa/function_builder/mod.rs b/crates/noirc_evaluator/src/ssa/function_builder/mod.rs index c8b67c94888..961ff8cc33e 100644 --- a/crates/noirc_evaluator/src/ssa/function_builder/mod.rs +++ b/crates/noirc_evaluator/src/ssa/function_builder/mod.rs @@ -275,8 +275,9 @@ impl FunctionBuilder { array: ValueId, index: ValueId, value: ValueId, + length: Option, ) -> ValueId { - self.insert_instruction(Instruction::ArraySet { array, index, value }, None).first() + self.insert_instruction(Instruction::ArraySet { array, index, value, length }, None).first() } /// Terminates the current block with the given terminator instruction diff --git a/crates/noirc_evaluator/src/ssa/ir/instruction.rs b/crates/noirc_evaluator/src/ssa/ir/instruction.rs index e57ae55209b..67c6ded4da2 100644 --- a/crates/noirc_evaluator/src/ssa/ir/instruction.rs +++ b/crates/noirc_evaluator/src/ssa/ir/instruction.rs @@ -170,7 +170,9 @@ pub(crate) enum Instruction { /// Creates a new array with the new value at the given index. All other elements are identical /// to those in the given array. This will not modify the original array. - ArraySet { array: ValueId, index: ValueId, value: ValueId }, + /// + /// An optional length can be provided to enabling handling of dynamic slice indices + ArraySet { array: ValueId, index: ValueId, value: ValueId, length: Option }, } impl Instruction { @@ -296,9 +298,12 @@ impl Instruction { Instruction::ArrayGet { array, index } => { Instruction::ArrayGet { array: f(*array), index: f(*index) } } - Instruction::ArraySet { array, index, value } => { - Instruction::ArraySet { array: f(*array), index: f(*index), value: f(*value) } - } + Instruction::ArraySet { array, index, value, length } => Instruction::ArraySet { + array: f(*array), + index: f(*index), + value: f(*value), + length: length.map(f), + }, } } @@ -335,10 +340,11 @@ impl Instruction { f(*array); f(*index); } - Instruction::ArraySet { array, index, value } => { + Instruction::ArraySet { array, index, value, length } => { f(*array); f(*index); f(*value); + length.map(&mut f); } Instruction::EnableSideEffects { condition } => { f(*condition); @@ -396,7 +402,7 @@ impl Instruction { } None } - Instruction::ArraySet { array, index, value } => { + Instruction::ArraySet { array, index, value, .. } => { let array = dfg.get_array_constant(*array); let index = dfg.get_numeric_constant(*index); if let (Some((array, element_type)), Some(index)) = (array, index) { @@ -419,7 +425,7 @@ impl Instruction { None } } - Instruction::Call { func, arguments } => simplify_call(*func, arguments, dfg), + Instruction::Call { func, arguments } => simplify_call(*func, arguments, dfg, block), Instruction::EnableSideEffects { condition } => { if let Some(last) = dfg[block].instructions().last().copied() { let last = &mut dfg[last]; diff --git a/crates/noirc_evaluator/src/ssa/ir/instruction/call.rs b/crates/noirc_evaluator/src/ssa/ir/instruction/call.rs index 34ec8a90092..dbc358c5f19 100644 --- a/crates/noirc_evaluator/src/ssa/ir/instruction/call.rs +++ b/crates/noirc_evaluator/src/ssa/ir/instruction/call.rs @@ -9,17 +9,23 @@ use crate::ssa::ir::{ instruction::Intrinsic, map::Id, types::Type, - value::{Value, ValueId}, + value::{Value, ValueId}, basic_block::BasicBlockId, }; use super::{Binary, BinaryOp, Endian, Instruction, SimplifyResult}; /// Try to simplify this call instruction. If the instruction can be simplified to a known value, /// that value is returned. Otherwise None is returned. +/// +/// The `block` parameter indicates the block any new instructions that are part of a call's +/// simplification will be inserted into. For example, all slice intrinsics require updates +/// to the slice length, which requires inserting a binary instruction. This update instruction +/// must be inserted into the same block that the call itself is being simplified into. pub(super) fn simplify_call( func: ValueId, arguments: &[ValueId], dfg: &mut DataFlowGraph, + block: BasicBlockId, ) -> SimplifyResult { let intrinsic = match &dfg[func] { Value::Intrinsic(intrinsic) => *intrinsic, @@ -88,7 +94,7 @@ pub(super) fn simplify_call( slice.push_back(*elem); } - let new_slice_length = update_slice_length(arguments[0], dfg, BinaryOp::Add); + let new_slice_length = update_slice_length(arguments[0], dfg, BinaryOp::Add, block); let new_slice = dfg.make_array(slice, element_type); SimplifyResult::SimplifiedToMultiple(vec![new_slice_length, new_slice]) @@ -103,7 +109,7 @@ pub(super) fn simplify_call( slice.push_front(*elem); } - let new_slice_length = update_slice_length(arguments[0], dfg, BinaryOp::Add); + let new_slice_length = update_slice_length(arguments[0], dfg, BinaryOp::Add, block); let new_slice = dfg.make_array(slice, element_type); SimplifyResult::SimplifiedToMultiple(vec![new_slice_length, new_slice]) @@ -128,7 +134,7 @@ pub(super) fn simplify_call( let new_slice = dfg.make_array(slice, typ); results.push_front(new_slice); - let new_slice_length = update_slice_length(arguments[0], dfg, BinaryOp::Sub); + let new_slice_length = update_slice_length(arguments[0], dfg, BinaryOp::Sub, block); results.push_front(new_slice_length); SimplifyResult::SimplifiedToMultiple(results.into()) @@ -146,7 +152,7 @@ pub(super) fn simplify_call( slice.pop_front().expect("There are no elements in this slice to be removed") }); - let new_slice_length = update_slice_length(arguments[0], dfg, BinaryOp::Sub); + let new_slice_length = update_slice_length(arguments[0], dfg, BinaryOp::Sub, block); results.push(new_slice_length); @@ -171,7 +177,7 @@ pub(super) fn simplify_call( index += 1; } - let new_slice_length = update_slice_length(arguments[0], dfg, BinaryOp::Add); + let new_slice_length = update_slice_length(arguments[0], dfg, BinaryOp::Add, block); let new_slice = dfg.make_array(slice, typ); SimplifyResult::SimplifiedToMultiple(vec![new_slice_length, new_slice]) @@ -194,7 +200,7 @@ pub(super) fn simplify_call( let new_slice = dfg.make_array(slice, typ); results.insert(0, new_slice); - let new_slice_length = update_slice_length(arguments[0], dfg, BinaryOp::Sub); + let new_slice_length = update_slice_length(arguments[0], dfg, BinaryOp::Sub, block); results.insert(0, new_slice_length); @@ -226,9 +232,8 @@ pub(super) fn simplify_call( /// The binary operation performed on the slice length is always an addition or subtraction of `1`. /// This is because the slice length holds the user length (length as displayed by a `.len()` call), /// and not a flattened length used internally to represent arrays of tuples. -fn update_slice_length(slice_len: ValueId, dfg: &mut DataFlowGraph, operator: BinaryOp) -> ValueId { +fn update_slice_length(slice_len: ValueId, dfg: &mut DataFlowGraph, operator: BinaryOp, block: BasicBlockId) -> ValueId { let one = dfg.make_constant(FieldElement::one(), Type::field()); - let block = dfg.make_block(); let instruction = Instruction::Binary(Binary { lhs: slice_len, operator, rhs: one }); let call_stack = dfg.get_value_call_stack(slice_len); dfg.insert_instruction_and_results(instruction, block, None, call_stack).first() diff --git a/crates/noirc_evaluator/src/ssa/ir/printer.rs b/crates/noirc_evaluator/src/ssa/ir/printer.rs index 037f8aaac2b..537b474a8d9 100644 --- a/crates/noirc_evaluator/src/ssa/ir/printer.rs +++ b/crates/noirc_evaluator/src/ssa/ir/printer.rs @@ -163,14 +163,19 @@ pub(crate) fn display_instruction( Instruction::ArrayGet { array, index } => { writeln!(f, "array_get {}, index {}", show(*array), show(*index)) } - Instruction::ArraySet { array, index, value } => { - writeln!( + Instruction::ArraySet { array, index, value, length } => { + write!( f, "array_set {}, index {}, value {}", show(*array), show(*index), show(*value) - ) + )?; + if let Some(length) = length { + writeln!(f, ", length {}", show(*length)) + } else { + writeln!(f) + } } } } diff --git a/crates/noirc_evaluator/src/ssa/opt/flatten_cfg.rs b/crates/noirc_evaluator/src/ssa/opt/flatten_cfg.rs index 35f01f293b7..2b9991844c8 100644 --- a/crates/noirc_evaluator/src/ssa/opt/flatten_cfg.rs +++ b/crates/noirc_evaluator/src/ssa/opt/flatten_cfg.rs @@ -144,7 +144,7 @@ use crate::ssa::{ dfg::{CallStack, InsertInstructionResult}, function::Function, function_inserter::FunctionInserter, - instruction::{BinaryOp, Instruction, InstructionId, TerminatorInstruction}, + instruction::{BinaryOp, Instruction, InstructionId, Intrinsic, TerminatorInstruction}, types::Type, value::{Value, ValueId}, }, @@ -176,8 +176,21 @@ struct Context<'f> { branch_ends: HashMap, /// Maps an address to the old and new value of the element at that address + /// These only hold stores for one block at a time and is cleared + /// between inlining of branches. store_values: HashMap, + /// Maps an address to the old and new value of the element at that address + /// The difference between this map and store_values is that this stores + /// the old and new value of an element from the outer block whose jmpif + /// terminator is being flattened. + /// + /// This map persists throughout the flattening process, where addresses + /// are overwritten as new stores are found. This overwriting is the desired behavior, + /// as we want the most update to date value to be stored at a given address as + /// we walk through blocks to flatten. + outer_block_stores: HashMap, + /// Stores all allocations local to the current branch. /// Since these branches are local to the current branch (ie. only defined within one branch of /// an if expression), they should not be merged with their previous value or stored value in @@ -223,6 +236,7 @@ fn flatten_function_cfg(function: &mut Function) { local_allocations: HashSet::new(), branch_ends, conditions: Vec::new(), + outer_block_stores: HashMap::default(), }; context.flatten(); } @@ -245,6 +259,26 @@ impl<'f> Context<'f> { /// Returns the last block to be inlined. This is either the return block of the function or, /// if self.conditions is not empty, the end block of the most recent condition. fn handle_terminator(&mut self, block: BasicBlockId) -> BasicBlockId { + if let TerminatorInstruction::JmpIf { .. } = + self.inserter.function.dfg[block].unwrap_terminator() + { + // Find stores in the outer block and insert into the `outer_block_stores` map. + // Not using this map can lead to issues when attempting to merge slices. + // When inlining a branch end, only the then branch and the else branch are checked for stores. + // However, there are cases where we want to load a value that comes from the outer block + // that we are handling the terminator for here. + let instructions = self.inserter.function.dfg[block].instructions().to_vec(); + for instruction in instructions { + let (instruction, _) = self.inserter.map_instruction(instruction); + if let Instruction::Store { address, value } = instruction { + let load = Instruction::Load { address }; + let load_type = Some(vec![self.inserter.function.dfg.type_of_value(value)]); + let old_value = self.insert_instruction_with_typevars(load, load_type).first(); + self.outer_block_stores.insert(address, Store { old_value, new_value: value }); + } + } + } + match self.inserter.function.dfg[block].unwrap_terminator() { TerminatorInstruction::JmpIf { condition, then_destination, else_destination } => { let old_condition = *condition; @@ -412,20 +446,10 @@ impl<'f> Context<'f> { _ => panic!("Expected slice type"), }; - let then_value = self.inserter.function.dfg[then_value_id].clone(); - let else_value = self.inserter.function.dfg[else_value_id].clone(); - - let len = match then_value { - Value::Array { array, .. } => array.len(), - _ => panic!("Expected array value"), - }; + let then_len = self.get_slice_length(then_value_id); + let else_len = self.get_slice_length(else_value_id); - let else_len = match else_value { - Value::Array { array, .. } => array.len(), - _ => panic!("Expected array value"), - }; - - let len = len.max(else_len); + let len = then_len.max(else_len); for i in 0..len { for (element_index, element_type) in element_types.iter().enumerate() { @@ -446,7 +470,7 @@ impl<'f> Context<'f> { } }; - let then_element = get_element(then_value_id, typevars.clone(), len); + let then_element = get_element(then_value_id, typevars.clone(), then_len); let else_element = get_element(else_value_id, typevars, else_len); merged.push_back(self.merge_values( @@ -461,6 +485,51 @@ impl<'f> Context<'f> { self.inserter.function.dfg.make_array(merged, typ) } + fn get_slice_length(&mut self, value_id: ValueId) -> usize { + let value = &self.inserter.function.dfg[value_id]; + match value { + Value::Array { array, .. } => array.len(), + Value::NumericConstant { constant, .. } => constant.to_u128() as usize, + Value::Instruction { instruction: instruction_id, .. } => { + let instruction = &self.inserter.function.dfg[*instruction_id]; + match instruction { + Instruction::ArraySet { length, .. } => { + let length = length.expect("ICE: array set on a slice must have a length"); + self.get_slice_length(length) + } + Instruction::Load { address } => { + let context_store = self + .outer_block_stores + .get(address) + .expect("ICE: load in merger should have store from outer block"); + + self.get_slice_length(context_store.new_value) + } + Instruction::Call { func, arguments } => { + let func = &self.inserter.function.dfg[*func]; + let length = arguments[0]; + match func { + Value::Intrinsic(intrinsic) => match intrinsic { + Intrinsic::SlicePushBack + | Intrinsic::SlicePushFront + | Intrinsic::SliceInsert => self.get_slice_length(length) + 1, + Intrinsic::SlicePopBack + | Intrinsic::SlicePopFront + | Intrinsic::SliceRemove => self.get_slice_length(length) - 1, + _ => { + unreachable!("ICE: Intrinsic not supported, got {intrinsic:?}") + } + }, + _ => unreachable!("ICE: Expected intrinsic value but got {func:?}"), + } + } + _ => unreachable!("ICE: Got unexpected instruction: {instruction:?}"), + } + } + _ => unreachable!("ICE: Got unexpected value when resolving slice length {value:?}"), + } + } + /// Given an if expression that returns an array: `if c { array1 } else { array2 }`, /// this function will recursively merge array1 and array2 into a single resulting array /// by creating a new array containing the result of self.merge_values for each element. diff --git a/crates/noirc_evaluator/src/ssa/ssa_gen/context.rs b/crates/noirc_evaluator/src/ssa/ssa_gen/context.rs index 5631aa8d351..e7f8131bc35 100644 --- a/crates/noirc_evaluator/src/ssa/ssa_gen/context.rs +++ b/crates/noirc_evaluator/src/ssa/ssa_gen/context.rs @@ -659,14 +659,19 @@ impl<'a> FunctionContext<'a> { match lvalue { LValue::Ident => unreachable!("Cannot assign to a variable without a reference"), LValue::Index { old_array: mut array, index, array_lvalue, location } => { - array = self.assign_lvalue_index(new_value, array, index, location); + array = self.assign_lvalue_index(new_value, array, index, None, location); self.assign_new_value(*array_lvalue, array.into()); } LValue::SliceIndex { old_slice: slice, index, slice_lvalue, location } => { let mut slice_values = slice.into_value_list(self); - slice_values[1] = - self.assign_lvalue_index(new_value, slice_values[1], index, location); + slice_values[1] = self.assign_lvalue_index( + new_value, + slice_values[1], + index, + Some(slice_values[0]), + location, + ); // The size of the slice does not change in a slice index assignment so we can reuse the same length value let new_slice = Tree::Branch(vec![slice_values[0].into(), slice_values[1].into()]); @@ -687,6 +692,7 @@ impl<'a> FunctionContext<'a> { new_value: Values, mut array: ValueId, index: ValueId, + length: Option, location: Location, ) -> ValueId { let element_size = self.builder.field_constant(self.element_size(array)); @@ -698,7 +704,7 @@ impl<'a> FunctionContext<'a> { new_value.for_each(|value| { let value = value.eval(self); - array = self.builder.insert_array_set(array, index, value); + array = self.builder.insert_array_set(array, index, value, length); index = self.builder.insert_binary(index, BinaryOp::Add, one); }); array diff --git a/noir_stdlib/src/slice.nr b/noir_stdlib/src/slice.nr index 4f73f3a2994..053a8acacb6 100644 --- a/noir_stdlib/src/slice.nr +++ b/noir_stdlib/src/slice.nr @@ -22,16 +22,28 @@ impl [T] { #[builtin(slice_pop_front)] fn pop_front(_self: Self) -> (T, Self) { } + fn insert(self, _index: Field, _elem: T) -> Self { + // TODO(#2462): Slice insert with a dynamic index + crate::assert_constant(_index); + self.__slice_insert(_index, _elem) + } + /// Insert an element at a specified index, shifting all elements /// after it to the right #[builtin(slice_insert)] - fn insert(_self: Self, _index: Field, _elem: T) -> Self { } + fn __slice_insert(_self: Self, _index: Field, _elem: T) -> Self { } + + fn remove(self, _index: Field) -> (Self, T) { + // TODO(#2462): Slice remove with a dynamic index + crate::assert_constant(_index); + self.__slice_remove(_index) + } /// Remove an element at a specified index, shifting all elements /// after it to the left, returning the altered slice and /// the removed element #[builtin(slice_remove)] - fn remove(_self: Self, _index: Field) -> (Self, T) { } + fn __slice_remove(_self: Self, _index: Field) -> (Self, T) { } // Append each element of the `other` slice to the end of `self`. // This returns a new slice and leaves both input slices unchanged.