diff --git a/.noir-sync-commit b/.noir-sync-commit index 2581c731215..1879b321c90 100644 --- a/.noir-sync-commit +++ b/.noir-sync-commit @@ -1 +1 @@ -55545d630a5b338cf97068d23695779c32e5109b \ No newline at end of file +c3deb6ab504df75ae8c90d483d53083c6cd8d443 diff --git a/noir/noir-repo/acvm-repo/brillig_vm/src/arithmetic.rs b/noir/noir-repo/acvm-repo/brillig_vm/src/arithmetic.rs index 527b0a7849e..a87635bd542 100644 --- a/noir/noir-repo/acvm-repo/brillig_vm/src/arithmetic.rs +++ b/noir/noir-repo/acvm-repo/brillig_vm/src/arithmetic.rs @@ -1,7 +1,9 @@ -use acir::brillig::{BinaryFieldOp, BinaryIntOp, IntegerBitSize}; +use std::ops::{BitAnd, BitOr, BitXor, Shl, Shr}; + +use acir::brillig::{BinaryFieldOp, BinaryIntOp, BitSize, IntegerBitSize}; use acir::AcirField; use num_bigint::BigUint; -use num_traits::{AsPrimitive, PrimInt, WrappingAdd, WrappingMul, WrappingSub}; +use num_traits::{CheckedDiv, WrappingAdd, WrappingMul, WrappingSub, Zero}; use crate::memory::{MemoryTypeError, MemoryValue}; @@ -21,24 +23,20 @@ pub(crate) fn evaluate_binary_field_op( lhs: MemoryValue, rhs: MemoryValue, ) -> Result, BrilligArithmeticError> { - let a = match lhs { - MemoryValue::Field(a) => a, - MemoryValue::Integer(_, bit_size) => { - return Err(BrilligArithmeticError::MismatchedLhsBitSize { - lhs_bit_size: bit_size.into(), - op_bit_size: F::max_num_bits(), - }); + let a = *lhs.expect_field().map_err(|err| { + let MemoryTypeError::MismatchedBitSize { value_bit_size, expected_bit_size } = err; + BrilligArithmeticError::MismatchedLhsBitSize { + lhs_bit_size: value_bit_size, + op_bit_size: expected_bit_size, } - }; - let b = match rhs { - MemoryValue::Field(b) => b, - MemoryValue::Integer(_, bit_size) => { - return Err(BrilligArithmeticError::MismatchedRhsBitSize { - rhs_bit_size: bit_size.into(), - op_bit_size: F::max_num_bits(), - }); + })?; + let b = *rhs.expect_field().map_err(|err| { + let MemoryTypeError::MismatchedBitSize { value_bit_size, expected_bit_size } = err; + BrilligArithmeticError::MismatchedRhsBitSize { + rhs_bit_size: value_bit_size, + op_bit_size: expected_bit_size, } - }; + })?; Ok(match op { // Perform addition, subtraction, multiplication, and division based on the BinaryOp variant. @@ -70,46 +68,120 @@ pub(crate) fn evaluate_binary_int_op( rhs: MemoryValue, bit_size: IntegerBitSize, ) -> Result, BrilligArithmeticError> { - let lhs = lhs.expect_integer_with_bit_size(bit_size).map_err(|err| match err { - MemoryTypeError::MismatchedBitSize { value_bit_size, expected_bit_size } => { - BrilligArithmeticError::MismatchedLhsBitSize { - lhs_bit_size: value_bit_size, - op_bit_size: expected_bit_size, + match op { + BinaryIntOp::Add + | BinaryIntOp::Sub + | BinaryIntOp::Mul + | BinaryIntOp::Div + | BinaryIntOp::And + | BinaryIntOp::Or + | BinaryIntOp::Xor => match (lhs, rhs, bit_size) { + (MemoryValue::U1(lhs), MemoryValue::U1(rhs), IntegerBitSize::U1) => { + evaluate_binary_int_op_u1(op, lhs, rhs).map(MemoryValue::U1) } - } - })?; - - let rhs_bit_size = if op == &BinaryIntOp::Shl || op == &BinaryIntOp::Shr { - IntegerBitSize::U8 - } else { - bit_size - }; + (MemoryValue::U8(lhs), MemoryValue::U8(rhs), IntegerBitSize::U8) => { + evaluate_binary_int_op_arith(op, lhs, rhs).map(MemoryValue::U8) + } + (MemoryValue::U16(lhs), MemoryValue::U16(rhs), IntegerBitSize::U16) => { + evaluate_binary_int_op_arith(op, lhs, rhs).map(MemoryValue::U16) + } + (MemoryValue::U32(lhs), MemoryValue::U32(rhs), IntegerBitSize::U32) => { + evaluate_binary_int_op_arith(op, lhs, rhs).map(MemoryValue::U32) + } + (MemoryValue::U64(lhs), MemoryValue::U64(rhs), IntegerBitSize::U64) => { + evaluate_binary_int_op_arith(op, lhs, rhs).map(MemoryValue::U64) + } + (MemoryValue::U128(lhs), MemoryValue::U128(rhs), IntegerBitSize::U128) => { + evaluate_binary_int_op_arith(op, lhs, rhs).map(MemoryValue::U128) + } + (lhs, _, _) if lhs.bit_size() != BitSize::Integer(bit_size) => { + Err(BrilligArithmeticError::MismatchedLhsBitSize { + lhs_bit_size: lhs.bit_size().to_u32::(), + op_bit_size: bit_size.into(), + }) + } + (_, rhs, _) if rhs.bit_size() != BitSize::Integer(bit_size) => { + Err(BrilligArithmeticError::MismatchedRhsBitSize { + rhs_bit_size: rhs.bit_size().to_u32::(), + op_bit_size: bit_size.into(), + }) + } + _ => unreachable!("Invalid arguments are covered by the two arms above."), + }, - let rhs = rhs.expect_integer_with_bit_size(rhs_bit_size).map_err(|err| match err { - MemoryTypeError::MismatchedBitSize { value_bit_size, expected_bit_size } => { - BrilligArithmeticError::MismatchedRhsBitSize { - rhs_bit_size: value_bit_size, - op_bit_size: expected_bit_size, + BinaryIntOp::Equals | BinaryIntOp::LessThan | BinaryIntOp::LessThanEquals => { + match (lhs, rhs, bit_size) { + (MemoryValue::U1(lhs), MemoryValue::U1(rhs), IntegerBitSize::U1) => { + Ok(MemoryValue::U1(evaluate_binary_int_op_cmp(op, lhs, rhs))) + } + (MemoryValue::U8(lhs), MemoryValue::U8(rhs), IntegerBitSize::U8) => { + Ok(MemoryValue::U1(evaluate_binary_int_op_cmp(op, lhs, rhs))) + } + (MemoryValue::U16(lhs), MemoryValue::U16(rhs), IntegerBitSize::U16) => { + Ok(MemoryValue::U1(evaluate_binary_int_op_cmp(op, lhs, rhs))) + } + (MemoryValue::U32(lhs), MemoryValue::U32(rhs), IntegerBitSize::U32) => { + Ok(MemoryValue::U1(evaluate_binary_int_op_cmp(op, lhs, rhs))) + } + (MemoryValue::U64(lhs), MemoryValue::U64(rhs), IntegerBitSize::U64) => { + Ok(MemoryValue::U1(evaluate_binary_int_op_cmp(op, lhs, rhs))) + } + (MemoryValue::U128(lhs), MemoryValue::U128(rhs), IntegerBitSize::U128) => { + Ok(MemoryValue::U1(evaluate_binary_int_op_cmp(op, lhs, rhs))) + } + (lhs, _, _) if lhs.bit_size() != BitSize::Integer(bit_size) => { + Err(BrilligArithmeticError::MismatchedLhsBitSize { + lhs_bit_size: lhs.bit_size().to_u32::(), + op_bit_size: bit_size.into(), + }) + } + (_, rhs, _) if rhs.bit_size() != BitSize::Integer(bit_size) => { + Err(BrilligArithmeticError::MismatchedRhsBitSize { + rhs_bit_size: rhs.bit_size().to_u32::(), + op_bit_size: bit_size.into(), + }) + } + _ => unreachable!("Invalid arguments are covered by the two arms above."), } } - })?; - - // `lhs` and `rhs` are asserted to fit within their given types when being read from memory so this is safe. - let result = match bit_size { - IntegerBitSize::U1 => evaluate_binary_int_op_u1(op, lhs != 0, rhs != 0)?.into(), - IntegerBitSize::U8 => evaluate_binary_int_op_num(op, lhs as u8, rhs as u8, 8)?.into(), - IntegerBitSize::U16 => evaluate_binary_int_op_num(op, lhs as u16, rhs as u16, 16)?.into(), - IntegerBitSize::U32 => evaluate_binary_int_op_num(op, lhs as u32, rhs as u32, 32)?.into(), - IntegerBitSize::U64 => evaluate_binary_int_op_num(op, lhs as u64, rhs as u64, 64)?.into(), - IntegerBitSize::U128 => evaluate_binary_int_op_num(op, lhs, rhs, 128)?, - }; - Ok(match op { - BinaryIntOp::Equals | BinaryIntOp::LessThan | BinaryIntOp::LessThanEquals => { - MemoryValue::new_integer(result, IntegerBitSize::U1) + BinaryIntOp::Shl | BinaryIntOp::Shr => { + let rhs = rhs.expect_u8().map_err( + |MemoryTypeError::MismatchedBitSize { value_bit_size, expected_bit_size }| { + BrilligArithmeticError::MismatchedRhsBitSize { + rhs_bit_size: value_bit_size, + op_bit_size: expected_bit_size, + } + }, + )?; + + match (lhs, bit_size) { + (MemoryValue::U1(lhs), IntegerBitSize::U1) => { + let result = if rhs == 0 { lhs } else { false }; + Ok(MemoryValue::U1(result)) + } + (MemoryValue::U8(lhs), IntegerBitSize::U8) => { + Ok(MemoryValue::U8(evaluate_binary_int_op_shifts(op, lhs, rhs))) + } + (MemoryValue::U16(lhs), IntegerBitSize::U16) => { + Ok(MemoryValue::U16(evaluate_binary_int_op_shifts(op, lhs, rhs))) + } + (MemoryValue::U32(lhs), IntegerBitSize::U32) => { + Ok(MemoryValue::U32(evaluate_binary_int_op_shifts(op, lhs, rhs))) + } + (MemoryValue::U64(lhs), IntegerBitSize::U64) => { + Ok(MemoryValue::U64(evaluate_binary_int_op_shifts(op, lhs, rhs))) + } + (MemoryValue::U128(lhs), IntegerBitSize::U128) => { + Ok(MemoryValue::U128(evaluate_binary_int_op_shifts(op, lhs, rhs))) + } + _ => Err(BrilligArithmeticError::MismatchedLhsBitSize { + lhs_bit_size: lhs.bit_size().to_u32::(), + op_bit_size: bit_size.into(), + }), + } } - _ => MemoryValue::new_integer(result, bit_size), - }) + } } fn evaluate_binary_int_op_u1( @@ -118,8 +190,12 @@ fn evaluate_binary_int_op_u1( rhs: bool, ) -> Result { let result = match op { - BinaryIntOp::Add | BinaryIntOp::Sub => lhs ^ rhs, - BinaryIntOp::Mul => lhs & rhs, + BinaryIntOp::Equals => lhs == rhs, + BinaryIntOp::LessThan => !lhs & rhs, + BinaryIntOp::LessThanEquals => lhs <= rhs, + BinaryIntOp::And | BinaryIntOp::Mul => lhs & rhs, + BinaryIntOp::Or => lhs | rhs, + BinaryIntOp::Xor | BinaryIntOp::Add | BinaryIntOp::Sub => lhs ^ rhs, BinaryIntOp::Div => { if !rhs { return Err(BrilligArithmeticError::DivisionByZero); @@ -127,58 +203,70 @@ fn evaluate_binary_int_op_u1( lhs } } + _ => unreachable!("Operator not handled by this function: {op:?}"), + }; + Ok(result) +} + +fn evaluate_binary_int_op_cmp(op: &BinaryIntOp, lhs: T, rhs: T) -> bool { + match op { BinaryIntOp::Equals => lhs == rhs, - BinaryIntOp::LessThan => !lhs & rhs, + BinaryIntOp::LessThan => lhs < rhs, BinaryIntOp::LessThanEquals => lhs <= rhs, - BinaryIntOp::And => lhs & rhs, - BinaryIntOp::Or => lhs | rhs, - BinaryIntOp::Xor => lhs ^ rhs, - BinaryIntOp::Shl | BinaryIntOp::Shr => { - if rhs { - false + _ => unreachable!("Operator not handled by this function: {op:?}"), + } +} + +fn evaluate_binary_int_op_shifts + Zero + Shl + Shr>( + op: &BinaryIntOp, + lhs: T, + rhs: u8, +) -> T { + match op { + BinaryIntOp::Shl => { + let rhs_usize: usize = rhs as usize; + #[allow(unused_qualifications)] + if rhs_usize >= 8 * std::mem::size_of::() { + T::zero() } else { - lhs + lhs << rhs.into() } } - }; - Ok(result) + BinaryIntOp::Shr => { + let rhs_usize: usize = rhs as usize; + #[allow(unused_qualifications)] + if rhs_usize >= 8 * std::mem::size_of::() { + T::zero() + } else { + lhs >> rhs.into() + } + } + _ => unreachable!("Operator not handled by this function: {op:?}"), + } } -fn evaluate_binary_int_op_num< - T: PrimInt + AsPrimitive + From + WrappingAdd + WrappingSub + WrappingMul, +fn evaluate_binary_int_op_arith< + T: WrappingAdd + + WrappingSub + + WrappingMul + + CheckedDiv + + BitAnd + + BitOr + + BitXor, >( op: &BinaryIntOp, lhs: T, rhs: T, - num_bits: usize, ) -> Result { let result = match op { BinaryIntOp::Add => lhs.wrapping_add(&rhs), BinaryIntOp::Sub => lhs.wrapping_sub(&rhs), BinaryIntOp::Mul => lhs.wrapping_mul(&rhs), BinaryIntOp::Div => lhs.checked_div(&rhs).ok_or(BrilligArithmeticError::DivisionByZero)?, - BinaryIntOp::Equals => (lhs == rhs).into(), - BinaryIntOp::LessThan => (lhs < rhs).into(), - BinaryIntOp::LessThanEquals => (lhs <= rhs).into(), BinaryIntOp::And => lhs & rhs, BinaryIntOp::Or => lhs | rhs, BinaryIntOp::Xor => lhs ^ rhs, - BinaryIntOp::Shl => { - let rhs_usize = rhs.as_(); - if rhs_usize >= num_bits { - T::zero() - } else { - lhs << rhs_usize - } - } - BinaryIntOp::Shr => { - let rhs_usize = rhs.as_(); - if rhs_usize >= num_bits { - T::zero() - } else { - lhs >> rhs_usize - } - } + _ => unreachable!("Operator not handled by this function: {op:?}"), }; Ok(result) } diff --git a/noir/noir-repo/acvm-repo/brillig_vm/src/black_box.rs b/noir/noir-repo/acvm-repo/brillig_vm/src/black_box.rs index bc8b1f3c230..ab0584d0d80 100644 --- a/noir/noir-repo/acvm-repo/brillig_vm/src/black_box.rs +++ b/noir/noir-repo/acvm-repo/brillig_vm/src/black_box.rs @@ -1,4 +1,4 @@ -use acir::brillig::{BlackBoxOp, HeapArray, HeapVector, IntegerBitSize}; +use acir::brillig::{BlackBoxOp, HeapArray, HeapVector}; use acir::{AcirField, BlackBoxFunc}; use acvm_blackbox_solver::{ aes128_encrypt, blake2s, blake3, ecdsa_secp256k1_verify, ecdsa_secp256r1_verify, keccakf1600, @@ -312,16 +312,13 @@ pub(crate) fn evaluate_black_box } BlackBoxOp::ToRadix { input, radix, output_pointer, num_limbs, output_bits } => { let input: F = *memory.read(*input).extract_field().expect("ToRadix input not a field"); - let radix = memory - .read(*radix) - .expect_integer_with_bit_size(IntegerBitSize::U32) - .expect("ToRadix opcode's radix bit size does not match expected bit size 32"); + let MemoryValue::U32(radix) = memory.read(*radix) else { + panic!("ToRadix opcode's radix bit size does not match expected bit size 32") + }; let num_limbs = memory.read(*num_limbs).to_usize(); - let output_bits = !memory - .read(*output_bits) - .expect_integer_with_bit_size(IntegerBitSize::U1) - .expect("ToRadix opcode's output_bits size does not match expected bit size 1") - .is_zero(); + let MemoryValue::U1(output_bits) = memory.read(*output_bits) else { + panic!("ToRadix opcode's output_bits size does not match expected bit size 1") + }; let mut input = BigUint::from_bytes_be(&input.to_be_bytes()); let radix = BigUint::from_bytes_be(&radix.to_be_bytes()); @@ -349,13 +346,10 @@ pub(crate) fn evaluate_black_box for i in (0..num_limbs).rev() { let limb = &input % &radix; if output_bits { - limbs[i] = MemoryValue::new_integer( - if limb.is_zero() { 0 } else { 1 }, - IntegerBitSize::U1, - ); + limbs[i] = MemoryValue::U1(!limb.is_zero()); } else { let limb: u8 = limb.try_into().unwrap(); - limbs[i] = MemoryValue::new_integer(limb as u128, IntegerBitSize::U8); + limbs[i] = MemoryValue::U8(limb); }; input /= &radix; } diff --git a/noir/noir-repo/acvm-repo/brillig_vm/src/lib.rs b/noir/noir-repo/acvm-repo/brillig_vm/src/lib.rs index 9a40022a3ba..f3eb3211e8e 100644 --- a/noir/noir-repo/acvm-repo/brillig_vm/src/lib.rs +++ b/noir/noir-repo/acvm-repo/brillig_vm/src/lib.rs @@ -21,6 +21,7 @@ use black_box::{evaluate_black_box, BrilligBigIntSolver}; // Re-export `brillig`. pub use acir::brillig; +use memory::MemoryTypeError; pub use memory::{Memory, MemoryValue, MEMORY_ADDRESSING_BIT_SIZE}; mod arithmetic; @@ -248,7 +249,7 @@ impl<'a, F: AcirField, B: BlackBoxFunctionSolver> VM<'a, F, B> { } Opcode::Not { destination, source, bit_size } => { if let Err(error) = self.process_not(*source, *destination, *bit_size) { - self.fail(error) + self.fail(error.to_string()) } else { self.increment_program_counter() } @@ -775,63 +776,91 @@ impl<'a, F: AcirField, B: BlackBoxFunctionSolver> VM<'a, F, B> { source: MemoryAddress, destination: MemoryAddress, op_bit_size: IntegerBitSize, - ) -> Result<(), String> { - let (value, bit_size) = self - .memory - .read(source) - .extract_integer() - .ok_or("Not opcode source is not an integer")?; - - if bit_size != op_bit_size { - return Err(format!( - "Not opcode source bit size {} does not match expected bit size {}", - bit_size, op_bit_size - )); - } - - let negated_value = if let IntegerBitSize::U128 = bit_size { - !value - } else { - let bit_size: u32 = bit_size.into(); - let mask = (1_u128 << bit_size as u128) - 1; - (!value) & mask + ) -> Result<(), MemoryTypeError> { + let value = self.memory.read(source); + + let negated_value = match op_bit_size { + IntegerBitSize::U1 => MemoryValue::U1(!value.expect_u1()?), + IntegerBitSize::U8 => MemoryValue::U8(!value.expect_u8()?), + IntegerBitSize::U16 => MemoryValue::U16(!value.expect_u16()?), + IntegerBitSize::U32 => MemoryValue::U32(!value.expect_u32()?), + IntegerBitSize::U64 => MemoryValue::U64(!value.expect_u64()?), + IntegerBitSize::U128 => MemoryValue::U128(!value.expect_u128()?), }; - self.memory.write(destination, MemoryValue::new_integer(negated_value, bit_size)); + self.memory.write(destination, negated_value); Ok(()) } /// Casts a value to a different bit size. fn cast(&self, target_bit_size: BitSize, source_value: MemoryValue) -> MemoryValue { + use MemoryValue::*; + match (source_value, target_bit_size) { - // Field to field, no op - (MemoryValue::Field(_), BitSize::Field) => source_value, // Field downcast to u128 - (MemoryValue::Field(field), BitSize::Integer(IntegerBitSize::U128)) => { - MemoryValue::Integer(field.to_u128(), IntegerBitSize::U128) - } + (Field(field), BitSize::Integer(IntegerBitSize::U128)) => U128(field.to_u128()), // Field downcast to arbitrary bit size - (MemoryValue::Field(field), BitSize::Integer(target_bit_size)) => { + (Field(field), BitSize::Integer(target_bit_size)) => { let as_u128 = field.to_u128(); - let target_bit_size_u32: u32 = target_bit_size.into(); - let mask = (1_u128 << target_bit_size_u32) - 1; - MemoryValue::Integer(as_u128 & mask, target_bit_size) - } - // Integer upcast to field - (MemoryValue::Integer(integer, _), BitSize::Field) => { - MemoryValue::new_field(integer.into()) - } - // Integer upcast to integer - (MemoryValue::Integer(integer, source_bit_size), BitSize::Integer(target_bit_size)) - if source_bit_size <= target_bit_size => - { - MemoryValue::Integer(integer, target_bit_size) - } - // Integer downcast - (MemoryValue::Integer(integer, _), BitSize::Integer(target_bit_size)) => { - let target_bit_size_u32: u32 = target_bit_size.into(); - let mask = (1_u128 << target_bit_size_u32) - 1; - MemoryValue::Integer(integer & mask, target_bit_size) + match target_bit_size { + IntegerBitSize::U1 => U1(as_u128 & 0x01 == 1), + IntegerBitSize::U8 => U8(as_u128 as u8), + IntegerBitSize::U16 => U16(as_u128 as u16), + IntegerBitSize::U32 => U32(as_u128 as u32), + IntegerBitSize::U64 => U64(as_u128 as u64), + IntegerBitSize::U128 => unreachable!(), + } } + + (U1(value), BitSize::Integer(IntegerBitSize::U8)) => U8(value.into()), + (U1(value), BitSize::Integer(IntegerBitSize::U16)) => U16(value.into()), + (U1(value), BitSize::Integer(IntegerBitSize::U32)) => U32(value.into()), + (U1(value), BitSize::Integer(IntegerBitSize::U64)) => U64(value.into()), + (U1(value), BitSize::Integer(IntegerBitSize::U128)) => U128(value.into()), + (U1(value), BitSize::Field) => Field(value.into()), + + (U8(value), BitSize::Integer(IntegerBitSize::U1)) => U1(value & 0x01 == 1), + (U8(value), BitSize::Integer(IntegerBitSize::U16)) => U16(value.into()), + (U8(value), BitSize::Integer(IntegerBitSize::U32)) => U32(value.into()), + (U8(value), BitSize::Integer(IntegerBitSize::U64)) => U64(value.into()), + (U8(value), BitSize::Integer(IntegerBitSize::U128)) => U128(value.into()), + (U8(value), BitSize::Field) => Field((value as u128).into()), + + (U16(value), BitSize::Integer(IntegerBitSize::U1)) => U1(value & 0x01 == 1), + (U16(value), BitSize::Integer(IntegerBitSize::U8)) => U8(value as u8), + (U16(value), BitSize::Integer(IntegerBitSize::U32)) => U32(value.into()), + (U16(value), BitSize::Integer(IntegerBitSize::U64)) => U64(value.into()), + (U16(value), BitSize::Integer(IntegerBitSize::U128)) => U128(value.into()), + (U16(value), BitSize::Field) => Field((value as u128).into()), + + (U32(value), BitSize::Integer(IntegerBitSize::U1)) => U1(value & 0x01 == 1), + (U32(value), BitSize::Integer(IntegerBitSize::U8)) => U8(value as u8), + (U32(value), BitSize::Integer(IntegerBitSize::U16)) => U16(value as u16), + (U32(value), BitSize::Integer(IntegerBitSize::U64)) => U64(value.into()), + (U32(value), BitSize::Integer(IntegerBitSize::U128)) => U128(value.into()), + (U32(value), BitSize::Field) => Field((value as u128).into()), + + (U64(value), BitSize::Integer(IntegerBitSize::U1)) => U1(value & 0x01 == 1), + (U64(value), BitSize::Integer(IntegerBitSize::U8)) => U8(value as u8), + (U64(value), BitSize::Integer(IntegerBitSize::U16)) => U16(value as u16), + (U64(value), BitSize::Integer(IntegerBitSize::U32)) => U32(value as u32), + (U64(value), BitSize::Integer(IntegerBitSize::U128)) => U128(value.into()), + (U64(value), BitSize::Field) => Field((value as u128).into()), + + (U128(value), BitSize::Integer(IntegerBitSize::U1)) => U1(value & 0x01 == 1), + (U128(value), BitSize::Integer(IntegerBitSize::U8)) => U8(value as u8), + (U128(value), BitSize::Integer(IntegerBitSize::U16)) => U16(value as u16), + (U128(value), BitSize::Integer(IntegerBitSize::U32)) => U32(value as u32), + (U128(value), BitSize::Integer(IntegerBitSize::U64)) => U64(value as u64), + (U128(value), BitSize::Field) => Field(value.into()), + + // no ops + (Field(_), BitSize::Field) => source_value, + (U1(_), BitSize::Integer(IntegerBitSize::U1)) => source_value, + (U8(_), BitSize::Integer(IntegerBitSize::U8)) => source_value, + (U16(_), BitSize::Integer(IntegerBitSize::U16)) => source_value, + (U32(_), BitSize::Integer(IntegerBitSize::U32)) => source_value, + (U64(_), BitSize::Integer(IntegerBitSize::U64)) => source_value, + (U128(_), BitSize::Integer(IntegerBitSize::U128)) => source_value, } } } @@ -1127,10 +1156,9 @@ mod tests { let VM { memory, .. } = vm; - let (negated_value, _) = memory - .read(MemoryAddress::direct(1)) - .extract_integer() - .expect("Expected integer as the output of Not"); + let MemoryValue::U128(negated_value) = memory.read(MemoryAddress::direct(1)) else { + panic!("Expected integer as the output of Not"); + }; assert_eq!(negated_value, !1_u128); } diff --git a/noir/noir-repo/acvm-repo/brillig_vm/src/memory.rs b/noir/noir-repo/acvm-repo/brillig_vm/src/memory.rs index 2bf45f77b54..7a942339a3a 100644 --- a/noir/noir-repo/acvm-repo/brillig_vm/src/memory.rs +++ b/noir/noir-repo/acvm-repo/brillig_vm/src/memory.rs @@ -2,14 +2,18 @@ use acir::{ brillig::{BitSize, IntegerBitSize, MemoryAddress}, AcirField, }; -use num_traits::{One, Zero}; pub const MEMORY_ADDRESSING_BIT_SIZE: IntegerBitSize = IntegerBitSize::U32; #[derive(Debug, Clone, Copy, PartialEq, Eq, PartialOrd, Ord)] pub enum MemoryValue { Field(F), - Integer(u128, IntegerBitSize), + U1(bool), + U8(u8), + U16(u16), + U32(u32), + U64(u64), + U128(u128), } #[derive(Debug, thiserror::Error)] @@ -26,7 +30,14 @@ impl MemoryValue { /// Builds an integer-typed memory value. pub fn new_integer(value: u128, bit_size: IntegerBitSize) -> Self { - MemoryValue::Integer(value, bit_size) + match bit_size { + IntegerBitSize::U1 => MemoryValue::U1(value != 0), + IntegerBitSize::U8 => MemoryValue::U8(value as u8), + IntegerBitSize::U16 => MemoryValue::U16(value as u16), + IntegerBitSize::U32 => MemoryValue::U32(value as u32), + IntegerBitSize::U64 => MemoryValue::U64(value as u64), + IntegerBitSize::U128 => MemoryValue::U128(value), + } } /// Extracts the field element from the memory value, if it is typed as field element. @@ -37,26 +48,21 @@ impl MemoryValue { } } - /// Extracts the integer from the memory value, if it is typed as integer. - pub fn extract_integer(&self) -> Option<(u128, IntegerBitSize)> { - match self { - MemoryValue::Integer(value, bit_size) => Some((*value, *bit_size)), - _ => None, - } - } - pub fn bit_size(&self) -> BitSize { match self { MemoryValue::Field(_) => BitSize::Field, - MemoryValue::Integer(_, bit_size) => BitSize::Integer(*bit_size), + MemoryValue::U1(_) => BitSize::Integer(IntegerBitSize::U1), + MemoryValue::U8(_) => BitSize::Integer(IntegerBitSize::U8), + MemoryValue::U16(_) => BitSize::Integer(IntegerBitSize::U16), + MemoryValue::U32(_) => BitSize::Integer(IntegerBitSize::U32), + MemoryValue::U64(_) => BitSize::Integer(IntegerBitSize::U64), + MemoryValue::U128(_) => BitSize::Integer(IntegerBitSize::U128), } } pub fn to_usize(&self) -> usize { match self { - MemoryValue::Integer(_, bit_size) if *bit_size == MEMORY_ADDRESSING_BIT_SIZE => { - self.extract_integer().unwrap().0.try_into().unwrap() - } + MemoryValue::U32(value) => (*value).try_into().unwrap(), _ => panic!("value is not typed as brillig usize"), } } @@ -87,38 +93,89 @@ impl MemoryValue { pub fn to_field(&self) -> F { match self { MemoryValue::Field(value) => *value, - MemoryValue::Integer(value, _) => F::from(*value), + MemoryValue::U1(value) => F::from(*value), + MemoryValue::U8(value) => F::from(*value as u128), + MemoryValue::U16(value) => F::from(*value as u128), + MemoryValue::U32(value) => F::from(*value as u128), + MemoryValue::U64(value) => F::from(*value as u128), + MemoryValue::U128(value) => F::from(*value), } } pub fn expect_field(&self) -> Result<&F, MemoryTypeError> { - match self { - MemoryValue::Integer(_, bit_size) => Err(MemoryTypeError::MismatchedBitSize { - value_bit_size: (*bit_size).into(), + if let MemoryValue::Field(field) = self { + Ok(field) + } else { + Err(MemoryTypeError::MismatchedBitSize { + value_bit_size: self.bit_size().to_u32::(), expected_bit_size: F::max_num_bits(), - }), - MemoryValue::Field(field) => Ok(field), + }) } } - pub fn expect_integer_with_bit_size( - &self, - expected_bit_size: IntegerBitSize, - ) -> Result { - match self { - MemoryValue::Integer(value, bit_size) => { - if *bit_size != expected_bit_size { - return Err(MemoryTypeError::MismatchedBitSize { - value_bit_size: (*bit_size).into(), - expected_bit_size: expected_bit_size.into(), - }); - } - Ok(*value) - } - MemoryValue::Field(_) => Err(MemoryTypeError::MismatchedBitSize { - value_bit_size: F::max_num_bits(), - expected_bit_size: expected_bit_size.into(), - }), + pub(crate) fn expect_u1(&self) -> Result { + if let MemoryValue::U1(value) = self { + Ok(*value) + } else { + Err(MemoryTypeError::MismatchedBitSize { + value_bit_size: self.bit_size().to_u32::(), + expected_bit_size: 1, + }) + } + } + + pub(crate) fn expect_u8(&self) -> Result { + if let MemoryValue::U8(value) = self { + Ok(*value) + } else { + Err(MemoryTypeError::MismatchedBitSize { + value_bit_size: self.bit_size().to_u32::(), + expected_bit_size: 8, + }) + } + } + + pub(crate) fn expect_u16(&self) -> Result { + if let MemoryValue::U16(value) = self { + Ok(*value) + } else { + Err(MemoryTypeError::MismatchedBitSize { + value_bit_size: self.bit_size().to_u32::(), + expected_bit_size: 16, + }) + } + } + + pub(crate) fn expect_u32(&self) -> Result { + if let MemoryValue::U32(value) = self { + Ok(*value) + } else { + Err(MemoryTypeError::MismatchedBitSize { + value_bit_size: self.bit_size().to_u32::(), + expected_bit_size: 32, + }) + } + } + + pub(crate) fn expect_u64(&self) -> Result { + if let MemoryValue::U64(value) = self { + Ok(*value) + } else { + Err(MemoryTypeError::MismatchedBitSize { + value_bit_size: self.bit_size().to_u32::(), + expected_bit_size: 64, + }) + } + } + + pub(crate) fn expect_u128(&self) -> Result { + if let MemoryValue::U128(value) = self { + Ok(*value) + } else { + Err(MemoryTypeError::MismatchedBitSize { + value_bit_size: self.bit_size().to_u32::(), + expected_bit_size: 128, + }) } } } @@ -127,9 +184,12 @@ impl std::fmt::Display for MemoryValue { fn fmt(&self, f: &mut ::std::fmt::Formatter) -> Result<(), ::std::fmt::Error> { match self { MemoryValue::Field(value) => write!(f, "{}: field", value), - MemoryValue::Integer(value, bit_size) => { - write!(f, "{}: {}", value, bit_size) - } + MemoryValue::U1(value) => write!(f, "{}: u1", value), + MemoryValue::U8(value) => write!(f, "{}: u8", value), + MemoryValue::U16(value) => write!(f, "{}: u16", value), + MemoryValue::U32(value) => write!(f, "{}: u32", value), + MemoryValue::U64(value) => write!(f, "{}: u64", value), + MemoryValue::U128(value) => write!(f, "{}: u128", value), } } } @@ -142,38 +202,37 @@ impl Default for MemoryValue { impl From for MemoryValue { fn from(value: bool) -> Self { - let value = if value { 1 } else { 0 }; - MemoryValue::new_integer(value, IntegerBitSize::U1) + MemoryValue::U1(value) } } impl From for MemoryValue { fn from(value: u8) -> Self { - MemoryValue::new_integer(value.into(), IntegerBitSize::U8) + MemoryValue::U8(value) } } impl From for MemoryValue { fn from(value: usize) -> Self { - MemoryValue::new_integer(value as u128, MEMORY_ADDRESSING_BIT_SIZE) + MemoryValue::U32(value as u32) } } impl From for MemoryValue { fn from(value: u32) -> Self { - MemoryValue::new_integer(value.into(), IntegerBitSize::U32) + MemoryValue::U32(value) } } impl From for MemoryValue { fn from(value: u64) -> Self { - MemoryValue::new_integer(value.into(), IntegerBitSize::U64) + MemoryValue::U64(value) } } impl From for MemoryValue { fn from(value: u128) -> Self { - MemoryValue::new_integer(value, IntegerBitSize::U128) + MemoryValue::U128(value) } } @@ -181,15 +240,7 @@ impl TryFrom> for bool { type Error = MemoryTypeError; fn try_from(memory_value: MemoryValue) -> Result { - let as_integer = memory_value.expect_integer_with_bit_size(IntegerBitSize::U1)?; - - if as_integer.is_zero() { - Ok(false) - } else if as_integer.is_one() { - Ok(true) - } else { - unreachable!("value typed as bool is greater than one") - } + memory_value.expect_u1() } } @@ -197,7 +248,7 @@ impl TryFrom> for u8 { type Error = MemoryTypeError; fn try_from(memory_value: MemoryValue) -> Result { - memory_value.expect_integer_with_bit_size(IntegerBitSize::U8).map(|value| value as u8) + memory_value.expect_u8() } } @@ -205,7 +256,7 @@ impl TryFrom> for u32 { type Error = MemoryTypeError; fn try_from(memory_value: MemoryValue) -> Result { - memory_value.expect_integer_with_bit_size(IntegerBitSize::U32).map(|value| value as u32) + memory_value.expect_u32() } } @@ -213,7 +264,7 @@ impl TryFrom> for u64 { type Error = MemoryTypeError; fn try_from(memory_value: MemoryValue) -> Result { - memory_value.expect_integer_with_bit_size(IntegerBitSize::U64).map(|value| value as u64) + memory_value.expect_u64() } } @@ -221,7 +272,7 @@ impl TryFrom> for u128 { type Error = MemoryTypeError; fn try_from(memory_value: MemoryValue) -> Result { - memory_value.expect_integer_with_bit_size(IntegerBitSize::U128) + memory_value.expect_u128() } } diff --git a/noir/noir-repo/compiler/noirc_driver/src/lib.rs b/noir/noir-repo/compiler/noirc_driver/src/lib.rs index 09102d530ad..807ee7385a3 100644 --- a/noir/noir-repo/compiler/noirc_driver/src/lib.rs +++ b/noir/noir-repo/compiler/noirc_driver/src/lib.rs @@ -11,6 +11,7 @@ use fm::{FileId, FileManager}; use iter_extended::vecmap; use noirc_abi::{AbiParameter, AbiType, AbiValue}; use noirc_errors::{CustomDiagnostic, DiagnosticKind, FileDiagnostic}; +use noirc_evaluator::brillig::BrilligOptions; use noirc_evaluator::create_program; use noirc_evaluator::errors::RuntimeError; use noirc_evaluator::ssa::{SsaLogging, SsaProgramArtifact}; @@ -141,6 +142,10 @@ pub struct CompileOptions { #[arg(long)] pub enable_brillig_constraints_check: bool, + /// Flag to turn on extra Brillig bytecode to be generated to guard against invalid states in testing. + #[arg(long, hide = true)] + pub enable_brillig_debug_assertions: bool, + /// Hidden Brillig call check flag to maintain CI compatibility (currently ignored) #[arg(long, hide = true)] pub skip_brillig_constraints_check: bool, @@ -680,7 +685,10 @@ pub fn compile_no_check( } } }, - enable_brillig_logging: options.show_brillig, + brillig_options: BrilligOptions { + enable_debug_trace: options.show_brillig, + enable_debug_assertions: options.enable_brillig_debug_assertions, + }, print_codegen_timings: options.benchmark_codegen, expression_width: if options.bounded_codegen { options.expression_width.unwrap_or(DEFAULT_EXPRESSION_WIDTH) diff --git a/noir/noir-repo/compiler/noirc_evaluator/src/acir/mod.rs b/noir/noir-repo/compiler/noirc_evaluator/src/acir/mod.rs index d883bfc9354..46c08242ce6 100644 --- a/noir/noir-repo/compiler/noirc_evaluator/src/acir/mod.rs +++ b/noir/noir-repo/compiler/noirc_evaluator/src/acir/mod.rs @@ -26,6 +26,7 @@ mod brillig_directive; mod generated_acir; use crate::brillig::brillig_gen::gen_brillig_for; +use crate::brillig::BrilligOptions; use crate::brillig::{ brillig_gen::brillig_fn::FunctionContext as BrilligFunctionContext, brillig_ir::artifact::{BrilligParameter, GeneratedBrillig}, @@ -209,6 +210,11 @@ struct Context<'a> { /// Contains state that is generated and also used across ACIR functions shared_context: &'a mut SharedContext, + + brillig: &'a Brillig, + + /// Options affecting Brillig code generation. + brillig_options: &'a BrilligOptions, } #[derive(Clone)] @@ -305,6 +311,7 @@ impl Ssa { pub(crate) fn into_acir( self, brillig: &Brillig, + brillig_options: &BrilligOptions, expression_width: ExpressionWidth, ) -> Result { let mut acirs = Vec::new(); @@ -312,10 +319,10 @@ impl Ssa { let mut shared_context = SharedContext::default(); for function in self.functions.values() { - let context = Context::new(&mut shared_context, expression_width); - if let Some(mut generated_acir) = - context.convert_ssa_function(&self, function, brillig)? - { + let context = + Context::new(&mut shared_context, expression_width, brillig, brillig_options); + + if let Some(mut generated_acir) = context.convert_ssa_function(&self, function)? { // We want to be able to insert Brillig stdlib functions anywhere during the ACIR generation process (e.g. such as on the `GeneratedAcir`). // As we don't want a reference to the `SharedContext` on the generated ACIR itself, // we instead store the opcode location at which a Brillig call to a std lib function occurred. @@ -364,6 +371,8 @@ impl<'a> Context<'a> { fn new( shared_context: &'a mut SharedContext, expression_width: ExpressionWidth, + brillig: &'a Brillig, + brillig_options: &'a BrilligOptions, ) -> Context<'a> { let mut acir_context = AcirContext::default(); acir_context.set_expression_width(expression_width); @@ -380,6 +389,8 @@ impl<'a> Context<'a> { max_block_id: 0, data_bus: DataBus::default(), shared_context, + brillig, + brillig_options, } } @@ -387,7 +398,6 @@ impl<'a> Context<'a> { self, ssa: &Ssa, function: &Function, - brillig: &Brillig, ) -> Result>, RuntimeError> { match function.runtime() { RuntimeType::Acir(inline_type) => { @@ -403,11 +413,11 @@ impl<'a> Context<'a> { InlineType::Fold => {} } // We only want to convert entry point functions. This being `main` and those marked with `InlineType::Fold` - Ok(Some(self.convert_acir_main(function, ssa, brillig)?)) + Ok(Some(self.convert_acir_main(function, ssa)?)) } RuntimeType::Brillig(_) => { if function.id() == ssa.main_id { - Ok(Some(self.convert_brillig_main(function, brillig)?)) + Ok(Some(self.convert_brillig_main(function)?)) } else { Ok(None) } @@ -419,7 +429,6 @@ impl<'a> Context<'a> { mut self, main_func: &Function, ssa: &Ssa, - brillig: &Brillig, ) -> Result, RuntimeError> { let dfg = &main_func.dfg; let entry_block = &dfg[main_func.entry_block()]; @@ -447,7 +456,7 @@ impl<'a> Context<'a> { self.data_bus = dfg.data_bus.to_owned(); let mut warnings = Vec::new(); for instruction_id in entry_block.instructions() { - warnings.extend(self.convert_ssa_instruction(*instruction_id, dfg, ssa, brillig)?); + warnings.extend(self.convert_ssa_instruction(*instruction_id, dfg, ssa)?); } let (return_vars, return_warnings) = self.convert_ssa_return(entry_block.unwrap_terminator(), dfg)?; @@ -504,7 +513,6 @@ impl<'a> Context<'a> { fn convert_brillig_main( mut self, main_func: &Function, - brillig: &Brillig, ) -> Result, RuntimeError> { let dfg = &main_func.dfg; @@ -519,7 +527,8 @@ impl<'a> Context<'a> { let outputs: Vec = vecmap(main_func.returns(), |result_id| dfg.type_of_value(*result_id).into()); - let code = gen_brillig_for(main_func, arguments.clone(), brillig)?; + let code = + gen_brillig_for(main_func, arguments.clone(), self.brillig, self.brillig_options)?; // 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. @@ -674,7 +683,6 @@ impl<'a> Context<'a> { instruction_id: InstructionId, dfg: &DataFlowGraph, ssa: &Ssa, - brillig: &Brillig, ) -> Result, RuntimeError> { let instruction = &dfg[instruction_id]; self.acir_context.set_call_stack(dfg.get_instruction_call_stack(instruction_id)); @@ -777,13 +785,7 @@ impl<'a> Context<'a> { } Instruction::Call { .. } => { let result_ids = dfg.instruction_results(instruction_id); - warnings.extend(self.convert_ssa_call( - instruction, - dfg, - ssa, - brillig, - result_ids, - )?); + warnings.extend(self.convert_ssa_call(instruction, dfg, ssa, result_ids)?); } Instruction::Not(value_id) => { let (acir_var, typ) = match self.convert_value(*value_id, dfg) { @@ -849,7 +851,6 @@ impl<'a> Context<'a> { instruction: &Instruction, dfg: &DataFlowGraph, ssa: &Ssa, - brillig: &Brillig, result_ids: &[ValueId], ) -> Result, RuntimeError> { let mut warnings = Vec::new(); @@ -927,7 +928,12 @@ impl<'a> Context<'a> { None, )? } else { - let code = gen_brillig_for(func, arguments.clone(), brillig)?; + let code = gen_brillig_for( + func, + arguments.clone(), + self.brillig, + self.brillig_options, + )?; let generated_pointer = self.shared_context.new_generated_pointer(); let output_values = self.acir_context.brillig_call( @@ -2904,7 +2910,7 @@ mod test { use crate::{ acir::BrilligStdlibFunc, - brillig::Brillig, + brillig::{Brillig, BrilligOptions}, ssa::{ function_builder::FunctionBuilder, ir::{ @@ -3005,7 +3011,7 @@ mod test { let ssa = builder.finish().generate_entry_point_index(); let (acir_functions, _, _, _) = ssa - .into_acir(&Brillig::default(), ExpressionWidth::default()) + .into_acir(&Brillig::default(), &BrilligOptions::default(), ExpressionWidth::default()) .expect("Should compile manually written SSA into ACIR"); // Expected result: // main f0 @@ -3111,7 +3117,7 @@ mod test { let (acir_functions, _, _, _) = ssa .generate_entry_point_index() - .into_acir(&Brillig::default(), ExpressionWidth::default()) + .into_acir(&Brillig::default(), &BrilligOptions::default(), ExpressionWidth::default()) .expect("Should compile manually written SSA into ACIR"); // The expected result should look very similar to the above test expect that the input witnesses of the `Call` // opcodes will be different. The changes can discerned from the checks below. @@ -3215,7 +3221,7 @@ mod test { let ssa = builder.finish().generate_entry_point_index(); let (acir_functions, _, _, _) = ssa - .into_acir(&Brillig::default(), ExpressionWidth::default()) + .into_acir(&Brillig::default(), &BrilligOptions::default(), ExpressionWidth::default()) .expect("Should compile manually written SSA into ACIR"); assert_eq!(acir_functions.len(), 3, "Should have three ACIR functions"); @@ -3336,11 +3342,11 @@ mod test { build_basic_foo_with_return(&mut builder, bar_id, true, InlineType::default()); let ssa = builder.finish(); - let brillig = ssa.to_brillig(false); + let brillig = ssa.to_brillig(&BrilligOptions::default()); let (acir_functions, brillig_functions, _, _) = ssa .generate_entry_point_index() - .into_acir(&brillig, ExpressionWidth::default()) + .into_acir(&brillig, &BrilligOptions::default(), ExpressionWidth::default()) .expect("Should compile manually written SSA into ACIR"); assert_eq!(acir_functions.len(), 1, "Should only have a `main` ACIR function"); @@ -3405,7 +3411,7 @@ mod test { // Brillig artifacts to the ACIR gen pass. let (acir_functions, brillig_functions, _, _) = ssa .generate_entry_point_index() - .into_acir(&Brillig::default(), ExpressionWidth::default()) + .into_acir(&Brillig::default(), &BrilligOptions::default(), ExpressionWidth::default()) .expect("Should compile manually written SSA into ACIR"); assert_eq!(acir_functions.len(), 1, "Should only have a `main` ACIR function"); @@ -3475,12 +3481,12 @@ mod test { let ssa = builder.finish(); // We need to generate Brillig artifacts for the regular Brillig function and pass them to the ACIR generation pass. - let brillig = ssa.to_brillig(false); + let brillig = ssa.to_brillig(&BrilligOptions::default()); println!("{}", ssa); let (acir_functions, brillig_functions, _, _) = ssa .generate_entry_point_index() - .into_acir(&brillig, ExpressionWidth::default()) + .into_acir(&brillig, &BrilligOptions::default(), ExpressionWidth::default()) .expect("Should compile manually written SSA into ACIR"); assert_eq!(acir_functions.len(), 1, "Should only have a `main` ACIR function"); @@ -3564,12 +3570,12 @@ mod test { let ssa = builder.finish(); // We need to generate Brillig artifacts for the regular Brillig function and pass them to the ACIR generation pass. - let brillig = ssa.to_brillig(false); + let brillig = ssa.to_brillig(&BrilligOptions::default()); println!("{}", ssa); let (acir_functions, brillig_functions, _, _) = ssa .generate_entry_point_index() - .into_acir(&brillig, ExpressionWidth::default()) + .into_acir(&brillig, &BrilligOptions::default(), ExpressionWidth::default()) .expect("Should compile manually written SSA into ACIR"); assert_eq!(acir_functions.len(), 2, "Should only have two ACIR functions"); @@ -3681,10 +3687,10 @@ mod test { } "; let ssa = Ssa::from_str(src).unwrap(); - let brillig = ssa.to_brillig(false); + let brillig = ssa.to_brillig(&BrilligOptions::default()); let (mut acir_functions, _brillig_functions, _, _) = ssa - .into_acir(&brillig, ExpressionWidth::default()) + .into_acir(&brillig, &BrilligOptions::default(), ExpressionWidth::default()) .expect("Should compile manually written SSA into ACIR"); assert_eq!(acir_functions.len(), 1); @@ -3712,17 +3718,17 @@ mod test { constrain v7 == Field 0 return } - + brillig(inline) fn foo f1 { b0(v0: u32, v1: [Field]): return } "; let ssa = Ssa::from_str(src).unwrap(); - let brillig = ssa.to_brillig(false); + let brillig = ssa.to_brillig(&BrilligOptions::default()); let (acir_functions, _brillig_functions, _, _) = ssa - .into_acir(&brillig, ExpressionWidth::default()) + .into_acir(&brillig, &BrilligOptions::default(), ExpressionWidth::default()) .expect("Should compile manually written SSA into ACIR"); assert_eq!(acir_functions.len(), 1); diff --git a/noir/noir-repo/compiler/noirc_evaluator/src/brillig/brillig_gen.rs b/noir/noir-repo/compiler/noirc_evaluator/src/brillig/brillig_gen.rs index 1594bac2acc..a2cfb6be5ad 100644 --- a/noir/noir-repo/compiler/noirc_evaluator/src/brillig/brillig_gen.rs +++ b/noir/noir-repo/compiler/noirc_evaluator/src/brillig/brillig_gen.rs @@ -16,7 +16,7 @@ use super::{ artifact::{BrilligArtifact, BrilligParameter, GeneratedBrillig, Label}, BrilligContext, }, - Brillig, BrilligVariable, ValueId, + Brillig, BrilligOptions, BrilligVariable, ValueId, }; use crate::{ errors::InternalError, @@ -26,10 +26,10 @@ use crate::{ /// Converting an SSA function into Brillig bytecode. pub(crate) fn convert_ssa_function( func: &Function, - enable_debug_trace: bool, + options: &BrilligOptions, globals: &HashMap, ) -> BrilligArtifact { - let mut brillig_context = BrilligContext::new(enable_debug_trace); + let mut brillig_context = BrilligContext::new(options); let mut function_context = FunctionContext::new(func); @@ -56,6 +56,7 @@ pub(crate) fn gen_brillig_for( func: &Function, arguments: Vec, brillig: &Brillig, + options: &BrilligOptions, ) -> Result, InternalError> { // Create the entry point artifact let globals_memory_size = brillig @@ -63,18 +64,22 @@ pub(crate) fn gen_brillig_for( .get(&func.id()) .copied() .expect("Should have the globals memory size specified for an entry point"); + + let options = BrilligOptions { enable_debug_trace: false, ..*options }; + let mut entry_point = BrilligContext::new_entry_point_artifact( arguments, FunctionContext::return_values(func), func.id(), true, globals_memory_size, + &options, ); entry_point.name = func.name().to_string(); // Link the entry point with all dependencies while let Some(unresolved_fn_label) = entry_point.first_unresolved_function_call() { - let artifact = &brillig.find_by_label(unresolved_fn_label.clone()); + let artifact = &brillig.find_by_label(unresolved_fn_label.clone(), &options); let artifact = match artifact { Some(artifact) => artifact, None => { diff --git a/noir/noir-repo/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_block.rs b/noir/noir-repo/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_block.rs index 97de1aea8c7..fea7f57d3f8 100644 --- a/noir/noir-repo/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_block.rs +++ b/noir/noir-repo/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_block.rs @@ -20,6 +20,7 @@ use crate::ssa::ir::{ value::{Value, ValueId}, }; use acvm::acir::brillig::{MemoryAddress, ValueOrArray}; +use acvm::brillig_vm::MEMORY_ADDRESSING_BIT_SIZE; use acvm::{acir::AcirField, FieldElement}; use fxhash::{FxHashMap as HashMap, FxHashSet as HashSet}; use iter_extended::vecmap; @@ -835,20 +836,65 @@ impl<'block, Registers: RegisterAllocator> BrilligBlock<'block, Registers> { .store_instruction(array_or_vector.extract_register(), rc_register); self.brillig_context.deallocate_register(rc_register); } - Instruction::DecrementRc { value } => { + Instruction::DecrementRc { value, original } => { let array_or_vector = self.convert_ssa_value(*value, dfg); - let rc_register = self.brillig_context.allocate_register(); + let orig_array_or_vector = self.convert_ssa_value(*original, dfg); - self.brillig_context - .load_instruction(rc_register, array_or_vector.extract_register()); - self.brillig_context.codegen_usize_op_in_place( - rc_register, - BrilligBinaryOp::Sub, - 1, - ); - self.brillig_context - .store_instruction(array_or_vector.extract_register(), rc_register); - self.brillig_context.deallocate_register(rc_register); + let array_register = array_or_vector.extract_register(); + let orig_array_register = orig_array_or_vector.extract_register(); + + let codegen_dec_rc = |ctx: &mut BrilligContext| { + let rc_register = ctx.allocate_register(); + + ctx.load_instruction(rc_register, array_register); + + ctx.codegen_usize_op_in_place(rc_register, BrilligBinaryOp::Sub, 1); + + // Check that the refcount didn't go below 1. If we allow it to underflow + // and become 0 or usize::MAX, and then return to 1, then it will indicate + // an array as mutable when it probably shouldn't be. + if ctx.enable_debug_assertions() { + let min_addr = ReservedRegisters::usize_one(); + let condition = SingleAddrVariable::new(ctx.allocate_register(), 1); + ctx.memory_op_instruction( + min_addr, + rc_register, + condition.address, + BrilligBinaryOp::LessThanEquals, + ); + ctx.codegen_constrain( + condition, + Some("array ref-count went to zero".to_owned()), + ); + ctx.deallocate_single_addr(condition); + } + + ctx.store_instruction(array_register, rc_register); + ctx.deallocate_register(rc_register); + }; + + if array_register == orig_array_register { + codegen_dec_rc(self.brillig_context); + } else { + // Check if the current and original register point at the same memory. + // If they don't, it means that an array-copy procedure has created a + // new array, which includes decrementing the RC of the original, + // which means we don't have to do the decrement here. + let condition = + SingleAddrVariable::new(self.brillig_context.allocate_register(), 1); + // We will use a binary op to interpret the pointer as a number. + let bit_size: u32 = MEMORY_ADDRESSING_BIT_SIZE.into(); + let array_var = SingleAddrVariable::new(array_register, bit_size); + let orig_array_var = SingleAddrVariable::new(orig_array_register, bit_size); + self.brillig_context.binary_instruction( + array_var, + orig_array_var, + condition, + BrilligBinaryOp::Equals, + ); + self.brillig_context.codegen_if(condition.address, codegen_dec_rc); + self.brillig_context.deallocate_single_addr(condition); + } } Instruction::EnableSideEffectsIf { .. } => { unreachable!("enable_side_effects not supported by brillig") diff --git a/noir/noir-repo/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_globals.rs b/noir/noir-repo/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_globals.rs index 5e7f250a6b0..04c43df9d7e 100644 --- a/noir/noir-repo/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_globals.rs +++ b/noir/noir-repo/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_globals.rs @@ -7,8 +7,8 @@ use super::{ BrilligArtifact, BrilligBlock, BrilligVariable, Function, FunctionContext, Label, ValueId, }; use crate::brillig::{ - brillig_ir::BrilligContext, called_functions_vec, Brillig, DataFlowGraph, FunctionId, - Instruction, Value, + brillig_ir::BrilligContext, called_functions_vec, Brillig, BrilligOptions, DataFlowGraph, + FunctionId, Instruction, Value, }; /// Context structure for generating Brillig globals @@ -149,7 +149,7 @@ impl BrilligGlobals { &mut self, globals_dfg: &DataFlowGraph, brillig: &mut Brillig, - enable_debug_trace: bool, + options: &BrilligOptions, ) { // Map for fetching the correct entry point globals when compiling any function let mut inner_call_to_entry_point: HashMap> = @@ -165,7 +165,7 @@ impl BrilligGlobals { let used_globals = self.used_globals.remove(&entry_point).unwrap_or_default(); let (artifact, brillig_globals, globals_size) = - convert_ssa_globals(enable_debug_trace, globals_dfg, &used_globals, entry_point); + convert_ssa_globals(options, globals_dfg, &used_globals, entry_point); entry_point_globals_map.insert(entry_point, brillig_globals); @@ -217,12 +217,12 @@ impl BrilligGlobals { } pub(crate) fn convert_ssa_globals( - enable_debug_trace: bool, + options: &BrilligOptions, globals_dfg: &DataFlowGraph, used_globals: &HashSet, entry_point: FunctionId, ) -> (BrilligArtifact, HashMap, usize) { - let mut brillig_context = BrilligContext::new_for_global_init(enable_debug_trace, entry_point); + let mut brillig_context = BrilligContext::new_for_global_init(options, entry_point); // The global space does not have globals itself let empty_globals = HashMap::default(); // We can use any ID here as this context is only going to be used for globals which does not differentiate @@ -258,7 +258,9 @@ mod tests { FieldElement, }; - use crate::brillig::{brillig_ir::registers::RegisterAllocator, GlobalSpace, LabelType, Ssa}; + use crate::brillig::{ + brillig_ir::registers::RegisterAllocator, BrilligOptions, GlobalSpace, LabelType, Ssa, + }; #[test] fn entry_points_different_globals() { @@ -291,7 +293,7 @@ mod tests { let mut ssa = ssa.dead_instruction_elimination(); let used_globals_map = std::mem::take(&mut ssa.used_globals); - let brillig = ssa.to_brillig_with_globals(false, used_globals_map); + let brillig = ssa.to_brillig_with_globals(&BrilligOptions::default(), used_globals_map); assert_eq!( brillig.globals.len(), @@ -409,7 +411,7 @@ mod tests { let mut ssa = ssa.dead_instruction_elimination(); let used_globals_map = std::mem::take(&mut ssa.used_globals); - let brillig = ssa.to_brillig_with_globals(false, used_globals_map); + let brillig = ssa.to_brillig_with_globals(&BrilligOptions::default(), used_globals_map); assert_eq!( brillig.globals.len(), diff --git a/noir/noir-repo/compiler/noirc_evaluator/src/brillig/brillig_ir.rs b/noir/noir-repo/compiler/noirc_evaluator/src/brillig/brillig_ir.rs index d25f7ddef85..95e6c5175b2 100644 --- a/noir/noir-repo/compiler/noirc_evaluator/src/brillig/brillig_ir.rs +++ b/noir/noir-repo/compiler/noirc_evaluator/src/brillig/brillig_ir.rs @@ -37,7 +37,7 @@ use acvm::{ }; use debug_show::DebugShow; -use super::{FunctionId, GlobalSpace, ProcedureId}; +use super::{BrilligOptions, FunctionId, GlobalSpace, ProcedureId}; /// The Brillig VM does not apply a limit to the memory address space, /// As a convention, we take use 32 bits. This means that we assume that @@ -95,20 +95,30 @@ pub(crate) struct BrilligContext { /// Whether this context can call procedures or not. /// This is used to prevent a procedure from calling another procedure. can_call_procedures: bool, + /// Insert extra assertions that we expect to be true, at the cost of larger bytecode size. + enable_debug_assertions: bool, globals_memory_size: Option, } +impl BrilligContext { + /// Enable the insertion of bytecode with extra assertions during testing. + pub(crate) fn enable_debug_assertions(&self) -> bool { + self.enable_debug_assertions + } +} + /// Regular brillig context to codegen user defined functions impl BrilligContext { - pub(crate) fn new(enable_debug_trace: bool) -> BrilligContext { + pub(crate) fn new(options: &BrilligOptions) -> BrilligContext { BrilligContext { obj: BrilligArtifact::default(), registers: Stack::new(), context_label: Label::entrypoint(), current_section: 0, next_section: 1, - debug_show: DebugShow::new(enable_debug_trace), + debug_show: DebugShow::new(options.enable_debug_trace), + enable_debug_assertions: options.enable_debug_assertions, can_call_procedures: true, globals_memory_size: None, } @@ -201,8 +211,8 @@ impl BrilligContext< /// Special brillig context to codegen compiler intrinsic shared procedures impl BrilligContext { pub(crate) fn new_for_procedure( - enable_debug_trace: bool, procedure_id: ProcedureId, + options: &BrilligOptions, ) -> BrilligContext { let mut obj = BrilligArtifact::default(); obj.procedure = Some(procedure_id); @@ -212,7 +222,8 @@ impl BrilligContext { context_label: Label::entrypoint(), current_section: 0, next_section: 1, - debug_show: DebugShow::new(enable_debug_trace), + debug_show: DebugShow::new(options.enable_debug_trace), + enable_debug_assertions: options.enable_debug_assertions, can_call_procedures: false, globals_memory_size: None, } @@ -222,7 +233,7 @@ impl BrilligContext { /// Special brillig context to codegen global values initialization impl BrilligContext { pub(crate) fn new_for_global_init( - enable_debug_trace: bool, + options: &BrilligOptions, entry_point: FunctionId, ) -> BrilligContext { BrilligContext { @@ -231,7 +242,8 @@ impl BrilligContext { context_label: Label::globals_init(entry_point), current_section: 0, next_section: 1, - debug_show: DebugShow::new(enable_debug_trace), + debug_show: DebugShow::new(options.enable_debug_trace), + enable_debug_assertions: options.enable_debug_assertions, can_call_procedures: false, globals_memory_size: None, } @@ -273,6 +285,7 @@ pub(crate) mod tests { use acvm::{BlackBoxFunctionSolver, BlackBoxResolutionError, FieldElement}; use crate::brillig::brillig_ir::{BrilligBinaryOp, BrilligContext}; + use crate::brillig::BrilligOptions; use crate::ssa::ir::function::FunctionId; use super::artifact::{BrilligParameter, GeneratedBrillig, Label, LabelType}; @@ -318,7 +331,8 @@ pub(crate) mod tests { } pub(crate) fn create_context(id: FunctionId) -> BrilligContext { - let mut context = BrilligContext::new(true); + let options = BrilligOptions { enable_debug_trace: true, enable_debug_assertions: true }; + let mut context = BrilligContext::new(&options); context.enter_context(Label::function(id)); context } @@ -328,6 +342,10 @@ pub(crate) mod tests { arguments: Vec, returns: Vec, ) -> GeneratedBrillig { + let options = BrilligOptions { + enable_debug_trace: false, + enable_debug_assertions: context.enable_debug_assertions, + }; let artifact = context.artifact(); let mut entry_point_artifact = BrilligContext::new_entry_point_artifact( arguments, @@ -335,6 +353,7 @@ pub(crate) mod tests { FunctionId::test_new(0), false, 0, + &options, ); entry_point_artifact.link_with(&artifact); while let Some(unresolved_fn_label) = entry_point_artifact.first_unresolved_function_call() @@ -342,7 +361,7 @@ pub(crate) mod tests { let LabelType::Procedure(procedure_id) = unresolved_fn_label.label_type else { panic!("Test functions cannot be linked with other functions"); }; - let procedure_artifact = compile_procedure(procedure_id); + let procedure_artifact = compile_procedure(procedure_id, &options); entry_point_artifact.link_with(&procedure_artifact); } entry_point_artifact.finish() @@ -376,7 +395,8 @@ pub(crate) mod tests { // let the_sequence = get_number_sequence(12); // assert(the_sequence.len() == 12); // } - let mut context = BrilligContext::new(true); + let options = BrilligOptions { enable_debug_trace: true, enable_debug_assertions: true }; + let mut context = BrilligContext::new(&options); let r_stack = ReservedRegisters::free_memory_pointer(); // Start stack pointer at 0 context.usize_const_instruction(r_stack, FieldElement::from(ReservedRegisters::len() + 3)); diff --git a/noir/noir-repo/compiler/noirc_evaluator/src/brillig/brillig_ir/codegen_stack.rs b/noir/noir-repo/compiler/noirc_evaluator/src/brillig/brillig_ir/codegen_stack.rs index 599c05fc0e8..f609c342288 100644 --- a/noir/noir-repo/compiler/noirc_evaluator/src/brillig/brillig_ir/codegen_stack.rs +++ b/noir/noir-repo/compiler/noirc_evaluator/src/brillig/brillig_ir/codegen_stack.rs @@ -132,7 +132,10 @@ mod tests { use fxhash::{FxHashMap as HashMap, FxHashSet as HashSet}; use crate::{ - brillig::brillig_ir::{artifact::Label, registers::Stack, BrilligContext}, + brillig::{ + brillig_ir::{artifact::Label, registers::Stack, BrilligContext}, + BrilligOptions, + }, ssa::ir::function::FunctionId, }; @@ -204,7 +207,8 @@ mod tests { } pub(crate) fn create_context() -> BrilligContext { - let mut context = BrilligContext::new(true); + let options = BrilligOptions { enable_debug_trace: true, enable_debug_assertions: true }; + let mut context = BrilligContext::new(&options); context.enter_context(Label::function(FunctionId::test_new(0))); context } diff --git a/noir/noir-repo/compiler/noirc_evaluator/src/brillig/brillig_ir/entry_point.rs b/noir/noir-repo/compiler/noirc_evaluator/src/brillig/brillig_ir/entry_point.rs index 6d4cc814d3e..4a5be8bb19c 100644 --- a/noir/noir-repo/compiler/noirc_evaluator/src/brillig/brillig_ir/entry_point.rs +++ b/noir/noir-repo/compiler/noirc_evaluator/src/brillig/brillig_ir/entry_point.rs @@ -1,4 +1,4 @@ -use crate::ssa::ir::function::FunctionId; +use crate::{brillig::BrilligOptions, ssa::ir::function::FunctionId}; use super::{ artifact::{BrilligArtifact, BrilligParameter}, @@ -24,8 +24,9 @@ impl BrilligContext { target_function: FunctionId, globals_init: bool, globals_memory_size: usize, + options: &BrilligOptions, ) -> BrilligArtifact { - let mut context = BrilligContext::new(false); + let mut context = BrilligContext::new(options); context.globals_memory_size = Some(globals_memory_size); diff --git a/noir/noir-repo/compiler/noirc_evaluator/src/brillig/brillig_ir/procedures/mod.rs b/noir/noir-repo/compiler/noirc_evaluator/src/brillig/brillig_ir/procedures/mod.rs index 0955142e414..8e1761d0eee 100644 --- a/noir/noir-repo/compiler/noirc_evaluator/src/brillig/brillig_ir/procedures/mod.rs +++ b/noir/noir-repo/compiler/noirc_evaluator/src/brillig/brillig_ir/procedures/mod.rs @@ -25,7 +25,7 @@ use vector_pop_back::compile_vector_pop_back_procedure; use vector_pop_front::compile_vector_pop_front_procedure; use vector_remove::compile_vector_remove_procedure; -use crate::brillig::brillig_ir::AcirField; +use crate::brillig::{brillig_ir::AcirField, BrilligOptions}; use super::{ artifact::{BrilligArtifact, Label}, @@ -110,8 +110,9 @@ impl std::fmt::Display for ProcedureId { pub(crate) fn compile_procedure( procedure_id: ProcedureId, + options: &BrilligOptions, ) -> BrilligArtifact { - let mut brillig_context = BrilligContext::new_for_procedure(false, procedure_id.clone()); + let mut brillig_context = BrilligContext::new_for_procedure(procedure_id.clone(), options); brillig_context.enter_context(Label::procedure(procedure_id.clone())); match procedure_id { diff --git a/noir/noir-repo/compiler/noirc_evaluator/src/brillig/mod.rs b/noir/noir-repo/compiler/noirc_evaluator/src/brillig/mod.rs index 791f6a466cf..a7e188602ce 100644 --- a/noir/noir-repo/compiler/noirc_evaluator/src/brillig/mod.rs +++ b/noir/noir-repo/compiler/noirc_evaluator/src/brillig/mod.rs @@ -28,6 +28,13 @@ use std::{borrow::Cow, collections::BTreeSet}; pub use self::brillig_ir::procedures::ProcedureId; +/// Options that affect Brillig code generation. +#[derive(Default, Clone, Debug)] +pub struct BrilligOptions { + pub enable_debug_trace: bool, + pub enable_debug_assertions: bool, +} + /// Context structure for the brillig pass. /// It stores brillig-related data required for brillig generation. #[derive(Default)] @@ -43,10 +50,10 @@ impl Brillig { pub(crate) fn compile( &mut self, func: &Function, - enable_debug_trace: bool, + options: &BrilligOptions, globals: &HashMap, ) { - let obj = convert_ssa_function(func, enable_debug_trace, globals); + let obj = convert_ssa_function(func, options, globals); self.ssa_function_to_brillig.insert(func.id(), obj); } @@ -54,13 +61,16 @@ impl Brillig { pub(crate) fn find_by_label( &self, function_label: Label, + options: &BrilligOptions, ) -> Option>> { match function_label.label_type { LabelType::Function(function_id, _) => { self.ssa_function_to_brillig.get(&function_id).map(Cow::Borrowed) } // Procedures are compiled as needed - LabelType::Procedure(procedure_id) => Some(Cow::Owned(compile_procedure(procedure_id))), + LabelType::Procedure(procedure_id) => { + Some(Cow::Owned(compile_procedure(procedure_id, options))) + } LabelType::GlobalInit(function_id) => self.globals.get(&function_id).map(Cow::Borrowed), _ => unreachable!("ICE: Expected a function or procedure label"), } @@ -76,15 +86,15 @@ impl std::ops::Index for Brillig { impl Ssa { #[tracing::instrument(level = "trace", skip_all)] - pub(crate) fn to_brillig(&self, enable_debug_trace: bool) -> Brillig { - self.to_brillig_with_globals(enable_debug_trace, HashMap::default()) + pub(crate) fn to_brillig(&self, options: &BrilligOptions) -> Brillig { + self.to_brillig_with_globals(options, HashMap::default()) } /// Compile Brillig functions and ACIR functions reachable from them #[tracing::instrument(level = "trace", skip_all)] pub(crate) fn to_brillig_with_globals( &self, - enable_debug_trace: bool, + options: &BrilligOptions, used_globals_map: HashMap>, ) -> Brillig { // Collect all the function ids that are reachable from brillig @@ -109,13 +119,13 @@ impl Ssa { // This same globals graph will then be used to declare Brillig globals for the respective entry points. let globals = (*self.functions[&self.main_id].dfg.globals).clone(); let globals_dfg = DataFlowGraph::from(globals); - brillig_globals.declare_globals(&globals_dfg, &mut brillig, enable_debug_trace); + brillig_globals.declare_globals(&globals_dfg, &mut brillig, options); for brillig_function_id in brillig_reachable_function_ids { let globals_allocations = brillig_globals.get_brillig_globals(brillig_function_id); let func = &self.functions[&brillig_function_id]; - brillig.compile(func, enable_debug_trace, &globals_allocations); + brillig.compile(func, options, &globals_allocations); } brillig diff --git a/noir/noir-repo/compiler/noirc_evaluator/src/ssa.rs b/noir/noir-repo/compiler/noirc_evaluator/src/ssa.rs index b57dabf9105..6d051da4550 100644 --- a/noir/noir-repo/compiler/noirc_evaluator/src/ssa.rs +++ b/noir/noir-repo/compiler/noirc_evaluator/src/ssa.rs @@ -13,7 +13,10 @@ use std::{ path::{Path, PathBuf}, }; -use crate::errors::{RuntimeError, SsaReport}; +use crate::{ + brillig::BrilligOptions, + errors::{RuntimeError, SsaReport}, +}; use acvm::{ acir::{ circuit::{ @@ -54,7 +57,8 @@ pub struct SsaEvaluatorOptions { /// Emit debug information for the intermediate SSA IR pub ssa_logging: SsaLogging, - pub enable_brillig_logging: bool, + /// Options affecting Brillig code generation. + pub brillig_options: BrilligOptions, /// Pretty print benchmark times of each code generation pass pub print_codegen_timings: bool, @@ -113,7 +117,7 @@ pub(crate) fn optimize_into_acir( let used_globals_map = std::mem::take(&mut ssa.used_globals); let brillig = time("SSA to Brillig", options.print_codegen_timings, || { - ssa.to_brillig_with_globals(options.enable_brillig_logging, used_globals_map) + ssa.to_brillig_with_globals(&options.brillig_options, used_globals_map) }); let ssa_gen_span = span!(Level::TRACE, "ssa_generation"); @@ -154,7 +158,7 @@ pub(crate) fn optimize_into_acir( drop(ssa_gen_span_guard); let artifacts = time("SSA to ACIR", options.print_codegen_timings, || { - ssa.into_acir(&brillig, options.expression_width) + ssa.into_acir(&brillig, &options.brillig_options, options.expression_width) })?; Ok(ArtifactsAndWarnings(artifacts, ssa_level_warnings)) diff --git a/noir/noir-repo/compiler/noirc_evaluator/src/ssa/checks/check_for_underconstrained_values.rs b/noir/noir-repo/compiler/noirc_evaluator/src/ssa/checks/check_for_underconstrained_values.rs index 3718956a93a..46cf0f12afe 100644 --- a/noir/noir-repo/compiler/noirc_evaluator/src/ssa/checks/check_for_underconstrained_values.rs +++ b/noir/noir-repo/compiler/noirc_evaluator/src/ssa/checks/check_for_underconstrained_values.rs @@ -970,7 +970,7 @@ mod test { v5 = add v4, Field 2 return } - + brillig(inline) fn br f1 { b0(v0: Field, v1: Field): v2 = add v0, v1 @@ -1037,8 +1037,8 @@ mod test { v19 = call f1(v5) -> u32 v20 = add v8, v19 constrain v6 == v20 - dec_rc v4 - dec_rc v5 + dec_rc v4 v4 + dec_rc v5 v5 return } @@ -1368,7 +1368,7 @@ mod test { constrain v32 == v35 return v30, v31, v32 } - + brillig(inline) fn foo f2 { b0(v0: Field): return Field 4, u8 2, v0 diff --git a/noir/noir-repo/compiler/noirc_evaluator/src/ssa/function_builder/mod.rs b/noir/noir-repo/compiler/noirc_evaluator/src/ssa/function_builder/mod.rs index ad0d0acd5e7..e4c00358c8c 100644 --- a/noir/noir-repo/compiler/noirc_evaluator/src/ssa/function_builder/mod.rs +++ b/noir/noir-repo/compiler/noirc_evaluator/src/ssa/function_builder/mod.rs @@ -390,8 +390,8 @@ impl FunctionBuilder { /// Insert an instruction to decrement an array's reference count. This only has an effect /// in unconstrained code where arrays are reference counted and copy on write. - pub(crate) fn insert_dec_rc(&mut self, value: ValueId) { - self.insert_instruction(Instruction::DecrementRc { value }, None); + pub(crate) fn insert_dec_rc(&mut self, value: ValueId, original: ValueId) { + self.insert_instruction(Instruction::DecrementRc { value, original }, None); } /// Insert an enable_side_effects_if instruction. These are normally only automatically @@ -491,53 +491,63 @@ impl FunctionBuilder { /// within the given value. If the given value is not an array and does not contain /// any arrays, this does nothing. /// - /// Returns whether a reference count instruction was issued. - pub(crate) fn increment_array_reference_count(&mut self, value: ValueId) -> bool { - self.update_array_reference_count(value, true) + /// Returns the ID of the array that was affected, if any. + pub(crate) fn increment_array_reference_count(&mut self, value: ValueId) -> Option { + self.update_array_reference_count(value, None) } /// Insert instructions to decrement the reference count of any array(s) stored /// within the given value. If the given value is not an array and does not contain /// any arrays, this does nothing. /// - /// Returns whether a reference count instruction was issued. - pub(crate) fn decrement_array_reference_count(&mut self, value: ValueId) -> bool { - self.update_array_reference_count(value, false) + /// Returns the ID of the array that was affected, if any. + pub(crate) fn decrement_array_reference_count( + &mut self, + value: ValueId, + original: ValueId, + ) -> Option { + self.update_array_reference_count(value, Some(original)) } /// Increment or decrement the given value's reference count if it is an array. /// If it is not an array, this does nothing. Note that inc_rc and dec_rc instructions /// are ignored outside of unconstrained code. /// - /// Returns whether a reference count instruction was issued. - fn update_array_reference_count(&mut self, value: ValueId, increment: bool) -> bool { - if self.current_function.runtime().is_brillig() { - match self.type_of_value(value) { - Type::Numeric(_) => false, - Type::Function => false, - Type::Reference(element) => { - if element.contains_an_array() { - let reference = value; - let value = self.insert_load(reference, element.as_ref().clone()); - self.update_array_reference_count(value, increment); - true - } else { - false - } + /// If there is an `original` value it indicates that we're now decrementing it, + /// but that should only happen if it hasn't been changed by an `array_set` in + /// the meantime, which in itself would make sure its RC is decremented. + /// + /// Returns the ID of the array that was affected, if any. + fn update_array_reference_count( + &mut self, + value: ValueId, + original: Option, + ) -> Option { + if self.current_function.runtime().is_acir() { + return None; + } + match self.type_of_value(value) { + Type::Numeric(_) | Type::Function => None, + Type::Reference(element) => { + if element.contains_an_array() { + let reference = value; + let value = self.insert_load(reference, element.as_ref().clone()); + self.update_array_reference_count(value, original); + Some(value) + } else { + None } - Type::Array(..) | Type::Slice(..) => { - // If there are nested arrays or slices, we wait until ArrayGet - // is issued to increment the count of that array. - if increment { - self.insert_inc_rc(value); - } else { - self.insert_dec_rc(value); - } - true + } + Type::Array(..) | Type::Slice(..) => { + // If there are nested arrays or slices, we wait until ArrayGet + // is issued to increment the count of that array. + if let Some(original) = original { + self.insert_dec_rc(value, original); + } else { + self.insert_inc_rc(value); } + Some(value) } - } else { - false } } diff --git a/noir/noir-repo/compiler/noirc_evaluator/src/ssa/ir/instruction.rs b/noir/noir-repo/compiler/noirc_evaluator/src/ssa/ir/instruction.rs index a0bab3e8e55..6e25469f43c 100644 --- a/noir/noir-repo/compiler/noirc_evaluator/src/ssa/ir/instruction.rs +++ b/noir/noir-repo/compiler/noirc_evaluator/src/ssa/ir/instruction.rs @@ -324,7 +324,9 @@ pub(crate) enum Instruction { /// This currently only has an effect in Brillig code where array sharing and copy on write is /// implemented via reference counting. In ACIR code this is done with im::Vector and these /// DecrementRc instructions are ignored. - DecrementRc { value: ValueId }, + /// + /// The `original` contains the value of the array which was incremented by the pair of this decrement. + DecrementRc { value: ValueId, original: ValueId }, /// Merge two values returned from opposite branches of a conditional into one. /// @@ -721,7 +723,9 @@ impl Instruction { mutable: *mutable, }, Instruction::IncrementRc { value } => Instruction::IncrementRc { value: f(*value) }, - Instruction::DecrementRc { value } => Instruction::DecrementRc { value: f(*value) }, + Instruction::DecrementRc { value, original } => { + Instruction::DecrementRc { value: f(*value), original: f(*original) } + } Instruction::RangeCheck { value, max_bit_size, assert_message } => { Instruction::RangeCheck { value: f(*value), @@ -792,7 +796,10 @@ impl Instruction { *value = f(*value); } Instruction::IncrementRc { value } => *value = f(*value), - Instruction::DecrementRc { value } => *value = f(*value), + Instruction::DecrementRc { value, original } => { + *value = f(*value); + *original = f(*original); + } Instruction::RangeCheck { value, max_bit_size: _, assert_message: _ } => { *value = f(*value); } @@ -858,10 +865,12 @@ impl Instruction { Instruction::EnableSideEffectsIf { condition } => { f(*condition); } - Instruction::IncrementRc { value } - | Instruction::DecrementRc { value } - | Instruction::RangeCheck { value, .. } => { + Instruction::IncrementRc { value } | Instruction::RangeCheck { value, .. } => { + f(*value); + } + Instruction::DecrementRc { value, original } => { f(*value); + f(*original); } Instruction::IfElse { then_condition, then_value, else_condition, else_value } => { f(*then_condition); diff --git a/noir/noir-repo/compiler/noirc_evaluator/src/ssa/ir/printer.rs b/noir/noir-repo/compiler/noirc_evaluator/src/ssa/ir/printer.rs index d3f8363ba66..06f2ecd70ea 100644 --- a/noir/noir-repo/compiler/noirc_evaluator/src/ssa/ir/printer.rs +++ b/noir/noir-repo/compiler/noirc_evaluator/src/ssa/ir/printer.rs @@ -250,8 +250,8 @@ fn display_instruction_inner( Instruction::IncrementRc { value } => { writeln!(f, "inc_rc {}", show(*value)) } - Instruction::DecrementRc { value } => { - writeln!(f, "dec_rc {}", show(*value)) + Instruction::DecrementRc { value, original } => { + writeln!(f, "dec_rc {} {}", show(*value), show(*original)) } Instruction::RangeCheck { value, max_bit_size, .. } => { writeln!(f, "range_check {} to {} bits", show(*value), *max_bit_size,) diff --git a/noir/noir-repo/compiler/noirc_evaluator/src/ssa/opt/constant_folding.rs b/noir/noir-repo/compiler/noirc_evaluator/src/ssa/opt/constant_folding.rs index 4afbc036513..fe2c0d35f73 100644 --- a/noir/noir-repo/compiler/noirc_evaluator/src/ssa/opt/constant_folding.rs +++ b/noir/noir-repo/compiler/noirc_evaluator/src/ssa/opt/constant_folding.rs @@ -34,7 +34,7 @@ use crate::{ brillig::{ brillig_gen::gen_brillig_for, brillig_ir::{artifact::BrilligParameter, brillig_variable::get_bit_size_from_ssa_type}, - Brillig, + Brillig, BrilligOptions, }, ssa::{ ir::{ @@ -590,7 +590,9 @@ impl<'brillig> Context<'brillig> { } } - let Ok(generated_brillig) = gen_brillig_for(func, brillig_arguments, brillig) else { + let Ok(generated_brillig) = + gen_brillig_for(func, brillig_arguments, brillig, &BrilligOptions::default()) + else { return EvaluationResult::CannotEvaluate; }; @@ -630,10 +632,7 @@ impl<'brillig> Context<'brillig> { let memory = memory_values[*memory_index]; *memory_index += 1; - let field_value = match memory { - MemoryValue::Field(field_value) => field_value, - MemoryValue::Integer(u128_value, _) => u128_value.into(), - }; + let field_value = memory.to_field(); dfg.make_constant(field_value, typ) } Type::Array(types, length) => { @@ -806,15 +805,18 @@ mod test { use noirc_frontend::monomorphization::ast::InlineType; - use crate::ssa::{ - function_builder::FunctionBuilder, - ir::{ - function::RuntimeType, - map::Id, - types::{NumericType, Type}, + use crate::{ + brillig::BrilligOptions, + ssa::{ + function_builder::FunctionBuilder, + ir::{ + function::RuntimeType, + map::Id, + types::{NumericType, Type}, + }, + opt::assert_normalized_ssa_equals, + Ssa, }, - opt::assert_normalized_ssa_equals, - Ssa, }; #[test] @@ -1292,7 +1294,7 @@ mod test { } "; let ssa = Ssa::from_str(src).unwrap(); - let brillig = ssa.to_brillig(false); + let brillig = ssa.to_brillig(&BrilligOptions::default()); let expected = " acir(inline) fn main f0 { @@ -1321,7 +1323,7 @@ mod test { } "; let ssa = Ssa::from_str(src).unwrap(); - let brillig = ssa.to_brillig(false); + let brillig = ssa.to_brillig(&BrilligOptions::default()); let expected = " acir(inline) fn main f0 { @@ -1350,7 +1352,7 @@ mod test { } "; let ssa = Ssa::from_str(src).unwrap(); - let brillig = ssa.to_brillig(false); + let brillig = ssa.to_brillig(&BrilligOptions::default()); let expected = " acir(inline) fn main f0 { @@ -1379,7 +1381,7 @@ mod test { } "; let ssa = Ssa::from_str(src).unwrap(); - let brillig = ssa.to_brillig(false); + let brillig = ssa.to_brillig(&BrilligOptions::default()); let expected = " acir(inline) fn main f0 { @@ -1409,7 +1411,7 @@ mod test { } "; let ssa = Ssa::from_str(src).unwrap(); - let brillig = ssa.to_brillig(false); + let brillig = ssa.to_brillig(&BrilligOptions::default()); let expected = " acir(inline) fn main f0 { @@ -1439,12 +1441,12 @@ mod test { v2 = array_get v0, index u32 0 -> Field v4 = array_get v0, index u32 1 -> Field v5 = add v2, v4 - dec_rc v0 + dec_rc v0 v0 return v5 } "; let ssa = Ssa::from_str(src).unwrap(); - let brillig = ssa.to_brillig(false); + let brillig = ssa.to_brillig(&BrilligOptions::default()); let expected = " acir(inline) fn main f0 { @@ -1478,7 +1480,7 @@ mod test { let ssa = Ssa::from_str(src).unwrap(); let mut ssa = ssa.dead_instruction_elimination(); let used_globals_map = std::mem::take(&mut ssa.used_globals); - let brillig = ssa.to_brillig_with_globals(false, used_globals_map); + let brillig = ssa.to_brillig_with_globals(&BrilligOptions::default(), used_globals_map); let expected = " g0 = Field 2 @@ -1520,7 +1522,7 @@ mod test { let ssa = Ssa::from_str(src).unwrap(); let mut ssa = ssa.dead_instruction_elimination(); let used_globals_map = std::mem::take(&mut ssa.used_globals); - let brillig = ssa.to_brillig_with_globals(false, used_globals_map); + let brillig = ssa.to_brillig_with_globals(&BrilligOptions::default(), used_globals_map); let expected = " g0 = Field 2 diff --git a/noir/noir-repo/compiler/noirc_evaluator/src/ssa/opt/die.rs b/noir/noir-repo/compiler/noirc_evaluator/src/ssa/opt/die.rs index 37cd93ca6af..d23cfee8a14 100644 --- a/noir/noir-repo/compiler/noirc_evaluator/src/ssa/opt/die.rs +++ b/noir/noir-repo/compiler/noirc_evaluator/src/ssa/opt/die.rs @@ -284,7 +284,7 @@ impl Context { self.rc_instructions.iter().fold(HashMap::default(), |mut acc, (rc, block)| { let value = match &dfg[*rc] { Instruction::IncrementRc { value } => *value, - Instruction::DecrementRc { value } => *value, + Instruction::DecrementRc { value, .. } => *value, other => { unreachable!( "Expected IncrementRc or DecrementRc instruction, found {other:?}" @@ -434,7 +434,7 @@ impl Context { dfg: &DataFlowGraph, ) -> bool { use Instruction::*; - if let IncrementRc { value } | DecrementRc { value } = instruction { + if let IncrementRc { value } | DecrementRc { value, .. } = instruction { let Some(instruction) = dfg.get_local_or_global_instruction(*value) else { return false; }; @@ -685,7 +685,7 @@ impl<'a> RcTracker<'a> { // Remember that this array was RC'd by this instruction. self.inc_rcs.entry(*value).or_default().insert(instruction_id); } - Instruction::DecrementRc { value } => { + Instruction::DecrementRc { value, .. } => { let typ = function.dfg.type_of_value(*value); // We assume arrays aren't mutated until we find an array_set @@ -835,7 +835,7 @@ mod test { b0(v0: [Field; 2]): inc_rc v0 v2 = array_get v0, index u32 0 -> Field - dec_rc v0 + dec_rc v0 v0 return v2 } "; @@ -859,7 +859,7 @@ mod test { b0(v0: [Field; 2]): inc_rc v0 v2 = array_set v0, index u32 0, value u32 0 - dec_rc v0 + dec_rc v0 v0 return v2 } "; @@ -967,7 +967,7 @@ mod test { v3 = load v0 -> [Field; 3] v6 = array_set v3, index u32 0, value Field 5 store v6 at v0 - dec_rc v6 + dec_rc v6 v1 return } "; diff --git a/noir/noir-repo/compiler/noirc_evaluator/src/ssa/opt/hint.rs b/noir/noir-repo/compiler/noirc_evaluator/src/ssa/opt/hint.rs index 6d7a16a8cfa..be088c3da94 100644 --- a/noir/noir-repo/compiler/noirc_evaluator/src/ssa/opt/hint.rs +++ b/noir/noir-repo/compiler/noirc_evaluator/src/ssa/opt/hint.rs @@ -3,6 +3,7 @@ mod tests { use acvm::acir::circuit::ExpressionWidth; use crate::{ + brillig::BrilligOptions, errors::RuntimeError, ssa::{ opt::assert_normalized_ssa_equals, optimize_all, Ssa, SsaBuilder, SsaEvaluatorOptions, @@ -13,7 +14,7 @@ mod tests { fn run_all_passes(ssa: Ssa) -> Result { let options = &SsaEvaluatorOptions { ssa_logging: SsaLogging::None, - enable_brillig_logging: false, + brillig_options: BrilligOptions::default(), print_codegen_timings: false, expression_width: ExpressionWidth::default(), emit_ssa: None, diff --git a/noir/noir-repo/compiler/noirc_evaluator/src/ssa/opt/rc.rs b/noir/noir-repo/compiler/noirc_evaluator/src/ssa/opt/rc.rs index e25ad350145..13137c94046 100644 --- a/noir/noir-repo/compiler/noirc_evaluator/src/ssa/opt/rc.rs +++ b/noir/noir-repo/compiler/noirc_evaluator/src/ssa/opt/rc.rs @@ -113,7 +113,7 @@ impl Context { let mut to_remove = HashSet::default(); for instruction in function.dfg[last_block].instructions() { - if let Instruction::DecrementRc { value } = &function.dfg[*instruction] { + if let Instruction::DecrementRc { value, .. } = &function.dfg[*instruction] { if let Some(inc_rc) = pop_rc_for(*value, function, &mut self.inc_rcs) { if !inc_rc.possibly_mutated { to_remove.insert(inc_rc.id); @@ -153,20 +153,10 @@ fn remove_instructions(to_remove: HashSet, function: &mut Functio #[cfg(test)] mod test { - use std::sync::Arc; - - use noirc_frontend::monomorphization::ast::InlineType; use crate::ssa::{ - function_builder::FunctionBuilder, - ir::{ - basic_block::BasicBlockId, - dfg::DataFlowGraph, - function::RuntimeType, - instruction::Instruction, - map::Id, - types::{NumericType, Type}, - }, + ir::{basic_block::BasicBlockId, dfg::DataFlowGraph, instruction::Instruction}, + ssa_gen::Ssa, }; fn count_inc_rcs(block: BasicBlockId, dfg: &DataFlowGraph) -> usize { @@ -195,31 +185,18 @@ mod test { // unconstrained fn foo(x: [Field; 2]) -> [[Field; 2]; 1] { // [array] // } - // - // fn foo { - // b0(v0: [Field; 2]): - // inc_rc v0 - // inc_rc v0 - // dec_rc v0 - // v1 = make_array [v0] - // return v1 - // } - let main_id = Id::test_new(0); - let mut builder = FunctionBuilder::new("foo".into(), main_id); - builder.set_runtime(RuntimeType::Brillig(InlineType::default())); - - let inner_array_type = Type::Array(Arc::new(vec![Type::field()]), 2); - let v0 = builder.add_parameter(inner_array_type.clone()); - - builder.insert_inc_rc(v0); - builder.insert_inc_rc(v0); - builder.insert_dec_rc(v0); - - let outer_array_type = Type::Array(Arc::new(vec![inner_array_type]), 1); - let v1 = builder.insert_make_array(vec![v0].into(), outer_array_type); - builder.terminate_with_return(vec![v1]); - - let ssa = builder.finish().remove_paired_rc(); + let src = " + brillig(inline) fn foo f0 { + b0(v0: [Field; 2]): + inc_rc v0 + inc_rc v0 + dec_rc v0 v0 + v1 = make_array [v0] : [[Field; 2]; 1] + return v1 + } + "; + let ssa = Ssa::from_str(src).unwrap(); + let ssa = ssa.remove_paired_rc(); let main = ssa.main(); let entry = main.entry_block(); @@ -232,39 +209,22 @@ mod test { // fn mutator(mut array: [Field; 2]) { // array[0] = 5; // } - // - // fn mutator { - // b0(v0: [Field; 2]): - // v1 = allocate - // store v0 at v1 - // inc_rc v0 - // v2 = load v1 - // v7 = array_set v2, index u64 0, value Field 5 - // store v7 at v1 - // dec_rc v0 - // return - // } - let main_id = Id::test_new(0); - let mut builder = FunctionBuilder::new("mutator".into(), main_id); - builder.set_runtime(RuntimeType::Brillig(InlineType::default())); - - let array_type = Type::Array(Arc::new(vec![Type::field()]), 2); - let v0 = builder.add_parameter(array_type.clone()); - - let v1 = builder.insert_allocate(array_type.clone()); - builder.insert_store(v1, v0); - builder.insert_inc_rc(v0); - let v2 = builder.insert_load(v1, array_type); - - let zero = builder.numeric_constant(0u128, NumericType::unsigned(64)); - let five = builder.field_constant(5u128); - let v7 = builder.insert_array_set(v2, zero, five); - - builder.insert_store(v1, v7); - builder.insert_dec_rc(v0); - builder.terminate_with_return(vec![]); + let src = " + brillig(inline) fn mutator f0 { + b0(v0: [Field; 2]): + v1 = allocate -> &mut [Field; 2] + store v0 at v1 + inc_rc v0 + v2 = load v1 -> [Field; 2] + v5 = array_set v2, index u64 0, value Field 5 + store v5 at v1 + dec_rc v0 v0 + return + } + "; - let ssa = builder.finish().remove_paired_rc(); + let ssa = Ssa::from_str(src).unwrap(); + let ssa = ssa.remove_paired_rc(); let main = ssa.main(); let entry = main.entry_block(); @@ -280,45 +240,24 @@ mod test { // fn mutator2(array: &mut [Field; 2]) { // array[0] = 5; // } - // - // fn mutator2 { - // b0(v0: &mut [Field; 2]): - // v1 = load v0 - // inc_rc v1 - // store v1 at v0 - // v2 = load v0 - // v7 = array_set v2, index u64 0, value Field 5 - // store v7 at v0 - // v8 = load v0 - // dec_rc v8 - // store v8 at v0 - // return - // } - let main_id = Id::test_new(0); - let mut builder = FunctionBuilder::new("mutator2".into(), main_id); - builder.set_runtime(RuntimeType::Brillig(InlineType::default())); - - let array_type = Type::Array(Arc::new(vec![Type::field()]), 2); - let reference_type = Type::Reference(Arc::new(array_type.clone())); - - let v0 = builder.add_parameter(reference_type); - - let v1 = builder.insert_load(v0, array_type.clone()); - builder.insert_inc_rc(v1); - builder.insert_store(v0, v1); - - let v2 = builder.insert_load(v1, array_type.clone()); - let zero = builder.numeric_constant(0u128, NumericType::unsigned(64)); - let five = builder.field_constant(5u128); - let v7 = builder.insert_array_set(v2, zero, five); - - builder.insert_store(v0, v7); - let v8 = builder.insert_load(v0, array_type); - builder.insert_dec_rc(v8); - builder.insert_store(v0, v8); - builder.terminate_with_return(vec![]); + let src = " + brillig(inline) fn mutator2 f0 { + b0(v0: &mut [Field; 2]): + v1 = load v0 -> [Field; 2] + inc_rc v1 + store v1 at v0 + v2 = load v1 -> [Field; 2] + v5 = array_set v2, index u64 0, value Field 5 + store v5 at v0 + v6 = load v0 -> [Field; 2] + dec_rc v6 v1 + store v6 at v0 + return + } + "; - let ssa = builder.finish().remove_paired_rc(); + let ssa = Ssa::from_str(src).unwrap(); + let ssa = ssa.remove_paired_rc(); let main = ssa.main(); let entry = main.entry_block(); diff --git a/noir/noir-repo/compiler/noirc_evaluator/src/ssa/opt/unrolling.rs b/noir/noir-repo/compiler/noirc_evaluator/src/ssa/opt/unrolling.rs index 014ff721b76..547a8a042c6 100644 --- a/noir/noir-repo/compiler/noirc_evaluator/src/ssa/opt/unrolling.rs +++ b/noir/noir-repo/compiler/noirc_evaluator/src/ssa/opt/unrolling.rs @@ -1275,7 +1275,7 @@ mod tests { jmp b1() b1(): v20 = load v3 -> [u64; 6] - dec_rc v0 + dec_rc v0 v0 return v20 } "; @@ -1451,7 +1451,7 @@ mod tests { jmp b1(v16) b2(): v8 = load v4 -> [u64; 6] - dec_rc v0 + dec_rc v0 v0 return v8 }} " diff --git a/noir/noir-repo/compiler/noirc_evaluator/src/ssa/parser/ast.rs b/noir/noir-repo/compiler/noirc_evaluator/src/ssa/parser/ast.rs index 05743ffd7ca..7bc88a1fde1 100644 --- a/noir/noir-repo/compiler/noirc_evaluator/src/ssa/parser/ast.rs +++ b/noir/noir-repo/compiler/noirc_evaluator/src/ssa/parser/ast.rs @@ -112,6 +112,7 @@ pub(crate) enum ParsedInstruction { }, DecrementRc { value: ParsedValue, + original: ParsedValue, }, EnableSideEffectsIf { condition: ParsedValue, diff --git a/noir/noir-repo/compiler/noirc_evaluator/src/ssa/parser/into_ssa.rs b/noir/noir-repo/compiler/noirc_evaluator/src/ssa/parser/into_ssa.rs index 9fb6f43535c..fc9b1ae98bc 100644 --- a/noir/noir-repo/compiler/noirc_evaluator/src/ssa/parser/into_ssa.rs +++ b/noir/noir-repo/compiler/noirc_evaluator/src/ssa/parser/into_ssa.rs @@ -267,9 +267,10 @@ impl Translator { }; self.builder.insert_constrain(lhs, rhs, assert_message); } - ParsedInstruction::DecrementRc { value } => { + ParsedInstruction::DecrementRc { value, original } => { let value = self.translate_value(value)?; - self.builder.decrement_array_reference_count(value); + let original = self.translate_value(original)?; + self.builder.decrement_array_reference_count(value, original); } ParsedInstruction::EnableSideEffectsIf { condition } => { let condition = self.translate_value(condition)?; diff --git a/noir/noir-repo/compiler/noirc_evaluator/src/ssa/parser/mod.rs b/noir/noir-repo/compiler/noirc_evaluator/src/ssa/parser/mod.rs index fc50bdfba8e..4c5f6334430 100644 --- a/noir/noir-repo/compiler/noirc_evaluator/src/ssa/parser/mod.rs +++ b/noir/noir-repo/compiler/noirc_evaluator/src/ssa/parser/mod.rs @@ -1,5 +1,6 @@ use std::{ fmt::{self, Debug, Formatter}, + str::FromStr, sync::Arc, }; @@ -31,10 +32,18 @@ mod lexer; mod tests; mod token; +impl FromStr for Ssa { + type Err = SsaErrorWithSource; + + fn from_str(s: &str) -> Result { + Self::from_str_impl(s, false) + } +} + impl Ssa { /// Creates an Ssa object from the given string. pub(crate) fn from_str(src: &str) -> Result { - Self::from_str_impl(src, false) + FromStr::from_str(src) } /// Creates an Ssa object from the given string but trying to simplify @@ -387,7 +396,8 @@ impl<'a> Parser<'a> { } let value = self.parse_value_or_error()?; - Ok(Some(ParsedInstruction::DecrementRc { value })) + let original = self.parse_value_or_error()?; + Ok(Some(ParsedInstruction::DecrementRc { value, original })) } fn parse_enable_side_effects(&mut self) -> ParseResult> { diff --git a/noir/noir-repo/compiler/noirc_evaluator/src/ssa/parser/tests.rs b/noir/noir-repo/compiler/noirc_evaluator/src/ssa/parser/tests.rs index c803e2a94fe..358c2e89a41 100644 --- a/noir/noir-repo/compiler/noirc_evaluator/src/ssa/parser/tests.rs +++ b/noir/noir-repo/compiler/noirc_evaluator/src/ssa/parser/tests.rs @@ -445,7 +445,7 @@ fn test_dec_rc() { let src = " brillig(inline) fn main f0 { b0(v0: [Field; 3]): - dec_rc v0 + dec_rc v0 v0 return } "; diff --git a/noir/noir-repo/compiler/noirc_evaluator/src/ssa/ssa_gen/context.rs b/noir/noir-repo/compiler/noirc_evaluator/src/ssa/ssa_gen/context.rs index f0a52727a7a..cbac4bd6d84 100644 --- a/noir/noir-repo/compiler/noirc_evaluator/src/ssa/ssa_gen/context.rs +++ b/noir/noir-repo/compiler/noirc_evaluator/src/ssa/ssa_gen/context.rs @@ -964,11 +964,13 @@ impl<'a> FunctionContext<'a> { /// /// This is done on parameters rather than call arguments so that we can optimize out /// paired inc/dec instructions within brillig functions more easily. - pub(crate) fn increment_parameter_rcs(&mut self) -> HashSet { + /// + /// Returns the list of parameters incremented, together with the value ID of the arrays they refer to. + pub(crate) fn increment_parameter_rcs(&mut self) -> Vec<(ValueId, ValueId)> { let entry = self.builder.current_function.entry_block(); let parameters = self.builder.current_function.dfg.block_parameters(entry).to_vec(); - let mut incremented = HashSet::default(); + let mut incremented = Vec::default(); let mut seen_array_types = HashSet::default(); for parameter in parameters { @@ -979,10 +981,11 @@ impl<'a> FunctionContext<'a> { if element.contains_an_array() { // If we haven't already seen this array type, the value may be possibly // aliased, so issue an inc_rc for it. - if !seen_array_types.insert(element.get_contained_array().clone()) - && self.builder.increment_array_reference_count(parameter) - { - incremented.insert(parameter); + if seen_array_types.insert(element.get_contained_array().clone()) { + continue; + } + if let Some(id) = self.builder.increment_array_reference_count(parameter) { + incremented.push((parameter, id)); } } } @@ -997,14 +1000,14 @@ impl<'a> FunctionContext<'a> { /// ignored. pub(crate) fn end_scope( &mut self, - mut incremented_params: HashSet, + mut incremented_params: Vec<(ValueId, ValueId)>, terminator_args: &[ValueId], ) { - incremented_params.retain(|parameter| !terminator_args.contains(parameter)); + incremented_params.retain(|(parameter, _)| !terminator_args.contains(parameter)); - for parameter in incremented_params { + for (parameter, original) in incremented_params { if self.builder.current_function.dfg.value_is_reference(parameter) { - self.builder.decrement_array_reference_count(parameter); + self.builder.decrement_array_reference_count(parameter, original); } } } diff --git a/noir/noir-repo/docs/docs/noir/concepts/data_types/integers.md b/noir/noir-repo/docs/docs/noir/concepts/data_types/integers.md index 41a823646dd..b8a5d498029 100644 --- a/noir/noir-repo/docs/docs/noir/concepts/data_types/integers.md +++ b/noir/noir-repo/docs/docs/noir/concepts/data_types/integers.md @@ -111,8 +111,9 @@ fn main(x: U128, y: U128) { Computations that exceed the type boundaries will result in overflow errors. This happens with both signed and unsigned integers. For example, attempting to prove: ```rust -fn main(x: u8, y: u8) { +fn main(x: u8, y: u8) -> pub u8 { let z = x + y; + z } ``` @@ -140,10 +141,20 @@ error: Assertion failed: 'attempt to add with overflow' A similar error would happen with signed integers: ```rust -fn main() { +fn main() -> i8 { let x: i8 = -118; let y: i8 = -11; let z = x + y; + z +} +``` + +Note that if a computation ends up being unused the compiler might remove it and it won't end up producing an overflow: + +```rust +fn main() { + // "255 + 1" would overflow, but `z` is unused so no computation happens + let z: u8 = 255 + 1; } ``` diff --git a/noir/noir-repo/test_programs/execution_success/reference_counts/src/main.nr b/noir/noir-repo/test_programs/execution_success/reference_counts/src/main.nr index 8de4d0f2508..a4c573baf39 100644 --- a/noir/noir-repo/test_programs/execution_success/reference_counts/src/main.nr +++ b/noir/noir-repo/test_programs/execution_success/reference_counts/src/main.nr @@ -6,7 +6,7 @@ fn main() { borrow(array, array_refcount(array)); borrow_mut(&mut array, array_refcount(array)); - copy_mut(array, array_refcount(array)); + let _ = copy_mut(array, array_refcount(array)); borrow_mut_two(&mut array, &mut array, array_refcount(array)); @@ -14,6 +14,11 @@ fn main() { let rc1 = array_refcount(array); let rc2 = array_refcount(u32_array); borrow_mut_two_separate(&mut array, &mut u32_array, rc1, rc2); + + /// Safety: test + unsafe { + regression_7297(); + } } fn borrow(array: [Field; 3], rc_before_call: u32) { @@ -29,10 +34,13 @@ fn borrow_mut(array: &mut [Field; 3], rc_before_call: u32) { println(array[0]); } -fn copy_mut(mut array: [Field; 3], rc_before_call: u32) { +// Returning a copy of the array, otherwise the SSA can end up optimizing away +// the `array_set`, with the whole body just becoming basically `println(4);`. +fn copy_mut(mut array: [Field; 3], rc_before_call: u32) -> [Field; 3] { assert_refcount(array, rc_before_call + 1); array[0] = 4; println(array[0]); + array } /// Borrow the same array mutably through both parameters, inc_rc is necessary here, although @@ -65,7 +73,7 @@ fn borrow_mut_two_separate( fn assert_refcount(array: [T; 3], expected: u32) { let count = array_refcount(array); - // All refcounts are zero when running this as a constrained program + // All ref counts are zero when running this as a constrained program if std::runtime::is_unconstrained() { if count != expected { // Brillig doesn't print the actual & expected arguments on assertion failure @@ -76,3 +84,34 @@ fn assert_refcount(array: [T; 3], expected: u32) { assert_eq(count, 0); } } + +unconstrained fn regression_7297() { + println("regression_7297"); + let mut array = [0, 1, 2]; + + let refcount_0 = array_refcount(array); + borrow_mut_two(&mut array, &mut array, refcount_0); + let refcount_1 = array_refcount(array); + let array_2 = copy_mut(array, refcount_1); + let refcount_2 = array_refcount(array); + + println([refcount_0, refcount_1, refcount_2]); + + // Mutation of the original could occur if we double decremented the RC and then went back to 1 by accident. + // For this to come out we have to run the test with `--inliner-aggressiveness -9223372036854775808` + assert_eq(array[0], 6, "the original should not be mutated by copy_mut, only borrow_mut_two"); + assert_eq(array_2[0], 4, "the copy should have the expected content"); + // Double decrementing the RC could occur if we don't realize that array mutation made a copy, + // which decreases the RC of the original and sets the new one to 1. + // This assertion is redundant with the one following it, but it's here because `assert_eq` doesn't print + // what actual values that cause it to fail, so this is a way to highlight the bug about the refcount of + // still live arrays going to zero, without any doubt that it's just not 1, as it should be. + assert(refcount_1 != 0, "borrow_mut_two should create a fresh array and not decrease its RC"); + assert_eq(refcount_1, 1, "borrow_mut_two should create a fresh array with an RC of 1"); + // XXX: This assertion is here to demonstrate that this is happening, not because it must be like this + assert_eq( + refcount_2, + refcount_1 + 1, + "not ideal, but original RC permanently increased by copy_mut", + ); +} diff --git a/noir/noir-repo/tooling/lsp/src/requests/mod.rs b/noir/noir-repo/tooling/lsp/src/requests/mod.rs index 7bc510a2f6c..1789c3513f6 100644 --- a/noir/noir-repo/tooling/lsp/src/requests/mod.rs +++ b/noir/noir-repo/tooling/lsp/src/requests/mod.rs @@ -1,5 +1,5 @@ use std::collections::BTreeMap; -use std::path::PathBuf; +use std::path::{Path, PathBuf}; use std::{collections::HashMap, future::Future}; use crate::{insert_all_files_for_workspace_into_file_manager, parse_diff, PackageCacheData}; @@ -301,6 +301,10 @@ fn on_formatting_inner( state: &LspState, params: lsp_types::DocumentFormattingParams, ) -> Result>, ResponseError> { + // The file_path might be Err/None if the action runs against an unsaved file + let file_path = params.text_document.uri.to_file_path().ok(); + let directory_path = file_path.as_ref().and_then(|path| path.parent()); + let path = params.text_document.uri.to_string(); if let Some(source) = state.input_files.get(&path) { @@ -310,7 +314,8 @@ fn on_formatting_inner( return Ok(None); } - let new_text = nargo_fmt::format(source, module, &Config::default()); + let config = read_format_config(directory_path); + let new_text = nargo_fmt::format(source, module, &config); let start_position = Position { line: 0, character: 0 }; let end_position = Position { @@ -327,6 +332,19 @@ fn on_formatting_inner( } } +fn read_format_config(file_path: Option<&Path>) -> Config { + match file_path { + Some(file_path) => match Config::read(file_path) { + Ok(config) => config, + Err(err) => { + eprintln!("{}", err); + Config::default() + } + }, + None => Config::default(), + } +} + pub(crate) fn position_to_byte_index<'a, F>( files: &'a F, file_id: F::FileId, diff --git a/noir/noir-repo/tooling/nargo_cli/build.rs b/noir/noir-repo/tooling/nargo_cli/build.rs index 5e101bc0483..303576d137d 100644 --- a/noir/noir-repo/tooling/nargo_cli/build.rs +++ b/noir/noir-repo/tooling/nargo_cli/build.rs @@ -193,6 +193,8 @@ fn test_{test_name}(force_brillig: ForceBrillig, inliner_aggressiveness: Inliner nargo.arg("--inliner-aggressiveness").arg(inliner_aggressiveness.0.to_string()); // Check whether the test case is non-deterministic nargo.arg("--check-non-determinism"); + // Allow more bytecode in exchange to catch illegal states. + nargo.arg("--enable-brillig-debug-assertions"); if force_brillig.0 {{ nargo.arg("--force-brillig"); diff --git a/noir/noir-repo/tooling/nargo_fmt/build.rs b/noir/noir-repo/tooling/nargo_fmt/build.rs index a95cbe16525..988a7dcc94d 100644 --- a/noir/noir-repo/tooling/nargo_fmt/build.rs +++ b/noir/noir-repo/tooling/nargo_fmt/build.rs @@ -62,7 +62,7 @@ fn generate_formatter_tests(test_file: &mut File, test_data_dir: &Path) { let (parsed_module, _errors) = noirc_frontend::parse_program(input); - let config = nargo_fmt::Config::of("{config}").unwrap(); + let config = nargo_fmt::Config::of("{config}", &std::path::PathBuf::new()).unwrap(); let fmt_text = nargo_fmt::format(input, parsed_module, &config); if std::env::var("UPDATE_EXPECT").is_ok() {{ @@ -84,7 +84,7 @@ fn generate_formatter_tests(test_file: &mut File, test_data_dir: &Path) { let (parsed_module, _errors) = noirc_frontend::parse_program(expected_output); - let config = nargo_fmt::Config::of("{config}").unwrap(); + let config = nargo_fmt::Config::of("{config}", &std::path::PathBuf::new()).unwrap(); let fmt_text = nargo_fmt::format(expected_output, parsed_module, &config); similar_asserts::assert_eq!(fmt_text, expected_output); diff --git a/noir/noir-repo/tooling/nargo_fmt/src/config.rs b/noir/noir-repo/tooling/nargo_fmt/src/config.rs index 488647c0b39..f01afc87af2 100644 --- a/noir/noir-repo/tooling/nargo_fmt/src/config.rs +++ b/noir/noir-repo/tooling/nargo_fmt/src/config.rs @@ -56,21 +56,30 @@ config! { } impl Config { - pub fn read(path: &Path) -> Result { - let config_path = path.join("noirfmt.toml"); + /// Reads a Config starting at the given path and going through the path parents + /// until a `noirfmt.toml` file is found in one of them or the root is reached. + pub fn read(mut path: &Path) -> Result { + loop { + let config_path = path.join("noirfmt.toml"); + if config_path.exists() { + match std::fs::read_to_string(&config_path) { + Ok(input) => return Self::of(&input, &config_path), + Err(cause) => return Err(ConfigError::ReadFailed(config_path, cause)), + }; + } - let input = match std::fs::read_to_string(&config_path) { - Ok(input) => input, - Err(cause) if cause.kind() == std::io::ErrorKind::NotFound => String::new(), - Err(cause) => return Err(ConfigError::ReadFailed(config_path, cause)), - }; + let Some(parent_path) = path.parent() else { + return Ok(Config::default()); + }; - Self::of(&input) + path = parent_path; + } } - pub fn of(s: &str) -> Result { + pub fn of(s: &str, path: &Path) -> Result { let mut config = Self::default(); - let toml = toml::from_str(s).map_err(ConfigError::MalformedFile)?; + let toml = + toml::from_str(s).map_err(|err| ConfigError::MalformedFile(path.to_path_buf(), err))?; config.fill_from_toml(toml); Ok(config) } diff --git a/noir/noir-repo/tooling/nargo_fmt/src/errors.rs b/noir/noir-repo/tooling/nargo_fmt/src/errors.rs index e0a1758ae0f..f766234dcde 100644 --- a/noir/noir-repo/tooling/nargo_fmt/src/errors.rs +++ b/noir/noir-repo/tooling/nargo_fmt/src/errors.rs @@ -7,6 +7,6 @@ pub enum ConfigError { #[error("Cannot read file {0} - {1}")] ReadFailed(PathBuf, std::io::Error), - #[error("noirfmt.toml is badly formed, could not parse.\n\n {0}")] - MalformedFile(#[from] toml::de::Error), + #[error("{0} is badly formed, could not parse.\n\n {1}")] + MalformedFile(PathBuf, toml::de::Error), }