diff --git a/acvm-repo/acir/src/native_types/expression/mod.rs b/acvm-repo/acir/src/native_types/expression/mod.rs index 8bb07226529..037cd538c59 100644 --- a/acvm-repo/acir/src/native_types/expression/mod.rs +++ b/acvm-repo/acir/src/native_types/expression/mod.rs @@ -138,6 +138,11 @@ impl Expression { self.mul_terms.sort_by(|a, b| a.1.cmp(&b.1).then(a.2.cmp(&b.2))); self.linear_combinations.sort_by(|a, b| a.1.cmp(&b.1)); } + + pub(crate) fn is_sorted(&self) -> bool { + self.mul_terms.iter().is_sorted_by(|a, b| a.1.cmp(&b.1).then(a.2.cmp(&b.2)).is_le()) + && self.linear_combinations.iter().is_sorted_by(|a, b| a.1.cmp(&b.1).is_le()) + } } impl Expression { diff --git a/acvm-repo/acir/src/native_types/expression/ordering.rs b/acvm-repo/acir/src/native_types/expression/ordering.rs index 968dc310bc1..ce2b244de97 100644 --- a/acvm-repo/acir/src/native_types/expression/ordering.rs +++ b/acvm-repo/acir/src/native_types/expression/ordering.rs @@ -8,6 +8,10 @@ use super::Expression; impl Ord for Expression { fn cmp(&self, other: &Self) -> Ordering { + // `get_max_term` has a comment: "This function assumes the gate is sorted" + assert!(self.is_sorted()); + assert!(other.is_sorted()); + let mut i1 = self.get_max_idx(); let mut i2 = other.get_max_idx(); let mut result = Ordering::Equal; diff --git a/compiler/noirc_evaluator/src/acir/acir_context/black_box.rs b/compiler/noirc_evaluator/src/acir/acir_context/black_box.rs index fe04e08e0b5..a97790c340a 100644 --- a/compiler/noirc_evaluator/src/acir/acir_context/black_box.rs +++ b/compiler/noirc_evaluator/src/acir/acir_context/black_box.rs @@ -33,7 +33,7 @@ impl AcirContext { assert_eq!( output_count, - input_size + (16 - input_size % 16), + input_size + 16 - input_size % 16, "output count mismatch" ); @@ -105,7 +105,7 @@ impl AcirContext { let mut inputs = self.prepare_inputs_for_black_box_func_call(inputs, allow_constant_inputs)?; if name == BlackBoxFunc::EmbeddedCurveAdd { - inputs = self.all_or_nothing_for_ec_add(inputs)?; + inputs = self.all_variables_or_constants_for_ec_add(inputs)?; } Ok(inputs) } @@ -147,9 +147,11 @@ impl AcirContext { Ok(witnesses) } - /// EcAdd has 6 inputs representing the two points to add - /// Each point must be either all constant, or all witnesses - fn all_or_nothing_for_ec_add( + /// [`BlackBoxFunc::EmbeddedCurveAdd`] has 6 inputs representing the two points to add + /// Each point must be either all constants, or all witnesses, + /// where constants are converted to witnesses here if mixed constant and witnesses are + /// encountered + fn all_variables_or_constants_for_ec_add( &mut self, inputs: Vec>>, ) -> Result>>, RuntimeError> { @@ -157,14 +159,17 @@ impl AcirContext { let mut has_witness = false; let mut result = inputs.clone(); for (i, input) in inputs.iter().enumerate() { + assert_eq!(input.len(), 1); if input[0].is_constant() { has_constant = true; } else { has_witness = true; } + if i % 3 == 2 { if has_constant && has_witness { - // Convert the constants to witness if mixed constant and witness, + // Convert the constants to witnesses if mixed constants and witnesses are + // encountered for j in i - 2..i + 1 { if let FunctionInput::Constant(constant) = inputs[j][0] { let constant = self.add_constant(constant); diff --git a/compiler/noirc_evaluator/src/acir/acir_context/brillig_call.rs b/compiler/noirc_evaluator/src/acir/acir_context/brillig_call.rs index eb7c1e9caab..afc7bd64c4c 100644 --- a/compiler/noirc_evaluator/src/acir/acir_context/brillig_call.rs +++ b/compiler/noirc_evaluator/src/acir/acir_context/brillig_call.rs @@ -21,12 +21,13 @@ impl AcirContext { outputs: Vec, ) -> Result, RuntimeError> { let stdlib_func_bytecode = &self.brillig_stdlib.get_code(brillig_stdlib_func).clone(); + let safe_return_values = false; self.brillig_call( predicate, stdlib_func_bytecode, inputs, outputs, - false, + safe_return_values, PLACEHOLDER_BRILLIG_INDEX, Some(brillig_stdlib_func), ) diff --git a/compiler/noirc_evaluator/src/acir/acir_context/generated_acir/brillig_directive.rs b/compiler/noirc_evaluator/src/acir/acir_context/generated_acir/brillig_directive.rs index 0bd92def6f1..9fedb526d55 100644 --- a/compiler/noirc_evaluator/src/acir/acir_context/generated_acir/brillig_directive.rs +++ b/compiler/noirc_evaluator/src/acir/acir_context/generated_acir/brillig_directive.rs @@ -9,6 +9,7 @@ use acvm::acir::{ use crate::brillig::brillig_ir::artifact::GeneratedBrillig; +// TODO(https://github.com/noir-lang/noir/issues/10256): Use `Option` in place of `PLACEHOLDER_BRILLIG_INDEX` /// Brillig calls such as for the Brillig std lib are resolved only after code generation is finished. /// This index should be used when adding a Brillig call during code generation. /// Code generation should then keep track of that unresolved call opcode which will be resolved with the @@ -49,11 +50,11 @@ pub enum BrilligStdlibFunc { ToLeBytes, } -/// Generates brillig bytecode which computes the inverse of its input if not null, and zero else. +/// Generates brillig bytecode which computes the inverse of its input if not null, else zero. pub(crate) fn directive_invert() -> GeneratedBrillig { - // We generate the following code: + // We generate the following code: // fn invert(x : Field) -> Field { - // 1/ x + // 1 / x // } // The input argument, ie the value that will be inverted. @@ -151,28 +152,28 @@ pub(crate) fn directive_quotient() -> GeneratedBrillig { offset_address: MemoryAddress::direct(11), }, // No cast, since calldata is typed as field by default - //q = a/b is set into register (2) + // q = a/b is set into register (2) BrilligOpcode::BinaryFieldOp { op: BinaryFieldOp::IntegerDiv, // We want integer division, not field division! lhs: MemoryAddress::direct(0), rhs: MemoryAddress::direct(1), destination: MemoryAddress::direct(2), }, - //(1)= q*b + // (1)= q*b BrilligOpcode::BinaryFieldOp { op: BinaryFieldOp::Mul, lhs: MemoryAddress::direct(2), rhs: MemoryAddress::direct(1), destination: MemoryAddress::direct(1), }, - //(1) = a-q*b + // (1) = a-q*b BrilligOpcode::BinaryFieldOp { op: BinaryFieldOp::Sub, lhs: MemoryAddress::direct(0), rhs: MemoryAddress::direct(1), destination: MemoryAddress::direct(1), }, - //(0) = q + // (0) = q BrilligOpcode::Mov { destination: MemoryAddress::direct(0), source: MemoryAddress::direct(2), @@ -262,14 +263,14 @@ pub(crate) fn directive_to_radix() -> GeneratedBrillig { rhs: radix, destination: MemoryAddress::direct(3), }, - //(4) = (3)*256 + // (4) = (3)*256 BrilligOpcode::BinaryFieldOp { op: BinaryFieldOp::Mul, lhs: MemoryAddress::direct(3), rhs: radix, destination: MemoryAddress::direct(4), }, - //(4) = a-(3)*256 (remainder) + // (4) = a-(3)*256 (remainder) BrilligOpcode::BinaryFieldOp { op: BinaryFieldOp::Sub, lhs: MemoryAddress::direct(0), @@ -289,7 +290,7 @@ pub(crate) fn directive_to_radix() -> GeneratedBrillig { destination: result_pointer, bit_size: memory_adr_int_size, }, - //a := quotient + // a := quotient BrilligOpcode::Mov { destination: MemoryAddress::direct(0), source: MemoryAddress::direct(3), diff --git a/compiler/noirc_evaluator/src/acir/acir_context/generated_acir/mod.rs b/compiler/noirc_evaluator/src/acir/acir_context/generated_acir/mod.rs index 8b266b3d184..301a177999f 100644 --- a/compiler/noirc_evaluator/src/acir/acir_context/generated_acir/mod.rs +++ b/compiler/noirc_evaluator/src/acir/acir_context/generated_acir/mod.rs @@ -44,7 +44,7 @@ pub struct GeneratedAcir { /// This field is private should only ever be accessed through its getter and setter. /// /// Equivalent to acvm::acir::circuit::Circuit's field of the same name. - pub current_witness_index: Option, + current_witness_index: Option, /// The opcodes of which the compiled ACIR will comprise. pub opcodes: Vec>, @@ -63,7 +63,6 @@ pub struct GeneratedAcir { pub brillig_locations: BTreeMap, /// Source code location of the current instruction being processed - /// None if we do not know the location pub(crate) call_stack_id: CallStackId, /// Correspondence between an opcode index and the error message associated with it. @@ -336,7 +335,7 @@ impl GeneratedAcir { pub(crate) fn radix_le_decompose( &mut self, input_expr: &Expression, - radix: u32, + radix: u128, limb_count: u32, bit_size: u32, ) -> Result, RuntimeError> { @@ -387,7 +386,7 @@ impl GeneratedAcir { pub(crate) fn brillig_to_radix( &mut self, expr: &Expression, - radix: u32, + radix: u128, limb_count: u32, ) -> Vec { // Create the witness for the result @@ -404,7 +403,7 @@ impl GeneratedAcir { let radix_expr = Expression { mul_terms: Vec::new(), linear_combinations: Vec::new(), - q_c: F::from(u128::from(radix)), + q_c: F::from(radix), }; let inputs = vec![ BrilligInputs::Single(expr.clone()), @@ -589,7 +588,7 @@ impl GeneratedAcir { brillig_function_index: BrilligFunctionId, stdlib_func: Option, ) { - // Check whether we have a call to this Brillig function already exists. + // Check whether a call to this Brillig function already exists. // This helps us optimize the Brillig metadata to only be stored once per Brillig entry point. let inserted_func_before = self.brillig_locations.contains_key(&brillig_function_index); @@ -720,15 +719,13 @@ fn black_box_expected_output_size(name: BlackBoxFunc) -> Option { BlackBoxFunc::Sha256Compression => Some(8), - // Can only apply a range constraint to one - // witness at a time. BlackBoxFunc::RANGE => Some(0), // Signature verification algorithms will return a boolean BlackBoxFunc::EcdsaSecp256k1 | BlackBoxFunc::EcdsaSecp256r1 => Some(1), // Output of operations over the embedded curve - // will be 2 field elements representing the point. + // will be 3 field elements representing the point, i.e. (x,y,infinite) BlackBoxFunc::MultiScalarMul | BlackBoxFunc::EmbeddedCurveAdd => Some(3), // Recursive aggregation has a variable number of outputs diff --git a/compiler/noirc_evaluator/src/acir/acir_context/mod.rs b/compiler/noirc_evaluator/src/acir/acir_context/mod.rs index 1ce8ecdceff..71bdb1b33d7 100644 --- a/compiler/noirc_evaluator/src/acir/acir_context/mod.rs +++ b/compiler/noirc_evaluator/src/acir/acir_context/mod.rs @@ -86,7 +86,7 @@ impl AcirContext { self.acir_ir.current_witness_index() } - pub(crate) fn extract_witness(&self, inputs: &[AcirValue]) -> Vec { + pub(crate) fn extract_witnesses(&self, inputs: &[AcirValue]) -> Vec { inputs .iter() .flat_map(|value| value.clone().flatten()) @@ -329,7 +329,7 @@ impl AcirContext { Ok(result_var) } - /// Returns an `AcirVar` that is the XOR result of `lhs` & `rhs`. + /// Returns an `AcirVar` that is the XOR result of `lhs` and `rhs`. pub(crate) fn xor_var( &mut self, lhs: AcirVar, @@ -372,7 +372,7 @@ impl AcirContext { } } - /// Returns an `AcirVar` that is the AND result of `lhs` & `rhs`. + /// Returns an `AcirVar` that is the AND result of `lhs` and `rhs`. pub(crate) fn and_var( &mut self, lhs: AcirVar, @@ -408,7 +408,7 @@ impl AcirContext { } } - /// Returns an `AcirVar` that is the OR result of `lhs` & `rhs`. + /// Returns an `AcirVar` that is the OR result of `lhs` and `rhs`. pub(crate) fn or_var( &mut self, lhs: AcirVar, @@ -417,7 +417,11 @@ impl AcirContext { ) -> Result { let lhs_expr = self.var_to_expression(lhs)?; let rhs_expr = self.var_to_expression(rhs)?; - if lhs_expr.is_zero() { + + if lhs_expr == rhs_expr { + // x | x == x + return Ok(lhs); + } else if lhs_expr.is_zero() { // 0 | x == x return Ok(rhs); } else if rhs_expr.is_zero() { @@ -428,7 +432,7 @@ impl AcirContext { match typ.to_numeric_type() { NumericType::Signed { bit_size: 1 } | NumericType::Unsigned { bit_size: 1 } => { // Operands are booleans - // a + b - ab + // a + b - a*b let mul = self.mul_var(lhs, rhs)?; let sum = self.add_var(lhs, rhs)?; self.sub_var(sum, mul) @@ -535,7 +539,7 @@ impl AcirContext { Ok(()) } - pub(crate) fn vars_to_expressions_or_memory( + pub(crate) fn values_to_expressions_or_memory( &self, values: &[AcirValue], ) -> Result>, RuntimeError> { @@ -545,9 +549,9 @@ impl AcirContext { AcirValue::Var(var, _) => { result.push(ExpressionOrMemory::Expression(self.var_to_expression(*var)?)); } - AcirValue::Array(vars) => { - let vars_as_vec: Vec<_> = vars.iter().cloned().collect(); - result.extend(self.vars_to_expressions_or_memory(&vars_as_vec)?); + AcirValue::Array(values) => { + let values_as_vec: Vec<_> = values.iter().cloned().collect(); + result.extend(self.values_to_expressions_or_memory(&values_as_vec)?); } AcirValue::DynamicArray(AcirDynamicArray { block_id, .. }) => { result.push(ExpressionOrMemory::Memory(*block_id)); @@ -782,7 +786,7 @@ impl AcirContext { let lhs_const = lhs_const.to_u128(); let rhs_const = rhs_const.to_u128(); let quotient = lhs_const / rhs_const; - let remainder = lhs_const - quotient * rhs_const; + let remainder = lhs_const % rhs_const; let quotient_var = self.add_constant(quotient); let remainder_var = self.add_constant(remainder); @@ -839,7 +843,7 @@ impl AcirContext { // can't assume that the RHS will never have more bits than the operand. // Alternatively if the RHS is a result of an underflow, it could be a negative number which // is represented by a very large positive Field, which could fail to compile to ACIR in - // `range_constrain_var` below, because it can use all 254 bits. + // `range_constrain_var` below, because it can use all Field bits. // To avoid any uncertainty about how the rest of the calls would behave if we pretended that we // didn't know that the RHS has more bits than the operation assumes, we return zero and add an @@ -987,7 +991,7 @@ impl AcirContext { /// if lhs>rhs, rhs-lhs = p+rhs-lhs > p-2^bits >= 2^bits (if log(p) >= bits + 1) /// n.b: we do NOT check here that lhs and rhs are indeed 'bits' size /// lhs < rhs <=> a+1<=b - /// TODO: Consolidate this with bounds_check function. + /// TODO(): Consolidate this with bounds_check function. pub(super) fn bound_constraint_with_offset( &mut self, lhs: AcirVar, @@ -1007,7 +1011,7 @@ impl AcirContext { assert!( bits < F::max_num_bits(), - "range check with bit size of the prime field is not implemented yet" + "range check with bit size >= the prime field size is not implemented yet" ); let mut lhs_offset = self.add_var(lhs, offset)?; @@ -1032,7 +1036,7 @@ impl AcirContext { let two_pow_bit_size_minus_one = if bit_size == 128 { u128::MAX } else { (1_u128 << bit_size) - 1 }; let r = two_pow_bit_size_minus_one - rhs_offset; - // however, since it is a constant, we can compute it's actual bit size + // however, since it is a constant, we can compute its actual bit size let r_bit_size = bit_size_u128(r); //we need to ensure lhs_offset + r does not overflow @@ -1051,8 +1055,8 @@ impl AcirContext { return Ok(()); } } - // General case: lhs_offset<=rhs <=> rhs-lhs_offset>=0 <=> rhs-lhs_offset is a 'bits' bit integer - let sub_expression = self.sub_var(rhs, lhs_offset)?; //rhs-lhs_offset + // General case: lhs_offset<=rhs <=> rhs-lhs_offset>=0 <=> rhs-lhs_offset is a 'bits' bit integer + let sub_expression = self.sub_var(rhs, lhs_offset)?; // rhs-lhs_offset self.range_constrain_var( sub_expression, &NumericType::Unsigned { bit_size: bits }, @@ -1174,18 +1178,18 @@ impl AcirContext { // In other words, `1` means `a >= b` and `0` means `b > a`. // The important thing here is that `c` does not overflow nor underflow the field; // - By construction we have `c >= 0`, so there is no underflow - // - We assert at the beginning that `2^{max_bits+1}` does not overflow the field, so neither c. + // - We assert at the beginning that `2^{max_bits+1}` does not overflow the field, so neither does c. // Ensure that 2^{max_bits + 1} is less than the field size // - // TODO: perhaps this should be a user error, instead of an assert + // TODO(https://github.com/noir-lang/noir/issues/10257): perhaps this should be a user error, instead of an assert assert!(max_bits + 1 < F::max_num_bits()); let two_max_bits = self.add_constant(power_of_two::(max_bits)); let diff = self.sub_var(lhs, rhs)?; let comparison_evaluation = self.add_var(diff, two_max_bits)?; - // Euclidean division by 2^{max_bits} : 2^{max_bits} + a - b = q * 2^{max_bits} + r + // Euclidean division by 2^{max_bits} : 2^{max_bits} + a - b = q * 2^{max_bits} + r // // 2^{max_bits} is of max_bits+1 bit size // If a>b, then a-b is less than 2^{max_bits} - 1, so 2^{max_bits} + a - b is less than 2^{max_bits} + 2^{max_bits} - 1 = 2^{max_bits+1} - 1 @@ -1230,7 +1234,7 @@ impl AcirContext { let comparison = self.more_than_eq_var(lhs, rhs, bit_size)?; let one = self.add_constant(F::one()); - self.sub_var(one, comparison) // comparison_negated + self.sub_var(one, comparison) // comparison negated } /// Returns a vector of `AcirVar`s constrained to be the decomposition of the given input @@ -1246,7 +1250,7 @@ impl AcirContext { result_element_type: AcirType, ) -> Result { let radix = match self.vars[&radix_var].as_constant() { - Some(radix) => radix.to_u128() as u32, + Some(radix) => radix.try_into_u128().expect("expected radix to fit within a u128"), None => { return Err(RuntimeError::InternalError(InternalError::NotAConstant { name: "radix".to_string(), @@ -1262,7 +1266,7 @@ impl AcirContext { let input_expr = self.var_to_expression(input_var)?; - let bit_size = u32::BITS - (radix - 1).leading_zeros(); + let bit_size = u128::BITS - (radix - 1).leading_zeros(); let limbs = self.acir_ir.radix_le_decompose(&input_expr, radix, limb_count, bit_size)?; let mut limb_vars = vecmap(limbs, |witness| { @@ -1410,9 +1414,9 @@ impl AcirContext { let zero_witness = self.var_to_witness(zero)?; vec![zero_witness; len] } - Some(optional_value) => { + Some(value) => { let mut values = Vec::new(); - self.initialize_array_inner(&mut values, optional_value)?; + self.initialize_array_inner(&mut values, value)?; values } }; @@ -1442,13 +1446,11 @@ impl AcirContext { } } AcirValue::DynamicArray(AcirDynamicArray { block_id, len, value_types, .. }) => { - let dynamic_array_values = try_vecmap(0..len, |i| { + for i in 0..len { let index_var = self.add_constant(i); let read = self.read_from_memory(block_id, &index_var)?; let typ = value_types[i % value_types.len()]; - Ok::(AcirValue::Var(read, AcirType::NumericType(typ))) - })?; - for value in dynamic_array_values { + let value = AcirValue::Var(read, AcirType::NumericType(typ)); self.initialize_array_inner(witnesses, value)?; } } diff --git a/compiler/noirc_evaluator/src/acir/arrays.rs b/compiler/noirc_evaluator/src/acir/arrays.rs index 80070fae183..b95b24fbc7e 100644 --- a/compiler/noirc_evaluator/src/acir/arrays.rs +++ b/compiler/noirc_evaluator/src/acir/arrays.rs @@ -11,7 +11,7 @@ //! [BlockId]. This module provides helpers for translating SSA array //! operations into ACIR memory reads and writes. //! -//! ACIR generation use twos different array types for representing arrays: +//! ACIR generation use two different array types for representing arrays: //! //! [Constant arrays][AcirValue::Array] //! - Known at compile time. @@ -71,7 +71,7 @@ //! We then use the resulting type to increment the index appropriately and fetch ever element of `bar.inner`. //! //! This element type sizes array is dynamic as we still need to access it based upon the index which itself can be dynamic. -//! The module will also attempt to not create this array when possible (e.g., we have a simple homogenous array). +//! The module will also attempt to not create this array when possible (e.g., when we have a simple homogenous array). //! //! ### Side effects and Predication //! @@ -98,7 +98,7 @@ //! //! #### Writes //! -//! When the predicate is not a constant one, instead of actually overwriting memory, we compute a "dummy value". +//! When the predicate is not a constant, instead of actually overwriting memory, we compute a "dummy value". //! The dummy value is fetched from the same array at the requested `predicate_index`. //! The store value of an array write is then converted from a `store_value` to `predicate * store_value + (1 - predicate) * dummy` //! This ensures the memory remains unchanged when the write is disabled. In the case of a false predicate, the value stored will be itself. @@ -107,7 +107,7 @@ //! //! If we perform an array read under a false predicate we will read from `offset`. As arrays are not always homogenous //! the result at index `offset` may contain a value that will overflow the resulting type of the array read. -//! When we read a value from a non-homogenous array we multiply any resulting [AcirValue::Var] by the predicate +//! When we read a value from a non-homogenous array, we multiply any resulting [AcirValue::Var] by the predicate //! to avoid any possible mismatch. In the case of a false predicate, the value will now be zero. //! For homogenous arrays, the fallback `offset` will produce a value with a compatible type. //! @@ -322,13 +322,13 @@ impl Context<'_> { store_value: Option, ) -> Result { let array_size: usize = array.len(); - let index = match index.try_to_u64() { + let index = match index.try_to_u32() { 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(), + into: "u32".to_string(), call_stack, }); } @@ -513,7 +513,9 @@ impl Context<'_> { "ICE: The store value and dummy must have the same number of inner values" ); - let values = self.read_dynamic_array(*block_id, *len, value_types)?; + let values: Vec<_> = self + .read_dynamic_array(*block_id, *len, value_types) + .collect::>()?; let mut elements = im::Vector::new(); for (val, dummy_val) in values.iter().zip(dummy_values) { elements.push_back(self.convert_array_set_store_value(val, &dummy_val)?); @@ -935,7 +937,7 @@ impl Context<'_> { AcirValue::Var(_, _) => unreachable!("ICE: attempting to copy a non-array value"), AcirValue::Array(vars) => Ok(vars), AcirValue::DynamicArray(AcirDynamicArray { block_id, len, value_types, .. }) => { - self.read_dynamic_array(block_id, len, &value_types) + self.read_dynamic_array(block_id, len, &value_types).collect() } } } @@ -968,16 +970,15 @@ impl Context<'_> { source: BlockId, array_len: usize, value_types: &[NumericType], - ) -> Result, RuntimeError> { - let init_values = try_vecmap(0..array_len, |i| { + ) -> impl Iterator> { + (0..array_len).map(move |i| { let index_var = self.acir_context.add_constant(i); let read = self.acir_context.read_from_memory(source, &index_var)?; let typ = value_types[i % value_types.len()]; Ok::(AcirValue::Var(read, AcirType::NumericType(typ))) - })?; - Ok(init_values.into()) + }) } fn copy_dynamic_array( @@ -987,7 +988,8 @@ impl Context<'_> { array_len: usize, value_types: &[NumericType], ) -> Result<(), RuntimeError> { - let array = self.read_dynamic_array(source, array_len, value_types)?; + let array = + self.read_dynamic_array(source, array_len, value_types).collect::>()?; self.initialize_array(destination, array_len, Some(AcirValue::Array(array)))?; Ok(()) } diff --git a/compiler/noirc_evaluator/src/acir/call/mod.rs b/compiler/noirc_evaluator/src/acir/call/mod.rs index 2cfdc2e7433..b5931acb838 100644 --- a/compiler/noirc_evaluator/src/acir/call/mod.rs +++ b/compiler/noirc_evaluator/src/acir/call/mod.rs @@ -138,12 +138,13 @@ impl Context<'_> { self.shared_context.generated_brillig_pointer(func.id(), arguments.clone()) { let code = self.shared_context.generated_brillig(generated_pointer.as_usize()); + let safe_return_values = false; self.acir_context.brillig_call( self.current_side_effects_enabled_var, code, inputs, outputs, - false, + safe_return_values, *generated_pointer, None, )? @@ -151,12 +152,13 @@ impl Context<'_> { let code = gen_brillig_for(func, arguments.clone(), self.brillig, self.brillig_options)?; let generated_pointer = self.shared_context.new_generated_pointer(); + let safe_return_values = false; let output_values = self.acir_context.brillig_call( self.current_side_effects_enabled_var, &code, inputs, outputs, - false, + safe_return_values, generated_pointer, None, )?; diff --git a/compiler/noirc_evaluator/src/acir/mod.rs b/compiler/noirc_evaluator/src/acir/mod.rs index 0ec1317b264..dc58ea8e2c2 100644 --- a/compiler/noirc_evaluator/src/acir/mod.rs +++ b/compiler/noirc_evaluator/src/acir/mod.rs @@ -54,7 +54,7 @@ pub use {acir_context::GeneratedAcir, ssa::Artifacts}; /// Context struct for the acir generation pass. /// May be similar to the Evaluator struct in the current SSA IR. struct Context<'a> { - /// Maps SSA values to `AcirVar`. + /// Maps SSA values to `AcirVar`'s. /// /// This is needed so that we only create a single /// AcirVar per SSA value. Before creating an `AcirVar` @@ -77,12 +77,12 @@ struct Context<'a> { /// if there is already a MemoryInit opcode. initialized_arrays: HashSet, - /// Maps SSA values to BlockId + /// Maps SSA values to BlockId's /// A BlockId is an ACIR structure which identifies a memory block /// Each acir memory block corresponds to a different SSA array. memory_blocks: HashMap, BlockId>, - /// Maps SSA values to a BlockId used internally for computing the accurate flattened + /// Maps SSA values to BlockId's used internally for computing the accurate flattened /// index of non-homogenous arrays. /// See [arrays] for more information about the purpose of the type sizes array. /// @@ -233,10 +233,12 @@ impl<'a> Context<'a> { let (return_vars, return_warnings) = self.convert_ssa_return(entry_block.unwrap_terminator(), dfg)?; - // TODO: This is a naive method of assigning the return values to their witnesses as + // This is a naive method of assigning the return values to their witnesses as // we're likely to get a number of constraints which are asserting one witness to be equal to another. // - // We should search through the program and relabel these witnesses so we can remove this constraint. + // But an attempt at searching through the program and relabeling these witnesses so we could remove + // this constraint was [closed](https://github.com/noir-lang/noir/pull/10112#event-20171150226) + // but "the opcode count doesn't even change in real circuits." for (witness_var, return_var) in return_witness_vars.iter().zip(return_vars) { self.acir_context.assert_eq_var(*witness_var, return_var, None)?; } @@ -269,7 +271,7 @@ impl<'a> Context<'a> { })?; let arguments = self.gen_brillig_parameters(dfg[main_func.entry_block()].parameters(), dfg); - let witness_inputs = self.acir_context.extract_witness(&inputs); + let witness_inputs = self.acir_context.extract_witnesses(&inputs); let returns = main_func.returns().unwrap_or_default(); let outputs: Vec = @@ -280,12 +282,13 @@ impl<'a> Context<'a> { // We specifically do not attempt execution of the brillig code being generated as this can result in it being // replaced with constraints on witnesses to the program outputs. + let unsafe_return_values = true; let output_values = self.acir_context.brillig_call( self.current_side_effects_enabled_var, &code, inputs, outputs, - true, + unsafe_return_values, // We are guaranteed to have a Brillig function pointer of `0` as main itself is marked as unconstrained BrilligFunctionId(0), None, @@ -392,10 +395,9 @@ impl<'a> Context<'a> { ) -> Result { let acir_var = self.acir_context.add_variable(); let one = self.acir_context.add_constant(FieldElement::one()); - if matches!(numeric_type, NumericType::Signed { .. } | NumericType::Unsigned { .. }) { - // The predicate is one so that this constraint is is always applied. - self.acir_context.range_constrain_var(acir_var, numeric_type, None, one)?; - } + // The predicate is one so that this constraint is is always applied to Signed/Unsigned + // NumericType's + self.acir_context.range_constrain_var(acir_var, numeric_type, None, one)?; Ok(acir_var) } @@ -409,16 +411,15 @@ impl<'a> Context<'a> { let instruction = &dfg[instruction_id]; self.acir_context.set_call_stack(dfg.get_instruction_call_stack(instruction_id)); let mut warnings = Vec::new(); - // Disable the side effects if the binary instruction does not require them - let one = self.acir_context.add_constant(FieldElement::one()); - let predicate = if instruction.requires_acir_gen_predicate(dfg) { - self.current_side_effects_enabled_var - } else { - one - }; match instruction { Instruction::Binary(binary) => { + // Disable the side effects if the binary instruction does not require them + let predicate = if instruction.requires_acir_gen_predicate(dfg) { + self.current_side_effects_enabled_var + } else { + self.acir_context.add_constant(FieldElement::one()) + }; let result_acir_var = self.convert_ssa_binary(binary, dfg, predicate)?; self.define_result_var(dfg, instruction_id, result_acir_var); } @@ -480,6 +481,7 @@ impl<'a> Context<'a> { } Instruction::RangeCheck { value, max_bit_size, assert_message } => { let acir_var = self.convert_numeric_value(*value, dfg)?; + let one = self.acir_context.add_constant(FieldElement::one()); // Predicate is one because the predicate has already been // handled in the RangeCheck instruction during the flattening pass. self.acir_context.range_constrain_var( @@ -525,10 +527,11 @@ impl<'a> Context<'a> { { self.acir_context.generate_assertion_message_payload(constant_string) } else { - let acir_vars: Vec<_> = vecmap(values, |value| self.convert_value(*value, dfg)); + let acir_values: Vec<_> = + vecmap(values, |value| self.convert_value(*value, dfg)); let expressions_or_memory = - self.acir_context.vars_to_expressions_or_memory(&acir_vars)?; + self.acir_context.values_to_expressions_or_memory(&acir_values)?; let error_selector = error_selector.as_u64(); AssertionPayload { error_selector, payload: expressions_or_memory } @@ -635,6 +638,11 @@ impl<'a> Context<'a> { /// involving such values are evaluated via a separate path and stored in /// `ssa_value_to_array_address` instead. fn convert_value(&mut self, value_id: ValueId, dfg: &DataFlowGraph) -> AcirValue { + assert!( + !matches!(dfg.type_of_value(value_id), Type::Reference(_)), + "convert_value: did not expect a Reference type" + ); + let value = &dfg[value_id]; if let Some(acir_value) = self.ssa_values.get(&value_id) { return acir_value.clone(); @@ -866,12 +874,12 @@ impl<'a> Context<'a> { result } AcirValue::DynamicArray(AcirDynamicArray { block_id, len, value_types, .. }) => { - let elements = self.read_dynamic_array(*block_id, *len, value_types)?; + let elements = self.read_dynamic_array(*block_id, *len, value_types); let mut result = Vec::new(); for value in elements { - match value { - AcirValue::Var(var, _) => result.push(var), + match value? { + AcirValue::Var(var, _typ) => result.push(var), _ => unreachable!("ICE: Dynamic memory should already be flat"), } } diff --git a/compiler/noirc_evaluator/src/acir/shared_context.rs b/compiler/noirc_evaluator/src/acir/shared_context.rs index ffe5b79b3d3..c2f262b62f8 100644 --- a/compiler/noirc_evaluator/src/acir/shared_context.rs +++ b/compiler/noirc_evaluator/src/acir/shared_context.rs @@ -98,6 +98,11 @@ impl SharedContext { /// Finalize this context, consuming it and returning all generated Brillig functions. pub(super) fn finish(self) -> Vec> { + assert_eq!( + self.brillig_stdlib_calls_to_resolve.len(), + 0, + "expected zero remaining 'brillig_stdlib_calls_to_resolve'" + ); self.generated_brillig } @@ -155,12 +160,12 @@ impl SharedContext { self.brillig_stdlib_calls_to_resolve.entry(func_id).or_default().push(call_to_resolve); } - /// Get the list of unresolved stdlib call sites for a given function - pub(super) fn get_call_to_resolve( - &self, + /// Get and remove the list of unresolved stdlib call sites for a given function + pub(super) fn remove_call_to_resolve( + &mut self, func_id: FunctionId, - ) -> Option<&Vec<(OpcodeLocation, BrilligFunctionId)>> { - self.brillig_stdlib_calls_to_resolve.get(&func_id) + ) -> Option> { + self.brillig_stdlib_calls_to_resolve.remove(&func_id) } /// Remove and return the set of globals used by the given function, @@ -285,7 +290,7 @@ mod tests { } // Calls to resolve should be 2 per variant - let calls = context.get_call_to_resolve(func_id).unwrap(); + let calls = context.remove_call_to_resolve(func_id).unwrap(); assert_eq!(calls.len(), variants.len() * 2); // Check that each call matches the expected stdlib function pointer diff --git a/compiler/noirc_evaluator/src/acir/ssa.rs b/compiler/noirc_evaluator/src/acir/ssa.rs index e1875a3ae74..21bd895f2b3 100644 --- a/compiler/noirc_evaluator/src/acir/ssa.rs +++ b/compiler/noirc_evaluator/src/acir/ssa.rs @@ -43,7 +43,7 @@ pub(super) fn codegen_acir( let used_globals = ssa.used_globals_in_functions(); - // TODO: can we parallelize this? + // TODO(https://github.com/noir-lang/noir/issues/10269): can we parallelize this? let mut shared_context = SharedContext::new(brillig_stdlib.clone(), used_globals); for function in ssa.functions.values() { @@ -72,12 +72,12 @@ pub(super) fn codegen_acir( } // Fetch the Brillig stdlib calls to resolve for this function - if let Some(calls_to_resolve) = shared_context.get_call_to_resolve(function.id()) { + if let Some(calls_to_resolve) = shared_context.remove_call_to_resolve(function.id()) { // Resolve the Brillig stdlib calls // We have to do a separate loop as the generated ACIR cannot be borrowed as mutable after an immutable borrow for (opcode_location, brillig_function_pointer) in calls_to_resolve { generated_acir - .resolve_brillig_stdlib_call(*opcode_location, *brillig_function_pointer); + .resolve_brillig_stdlib_call(opcode_location, brillig_function_pointer); } } diff --git a/compiler/noirc_evaluator/src/acir/tests/arrays.rs b/compiler/noirc_evaluator/src/acir/tests/arrays.rs index 6e726b772c7..ab518a16301 100644 --- a/compiler/noirc_evaluator/src/acir/tests/arrays.rs +++ b/compiler/noirc_evaluator/src/acir/tests/arrays.rs @@ -140,6 +140,13 @@ fn constant_array_access_in_bounds() { // We know the circuit above to be trivially true assert_eq!(program.functions.len(), 1); assert_eq!(program.functions[0].opcodes.len(), 0); + + assert_circuit_snapshot!(program, @r" + func 0 + private parameters: [] + public parameters: [] + return values: [] + "); } #[test] diff --git a/compiler/noirc_evaluator/src/acir/tests/mod.rs b/compiler/noirc_evaluator/src/acir/tests/mod.rs index a09a0999ca4..39bf3161b5d 100644 --- a/compiler/noirc_evaluator/src/acir/tests/mod.rs +++ b/compiler/noirc_evaluator/src/acir/tests/mod.rs @@ -243,13 +243,10 @@ fn properly_constrains_quotient_when_truncating_fields() { let main = &acir_functions[0]; let initial_witness = WitnessMap::from(BTreeMap::from([(Witness(0), input)])); - let mut acvm = ACVM::new( - &StubbedBlackBoxSolver(true), - main.opcodes(), - initial_witness, - &brillig_functions, - &[], - ); + let pedantic_solving = true; + let blackbox_solver = StubbedBlackBoxSolver(pedantic_solving); + let mut acvm = + ACVM::new(&blackbox_solver, main.opcodes(), initial_witness, &brillig_functions, &[]); assert!(matches!(acvm.solve(), ACVMStatus::Failure::(_))); } @@ -310,13 +307,10 @@ fn execute_ssa( .expect("Should compile manually written SSA into ACIR"); assert_eq!(acir_functions.len(), 1); let main = &acir_functions[0]; - let mut acvm = ACVM::new( - &StubbedBlackBoxSolver(true), - main.opcodes(), - initial_witness, - &brillig_functions, - &[], - ); + let pedantic_solving = true; + let blackbox_solver = StubbedBlackBoxSolver(pedantic_solving); + let mut acvm = + ACVM::new(&blackbox_solver, main.opcodes(), initial_witness, &brillig_functions, &[]); let status = acvm.solve(); if status == ACVMStatus::Solved { (status, output.map(|o| acvm.witness_map()[o])) @@ -416,9 +410,9 @@ fn test_operators( let acir_execution_result = execute_ssa(ssa, initial_witness.clone(), output.as_ref()); match (ssa_interpreter_result, acir_execution_result) { - // Both execution failed, so it is the same behavior, as expected. + // Both executions failed, so it is the same behavior, as expected. (Err(_), (ACVMStatus::Failure(_), _)) => (), - // Both execution succeeded and output the same value + // Both executions succeeded and output the same value (Ok(ssa_inner_result), (ACVMStatus::Solved, acvm_result)) => { let ssa_result = if let Some(result) = ssa_inner_result.first() { result.as_numeric().map(|v| v.convert_to_field()) @@ -433,113 +427,136 @@ fn test_operators( } proptest! { -#[test] -fn test_binary_on_field(lhs in 0u128.., rhs in 0u128..) { - let lhs = FieldElement::from(lhs); - let rhs = FieldElement::from(rhs); - // Test the following Binary operation on Fields - let operators = [ - "add", - "sub", - "mul", - "div", - "eq", - // Bitwise operations are not allowed on field elements - // SSA interpreter will emit an error but not ACVM - // "and", - // "xor", - "unchecked_add", - "unchecked_sub", - "unchecked_mul", - "range_check 32", - "truncate 32 254", - ]; - let inputs = [lhs, rhs]; - test_operators(&operators, "Field", &inputs); -} - -#[test] -fn test_u32(lhs in 0u32.., rhs in 0u32..) { - let lhs = FieldElement::from(lhs); - let rhs = FieldElement::from(rhs); - - // Test the following operations on u32 - let operators = [ - "add", - "sub", - "mul", - "div", - "eq", - "and", - "xor", - "mod", - "lt", - "or", - "not", - "range_check 8", - "truncate 8 32", - ]; - let inputs = [lhs, rhs]; - test_operators(&operators, "u32", &inputs); - - //unchecked operations assume no under/over-flow - let mut unchecked_operators = vec![]; - if (lhs + rhs).to_u128() <= u128::from(u32::MAX) { - unchecked_operators.push("unchecked_add"); + #[test] + fn test_binary_on_field(lhs in 0u128.., rhs in 0u128..) { + let lhs = FieldElement::from(lhs); + let rhs = FieldElement::from(rhs); + // Test the following Binary operation on Fields + let operators = [ + "add", + "sub", + "mul", + "div", + "eq", + // Bitwise operations are not allowed on field elements + // SSA interpreter will emit an error but not ACVM + // "and", + // "xor", + "unchecked_add", + "unchecked_sub", + "unchecked_mul", + "range_check 32", + "truncate 32 254", + ]; + let inputs = [lhs, rhs]; + test_operators(&operators, "Field", &inputs); } - if (lhs * rhs).to_u128() <= u128::from(u32::MAX) { - unchecked_operators.push("unchecked_mul"); + + #[test] + #[should_panic(expected = "Cannot use `and` with field elements")] + fn test_and_on_field(lhs in 0u128.., rhs in 0u128..) { + let lhs = FieldElement::from(lhs); + let rhs = FieldElement::from(rhs); + let operators = [ + "and", + ]; + let inputs = [lhs, rhs]; + test_operators(&operators, "Field", &inputs); } - if lhs >= rhs { - unchecked_operators.push("unchecked_sub"); + + #[test] + #[should_panic(expected = "Cannot use `xor` with field elements")] + fn test_xor_on_field(lhs in 0u128.., rhs in 0u128..) { + let lhs = FieldElement::from(lhs); + let rhs = FieldElement::from(rhs); + let operators = [ + "xor", + ]; + let inputs = [lhs, rhs]; + test_operators(&operators, "Field", &inputs); } - test_operators(&unchecked_operators, "u32", &inputs); -} + #[test] + fn test_u32(lhs in 0u32.., rhs in 0u32..) { + let lhs = FieldElement::from(lhs); + let rhs = FieldElement::from(rhs); + + // Test the following operations on u32 + let operators = [ + "add", + "sub", + "mul", + "div", + "eq", + "and", + "xor", + "mod", + "lt", + "or", + "not", + "range_check 8", + "truncate 8 32", + ]; + let inputs = [lhs, rhs]; + test_operators(&operators, "u32", &inputs); + + //unchecked operations assume no under/over-flow + let mut unchecked_operators = vec![]; + if (lhs + rhs).to_u128() <= u128::from(u32::MAX) { + unchecked_operators.push("unchecked_add"); + } + if (lhs * rhs).to_u128() <= u128::from(u32::MAX) { + unchecked_operators.push("unchecked_mul"); + } + if lhs >= rhs { + unchecked_operators.push("unchecked_sub"); + } + test_operators(&unchecked_operators, "u32", &inputs); + } - #[test] -fn test_constraint_field(lhs in 0u128.., rhs in 0u128..) { - let lhs = FieldElement::from(lhs); - let rhs = FieldElement::from(rhs); - let operators = ["constrain ==", "constrain !="]; - test_operators(&operators, "Field", &[lhs,rhs]); - test_operators(&operators, "u128", &[lhs,rhs]); -} -#[test] -fn test_constraint_u32(lhs in 0u32.., rhs in 0u32..) { - let lhs = FieldElement::from(lhs); - let rhs = FieldElement::from(rhs); - let operators = ["constrain ==", "constrain !="]; - test_operators(&operators, "u32", &[lhs,rhs]); - test_operators(&operators, "i32", &[lhs,rhs]); -} + #[test] + fn test_constraint_field(lhs in 0u128.., rhs in 0u128..) { + let lhs = FieldElement::from(lhs); + let rhs = FieldElement::from(rhs); + let operators = ["constrain ==", "constrain !="]; + test_operators(&operators, "Field", &[lhs,rhs]); + test_operators(&operators, "u128", &[lhs,rhs]); + } -#[test] -fn test_constraint_u64(lhs in 0u64.., rhs in 0u64..) { - let lhs = FieldElement::from(lhs); - let rhs = FieldElement::from(rhs); - let operators = ["constrain ==", "constrain !="]; - test_operators(&operators, "u64", &[lhs,rhs]); - test_operators(&operators, "i64", &[lhs,rhs]); -} + #[test] + fn test_constraint_u32(lhs in 0u32.., rhs in 0u32..) { + let lhs = FieldElement::from(lhs); + let rhs = FieldElement::from(rhs); + let operators = ["constrain ==", "constrain !="]; + test_operators(&operators, "u32", &[lhs,rhs]); + test_operators(&operators, "i32", &[lhs,rhs]); + } -#[test] -fn test_constraint_u16(lhs in 0u16.., rhs in 0u16..) { - let lhs = FieldElement::from(u128::from(lhs)); - let rhs = FieldElement::from(u128::from(rhs)); - let operators = ["constrain ==", "constrain !="]; - test_operators(&operators, "u16", &[lhs,rhs]); - test_operators(&operators, "i16", &[lhs,rhs]); -} + #[test] + fn test_constraint_u64(lhs in 0u64.., rhs in 0u64..) { + let lhs = FieldElement::from(lhs); + let rhs = FieldElement::from(rhs); + let operators = ["constrain ==", "constrain !="]; + test_operators(&operators, "u64", &[lhs,rhs]); + test_operators(&operators, "i64", &[lhs,rhs]); + } -#[test] -fn test_constraint_u8(lhs in 0u8.., rhs in 0u8..) { - let lhs = FieldElement::from(u128::from(lhs)); - let rhs = FieldElement::from(u128::from(rhs)); - let operators = ["constrain ==", "constrain !="]; - test_operators(&operators, "u8", &[lhs,rhs]); - test_operators(&operators, "i8", &[lhs,rhs]); -} + #[test] + fn test_constraint_u16(lhs in 0u16.., rhs in 0u16..) { + let lhs = FieldElement::from(u128::from(lhs)); + let rhs = FieldElement::from(u128::from(rhs)); + let operators = ["constrain ==", "constrain !="]; + test_operators(&operators, "u16", &[lhs,rhs]); + test_operators(&operators, "i16", &[lhs,rhs]); + } + #[test] + fn test_constraint_u8(lhs in 0u8.., rhs in 0u8..) { + let lhs = FieldElement::from(u128::from(lhs)); + let rhs = FieldElement::from(u128::from(rhs)); + let operators = ["constrain ==", "constrain !="]; + test_operators(&operators, "u8", &[lhs,rhs]); + test_operators(&operators, "i8", &[lhs,rhs]); + } }