From 044d0fef3bbecf673c579bd63d2640dc81b35ba3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=81lvaro=20Rodr=C3=ADguez?= Date: Thu, 9 May 2024 17:42:45 +0200 Subject: [PATCH] fix: temporarily revert to_radix blackbox (#6304) This reverts commit ac27376b9a0cdf0624a02d36c64ec25886b44b4a. --- .../dsl/acir_format/serde/acir.hpp | 74 ++----------------- .../noir-repo/acvm-repo/acir/codegen/acir.cpp | 56 +------------- .../acvm-repo/brillig/src/black_box.rs | 5 -- .../acvm-repo/brillig_vm/src/black_box.rs | 21 ------ .../src/brillig/brillig_gen/brillig_block.rs | 38 +++------- .../brillig/brillig_ir/codegen_intrinsic.rs | 62 +++++++++------- .../src/brillig/brillig_ir/debug_show.rs | 9 --- noir/noir-repo/noir_stdlib/src/field/bn254.nr | 57 ++++++-------- 8 files changed, 73 insertions(+), 249 deletions(-) diff --git a/barretenberg/cpp/src/barretenberg/dsl/acir_format/serde/acir.hpp b/barretenberg/cpp/src/barretenberg/dsl/acir_format/serde/acir.hpp index 683e4c62407..9fb0e2b3a35 100644 --- a/barretenberg/cpp/src/barretenberg/dsl/acir_format/serde/acir.hpp +++ b/barretenberg/cpp/src/barretenberg/dsl/acir_format/serde/acir.hpp @@ -686,6 +686,7 @@ struct BlackBoxOp { Program::HeapVector inputs; Program::HeapArray iv; Program::HeapArray key; + Program::MemoryAddress length; Program::HeapVector outputs; friend bool operator==(const AES128Encrypt&, const AES128Encrypt&); @@ -895,16 +896,6 @@ struct BlackBoxOp { static Sha256Compression bincodeDeserialize(std::vector); }; - struct ToRadix { - Program::MemoryAddress input; - uint32_t radix; - Program::HeapArray output; - - friend bool operator==(const ToRadix&, const ToRadix&); - std::vector bincodeSerialize() const; - static ToRadix bincodeDeserialize(std::vector); - }; - std::variant + Sha256Compression> value; friend bool operator==(const BlackBoxOp&, const BlackBoxOp&); @@ -3949,6 +3939,9 @@ inline bool operator==(const BlackBoxOp::AES128Encrypt& lhs, const BlackBoxOp::A if (!(lhs.key == rhs.key)) { return false; } + if (!(lhs.length == rhs.length)) { + return false; + } if (!(lhs.outputs == rhs.outputs)) { return false; } @@ -5148,63 +5141,6 @@ Program::BlackBoxOp::Sha256Compression serde::Deserializable BlackBoxOp::ToRadix::bincodeSerialize() const -{ - auto serializer = serde::BincodeSerializer(); - serde::Serializable::serialize(*this, serializer); - return std::move(serializer).bytes(); -} - -inline BlackBoxOp::ToRadix BlackBoxOp::ToRadix::bincodeDeserialize(std::vector input) -{ - auto deserializer = serde::BincodeDeserializer(input); - auto value = serde::Deserializable::deserialize(deserializer); - if (deserializer.get_buffer_offset() < input.size()) { - throw_or_abort("Some input bytes were not read"); - } - return value; -} - -} // end of namespace Program - -template <> -template -void serde::Serializable::serialize(const Program::BlackBoxOp::ToRadix& obj, - Serializer& serializer) -{ - serde::Serializable::serialize(obj.input, serializer); - serde::Serializable::serialize(obj.radix, serializer); - serde::Serializable::serialize(obj.output, serializer); -} - -template <> -template -Program::BlackBoxOp::ToRadix serde::Deserializable::deserialize( - Deserializer& deserializer) -{ - Program::BlackBoxOp::ToRadix obj; - obj.input = serde::Deserializable::deserialize(deserializer); - obj.radix = serde::Deserializable::deserialize(deserializer); - obj.output = serde::Deserializable::deserialize(deserializer); - return obj; -} - -namespace Program { - inline bool operator==(const BlockId& lhs, const BlockId& rhs) { if (!(lhs.value == rhs.value)) { diff --git a/noir/noir-repo/acvm-repo/acir/codegen/acir.cpp b/noir/noir-repo/acvm-repo/acir/codegen/acir.cpp index 222a7da6399..5afcd68e987 100644 --- a/noir/noir-repo/acvm-repo/acir/codegen/acir.cpp +++ b/noir/noir-repo/acvm-repo/acir/codegen/acir.cpp @@ -870,17 +870,7 @@ namespace Program { static Sha256Compression bincodeDeserialize(std::vector); }; - struct ToRadix { - Program::MemoryAddress input; - uint32_t radix; - Program::HeapArray output; - - friend bool operator==(const ToRadix&, const ToRadix&); - std::vector bincodeSerialize() const; - static ToRadix bincodeDeserialize(std::vector); - }; - - std::variant value; + std::variant value; friend bool operator==(const BlackBoxOp&, const BlackBoxOp&); std::vector bincodeSerialize() const; @@ -4303,50 +4293,6 @@ Program::BlackBoxOp::Sha256Compression serde::Deserializable BlackBoxOp::ToRadix::bincodeSerialize() const { - auto serializer = serde::BincodeSerializer(); - serde::Serializable::serialize(*this, serializer); - return std::move(serializer).bytes(); - } - - inline BlackBoxOp::ToRadix BlackBoxOp::ToRadix::bincodeDeserialize(std::vector input) { - auto deserializer = serde::BincodeDeserializer(input); - auto value = serde::Deserializable::deserialize(deserializer); - if (deserializer.get_buffer_offset() < input.size()) { - throw serde::deserialization_error("Some input bytes were not read"); - } - return value; - } - -} // end of namespace Program - -template <> -template -void serde::Serializable::serialize(const Program::BlackBoxOp::ToRadix &obj, Serializer &serializer) { - serde::Serializable::serialize(obj.input, serializer); - serde::Serializable::serialize(obj.radix, serializer); - serde::Serializable::serialize(obj.output, serializer); -} - -template <> -template -Program::BlackBoxOp::ToRadix serde::Deserializable::deserialize(Deserializer &deserializer) { - Program::BlackBoxOp::ToRadix obj; - obj.input = serde::Deserializable::deserialize(deserializer); - obj.radix = serde::Deserializable::deserialize(deserializer); - obj.output = serde::Deserializable::deserialize(deserializer); - return obj; -} - namespace Program { inline bool operator==(const BlockId &lhs, const BlockId &rhs) { diff --git a/noir/noir-repo/acvm-repo/brillig/src/black_box.rs b/noir/noir-repo/acvm-repo/brillig/src/black_box.rs index 9a66b428dc3..15abc19ed90 100644 --- a/noir/noir-repo/acvm-repo/brillig/src/black_box.rs +++ b/noir/noir-repo/acvm-repo/brillig/src/black_box.rs @@ -126,9 +126,4 @@ pub enum BlackBoxOp { hash_values: HeapVector, output: HeapArray, }, - ToRadix { - input: MemoryAddress, - radix: u32, - output: HeapArray, - }, } 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 d6ecd25f454..c999b5bf330 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 @@ -5,7 +5,6 @@ use acvm_blackbox_solver::{ aes128_encrypt, blake2s, blake3, ecdsa_secp256k1_verify, ecdsa_secp256r1_verify, keccak256, keccakf1600, sha256, sha256compression, BlackBoxFunctionSolver, BlackBoxResolutionError, }; -use num_bigint::BigUint; use crate::memory::MemoryValue; use crate::Memory; @@ -296,25 +295,6 @@ pub(crate) fn evaluate_black_box( memory.write_slice(memory.read_ref(output.pointer), &state); Ok(()) } - BlackBoxOp::ToRadix { input, radix, output } => { - let input: FieldElement = - memory.read(*input).try_into().expect("ToRadix input not a field"); - - let mut input = BigUint::from_bytes_be(&input.to_be_bytes()); - let radix = BigUint::from(*radix); - - let mut limbs: Vec = Vec::with_capacity(output.size); - - for _ in 0..output.size { - let limb = &input % &radix; - limbs.push(FieldElement::from_be_bytes_reduce(&limb.to_bytes_be()).into()); - input /= &radix; - } - - memory.write_slice(memory.read_ref(output.pointer), &limbs); - - Ok(()) - } } } @@ -341,7 +321,6 @@ fn black_box_function_from_op(op: &BlackBoxOp) -> BlackBoxFunc { BlackBoxOp::BigIntToLeBytes { .. } => BlackBoxFunc::BigIntToLeBytes, BlackBoxOp::Poseidon2Permutation { .. } => BlackBoxFunc::Poseidon2Permutation, BlackBoxOp::Sha256Compression { .. } => BlackBoxFunc::Sha256Compression, - BlackBoxOp::ToRadix { .. } => unreachable!("ToRadix is not an ACIR BlackBoxFunc"), } } 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 6a4f9f5cc0e..f660c8e0b7a 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 @@ -488,22 +488,8 @@ impl<'block> BrilligBlock<'block> { } Value::Intrinsic(Intrinsic::ToRadix(endianness)) => { let source = self.convert_ssa_single_addr_value(arguments[0], dfg); - - let radix: u32 = dfg - .get_numeric_constant(arguments[1]) - .expect("Radix should be known") - .try_to_u64() - .expect("Radix should fit in u64") - .try_into() - .expect("Radix should be u32"); - - let limb_count: usize = dfg - .get_numeric_constant(arguments[2]) - .expect("Limb count should be known") - .try_to_u64() - .expect("Limb count should fit in u64") - .try_into() - .expect("Limb count should fit in usize"); + let radix = self.convert_ssa_single_addr_value(arguments[1], dfg); + let limb_count = self.convert_ssa_single_addr_value(arguments[2], dfg); let results = dfg.instruction_results(instruction_id); @@ -525,8 +511,7 @@ impl<'block> BrilligBlock<'block> { .extract_vector(); // Update the user-facing slice length - self.brillig_context - .usize_const_instruction(target_len.address, limb_count.into()); + self.brillig_context.cast_instruction(target_len, limb_count); self.brillig_context.codegen_to_radix( source, @@ -539,13 +524,7 @@ impl<'block> BrilligBlock<'block> { } Value::Intrinsic(Intrinsic::ToBits(endianness)) => { let source = self.convert_ssa_single_addr_value(arguments[0], dfg); - let limb_count: usize = dfg - .get_numeric_constant(arguments[1]) - .expect("Limb count should be known") - .try_to_u64() - .expect("Limb count should fit in u64") - .try_into() - .expect("Limb count should fit in usize"); + let limb_count = self.convert_ssa_single_addr_value(arguments[1], dfg); let results = dfg.instruction_results(instruction_id); @@ -570,18 +549,21 @@ impl<'block> BrilligBlock<'block> { BrilligVariable::SingleAddr(..) => unreachable!("ICE: ToBits on non-array"), }; + let radix = self.brillig_context.make_constant_instruction(2_usize.into(), 32); + // Update the user-facing slice length - self.brillig_context - .usize_const_instruction(target_len.address, limb_count.into()); + self.brillig_context.cast_instruction(target_len, limb_count); self.brillig_context.codegen_to_radix( source, target_vector, - 2, + radix, limb_count, matches!(endianness, Endian::Big), 1, ); + + self.brillig_context.deallocate_single_addr(radix); } _ => { unreachable!("unsupported function call type {:?}", dfg[*func]) diff --git a/noir/noir-repo/compiler/noirc_evaluator/src/brillig/brillig_ir/codegen_intrinsic.rs b/noir/noir-repo/compiler/noirc_evaluator/src/brillig/brillig_ir/codegen_intrinsic.rs index 58166554e1d..ab756217bcd 100644 --- a/noir/noir-repo/compiler/noirc_evaluator/src/brillig/brillig_ir/codegen_intrinsic.rs +++ b/noir/noir-repo/compiler/noirc_evaluator/src/brillig/brillig_ir/codegen_intrinsic.rs @@ -1,7 +1,6 @@ -use acvm::{ - acir::brillig::{BlackBoxOp, HeapArray}, - FieldElement, -}; +use acvm::FieldElement; + +use crate::brillig::brillig_ir::BrilligBinaryOp; use super::{ brillig_variable::{BrilligVector, SingleAddrVariable}, @@ -37,46 +36,57 @@ impl BrilligContext { &mut self, source_field: SingleAddrVariable, target_vector: BrilligVector, - radix: u32, - limb_count: usize, + radix: SingleAddrVariable, + limb_count: SingleAddrVariable, big_endian: bool, limb_bit_size: u32, ) { assert!(source_field.bit_size == FieldElement::max_num_bits()); + assert!(radix.bit_size == 32); + assert!(limb_count.bit_size == 32); + let radix_as_field = + SingleAddrVariable::new(self.allocate_register(), FieldElement::max_num_bits()); + self.cast_instruction(radix_as_field, radix); - self.usize_const_instruction(target_vector.size, limb_count.into()); + self.cast_instruction(SingleAddrVariable::new_usize(target_vector.size), limb_count); self.usize_const_instruction(target_vector.rc, 1_usize.into()); self.codegen_allocate_array(target_vector.pointer, target_vector.size); - self.black_box_op_instruction(BlackBoxOp::ToRadix { - input: source_field.address, - radix, - output: HeapArray { pointer: target_vector.pointer, size: limb_count }, - }); + let shifted_field = + SingleAddrVariable::new(self.allocate_register(), FieldElement::max_num_bits()); + self.mov_instruction(shifted_field.address, source_field.address); let limb_field = SingleAddrVariable::new(self.allocate_register(), FieldElement::max_num_bits()); let limb_casted = SingleAddrVariable::new(self.allocate_register(), limb_bit_size); - if limb_bit_size != FieldElement::max_num_bits() { - self.codegen_loop(target_vector.size, |ctx, iterator_register| { - // Read the limb - ctx.codegen_array_get(target_vector.pointer, iterator_register, limb_field.address); - // Cast it - ctx.cast_instruction(limb_casted, limb_field); - // Write it - ctx.codegen_array_set( - target_vector.pointer, - iterator_register, - limb_casted.address, - ); - }); - } + self.codegen_loop(target_vector.size, |ctx, iterator_register| { + // Compute the modulus + ctx.binary_instruction( + shifted_field, + radix_as_field, + limb_field, + BrilligBinaryOp::Modulo, + ); + // Cast it + ctx.cast_instruction(limb_casted, limb_field); + // Write it + ctx.codegen_array_set(target_vector.pointer, iterator_register, limb_casted.address); + // Integer div the field + ctx.binary_instruction( + shifted_field, + radix_as_field, + shifted_field, + BrilligBinaryOp::UnsignedDiv, + ); + }); // Deallocate our temporary registers + self.deallocate_single_addr(shifted_field); self.deallocate_single_addr(limb_field); self.deallocate_single_addr(limb_casted); + self.deallocate_single_addr(radix_as_field); if big_endian { self.codegen_reverse_vector_in_place(target_vector); diff --git a/noir/noir-repo/compiler/noirc_evaluator/src/brillig/brillig_ir/debug_show.rs b/noir/noir-repo/compiler/noirc_evaluator/src/brillig/brillig_ir/debug_show.rs index f02f6059e7c..667ccf6ddbe 100644 --- a/noir/noir-repo/compiler/noirc_evaluator/src/brillig/brillig_ir/debug_show.rs +++ b/noir/noir-repo/compiler/noirc_evaluator/src/brillig/brillig_ir/debug_show.rs @@ -451,15 +451,6 @@ impl DebugShow { output ); } - BlackBoxOp::ToRadix { input, radix, output } => { - debug_println!( - self.enable_debug_trace, - " TO_RADIX {} {} -> {}", - input, - radix, - output - ); - } } } diff --git a/noir/noir-repo/noir_stdlib/src/field/bn254.nr b/noir/noir-repo/noir_stdlib/src/field/bn254.nr index 2e82d9e7c23..d70310be391 100644 --- a/noir/noir-repo/noir_stdlib/src/field/bn254.nr +++ b/noir/noir-repo/noir_stdlib/src/field/bn254.nr @@ -25,7 +25,7 @@ unconstrained fn decompose_unsafe(x: Field) -> (Field, Field) { fn assert_gt_limbs(a: (Field, Field), b: (Field, Field)) { let (alo, ahi) = a; let (blo, bhi) = b; - let borrow = lte_unsafe_16(alo, blo); + let borrow = lte_unsafe(alo, blo, 16); let rlo = alo - blo - 1 + (borrow as Field) * TWO_POW_128; let rhi = ahi - bhi - (borrow as Field); @@ -51,9 +51,9 @@ pub fn decompose(x: Field) -> (Field, Field) { (xlo, xhi) } -fn lt_unsafe_internal(x: Field, y: Field, num_bytes: u32) -> bool { - let x_bytes = x.to_le_radix(256, num_bytes); - let y_bytes = y.to_le_radix(256, num_bytes); +unconstrained fn lt_unsafe(x: Field, y: Field, num_bytes: u32) -> bool { + let x_bytes = x.__to_le_radix(256, num_bytes); + let y_bytes = y.__to_le_radix(256, num_bytes); let mut x_is_lt = false; let mut done = false; for i in 0..num_bytes { @@ -70,20 +70,8 @@ fn lt_unsafe_internal(x: Field, y: Field, num_bytes: u32) -> bool { x_is_lt } -fn lte_unsafe_internal(x: Field, y: Field, num_bytes: u32) -> bool { - if x == y { - true - } else { - lt_unsafe_internal(x, y, num_bytes) - } -} - -unconstrained fn lt_unsafe_32(x: Field, y: Field) -> bool { - lt_unsafe_internal(x, y, 32) -} - -unconstrained fn lte_unsafe_16(x: Field, y: Field) -> bool { - lte_unsafe_internal(x, y, 16) +unconstrained fn lte_unsafe(x: Field, y: Field, num_bytes: u32) -> bool { + lt_unsafe(x, y, num_bytes) | (x == y) } pub fn assert_gt(a: Field, b: Field) { @@ -102,7 +90,7 @@ pub fn assert_lt(a: Field, b: Field) { pub fn gt(a: Field, b: Field) -> bool { if a == b { false - } else if lt_unsafe_32(a, b) { + } else if lt_unsafe(a, b, 32) { assert_gt(b, a); false } else { @@ -117,10 +105,7 @@ pub fn lt(a: Field, b: Field) -> bool { mod tests { // TODO: Allow imports from "super" - use crate::field::bn254::{ - decompose_unsafe, decompose, lt_unsafe_internal, assert_gt, gt, lt, TWO_POW_128, - lte_unsafe_internal, PLO, PHI - }; + use crate::field::bn254::{decompose_unsafe, decompose, lt_unsafe, assert_gt, gt, lt, TWO_POW_128, lte_unsafe, PLO, PHI}; #[test] fn check_decompose_unsafe() { @@ -138,23 +123,23 @@ mod tests { #[test] fn check_lt_unsafe() { - assert(lt_unsafe_internal(0, 1, 16)); - assert(lt_unsafe_internal(0, 0x100, 16)); - assert(lt_unsafe_internal(0x100, TWO_POW_128 - 1, 16)); - assert(!lt_unsafe_internal(0, TWO_POW_128, 16)); + assert(lt_unsafe(0, 1, 16)); + assert(lt_unsafe(0, 0x100, 16)); + assert(lt_unsafe(0x100, TWO_POW_128 - 1, 16)); + assert(!lt_unsafe(0, TWO_POW_128, 16)); } #[test] fn check_lte_unsafe() { - assert(lte_unsafe_internal(0, 1, 16)); - assert(lte_unsafe_internal(0, 0x100, 16)); - assert(lte_unsafe_internal(0x100, TWO_POW_128 - 1, 16)); - assert(!lte_unsafe_internal(0, TWO_POW_128, 16)); - - assert(lte_unsafe_internal(0, 0, 16)); - assert(lte_unsafe_internal(0x100, 0x100, 16)); - assert(lte_unsafe_internal(TWO_POW_128 - 1, TWO_POW_128 - 1, 16)); - assert(lte_unsafe_internal(TWO_POW_128, TWO_POW_128, 16)); + assert(lte_unsafe(0, 1, 16)); + assert(lte_unsafe(0, 0x100, 16)); + assert(lte_unsafe(0x100, TWO_POW_128 - 1, 16)); + assert(!lte_unsafe(0, TWO_POW_128, 16)); + + assert(lte_unsafe(0, 0, 16)); + assert(lte_unsafe(0x100, 0x100, 16)); + assert(lte_unsafe(TWO_POW_128 - 1, TWO_POW_128 - 1, 16)); + assert(lte_unsafe(TWO_POW_128, TWO_POW_128, 16)); } #[test]