From 4ac13e642c958392ce5606684c044ea014325e26 Mon Sep 17 00:00:00 2001 From: Jean M <132435771+jeanmon@users.noreply.github.com> Date: Tue, 17 Dec 2024 22:45:48 +0100 Subject: [PATCH] chore(avm): radix opcode - remove immediates (#10696) Resolves #10371 --- avm-transpiler/src/transpile.rs | 19 +-- .../dsl/acir_format/serde/acir.hpp | 16 ++- .../vm/avm/tests/execution.test.cpp | 115 ++++++------------ .../vm/avm/trace/deserialization.cpp | 4 +- .../barretenberg/vm/avm/trace/execution.cpp | 4 +- .../src/barretenberg/vm/avm/trace/trace.cpp | 47 ++++--- .../src/barretenberg/vm/avm/trace/trace.hpp | 8 +- .../noir-repo/acvm-repo/acir/codegen/acir.cpp | 14 ++- .../acvm-repo/brillig/src/black_box.rs | 5 +- .../acvm-repo/brillig_vm/src/black_box.rs | 16 ++- .../brillig/brillig_ir/codegen_intrinsic.rs | 16 ++- .../src/brillig/brillig_ir/debug_show.rs | 7 +- .../src/avm/opcodes/conversion.test.ts | 88 +++++++++----- .../simulator/src/avm/opcodes/conversion.ts | 37 +++--- 14 files changed, 206 insertions(+), 190 deletions(-) diff --git a/avm-transpiler/src/transpile.rs b/avm-transpiler/src/transpile.rs index 79f4a3f02bb..d8b9c9509c9 100644 --- a/avm-transpiler/src/transpile.rs +++ b/avm-transpiler/src/transpile.rs @@ -1064,29 +1064,30 @@ fn handle_black_box_function(avm_instrs: &mut Vec, operation: &B ..Default::default() }); } - BlackBoxOp::ToRadix { input, radix, output, output_bits } => { - let num_limbs = output.size as u32; + BlackBoxOp::ToRadix { input, radix, output_pointer, num_limbs, output_bits } => { let input_offset = input.to_usize() as u32; - let output_offset = output.pointer.to_usize() as u32; let radix_offset = radix.to_usize() as u32; + let output_offset = output_pointer.to_usize() as u32; + let num_limbs_offset = num_limbs.to_usize() as u32; + let output_bits_offset = output_bits.to_usize() as u32; avm_instrs.push(AvmInstruction { opcode: AvmOpcode::TORADIXBE, indirect: Some( AddressingModeBuilder::default() .direct_operand(input) - .indirect_operand(&output.pointer) .direct_operand(radix) + .direct_operand(num_limbs) + .direct_operand(output_bits) + .indirect_operand(output_pointer) .build(), ), operands: vec![ AvmOperand::U16 { value: input_offset as u16 }, - AvmOperand::U16 { value: output_offset as u16 }, AvmOperand::U16 { value: radix_offset as u16 }, - ], - immediates: vec![ - AvmOperand::U16 { value: num_limbs as u16 }, - AvmOperand::U8 { value: *output_bits as u8 }, + AvmOperand::U16 { value: num_limbs_offset as u16 }, + AvmOperand::U16 { value: output_bits_offset as u16 }, + AvmOperand::U16 { value: output_offset as u16 }, ], ..Default::default() }); 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 fda8739c9fb..5d9d94d25e3 100644 --- a/barretenberg/cpp/src/barretenberg/dsl/acir_format/serde/acir.hpp +++ b/barretenberg/cpp/src/barretenberg/dsl/acir_format/serde/acir.hpp @@ -424,8 +424,9 @@ struct BlackBoxOp { struct ToRadix { Program::MemoryAddress input; Program::MemoryAddress radix; - Program::HeapArray output; - bool output_bits; + Program::MemoryAddress output_pointer; + Program::MemoryAddress num_limbs; + Program::MemoryAddress output_bits; friend bool operator==(const ToRadix&, const ToRadix&); std::vector bincodeSerialize() const; @@ -4611,7 +4612,10 @@ inline bool operator==(const BlackBoxOp::ToRadix& lhs, const BlackBoxOp::ToRadix if (!(lhs.radix == rhs.radix)) { return false; } - if (!(lhs.output == rhs.output)) { + if (!(lhs.output_pointer == rhs.output_pointer)) { + return false; + } + if (!(lhs.num_limbs == rhs.num_limbs)) { return false; } if (!(lhs.output_bits == rhs.output_bits)) { @@ -4646,7 +4650,8 @@ void serde::Serializable::serialize(const Program: { serde::Serializable::serialize(obj.input, serializer); serde::Serializable::serialize(obj.radix, serializer); - serde::Serializable::serialize(obj.output, serializer); + serde::Serializable::serialize(obj.output_pointer, serializer); + serde::Serializable::serialize(obj.num_limbs, serializer); serde::Serializable::serialize(obj.output_bits, serializer); } @@ -4658,7 +4663,8 @@ Program::BlackBoxOp::ToRadix serde::Deserializable Program::BlackBoxOp::ToRadix obj; obj.input = serde::Deserializable::deserialize(deserializer); obj.radix = serde::Deserializable::deserialize(deserializer); - obj.output = serde::Deserializable::deserialize(deserializer); + obj.output_pointer = serde::Deserializable::deserialize(deserializer); + obj.num_limbs = serde::Deserializable::deserialize(deserializer); obj.output_bits = serde::Deserializable::deserialize(deserializer); return obj; } diff --git a/barretenberg/cpp/src/barretenberg/vm/avm/tests/execution.test.cpp b/barretenberg/cpp/src/barretenberg/vm/avm/tests/execution.test.cpp index 89946de9b53..86b62b8a64e 100644 --- a/barretenberg/cpp/src/barretenberg/vm/avm/tests/execution.test.cpp +++ b/barretenberg/cpp/src/barretenberg/vm/avm/tests/execution.test.cpp @@ -130,6 +130,8 @@ class AvmExecutionTests : public ::testing::Test { } }; +class AvmExecutionTestsToRadix : public AvmExecutionTests, public testing::WithParamInterface {}; + // Basic positive test with an ADD and RETURN opcode. // Parsing, trace generation and proving is verified. TEST_F(AvmExecutionTests, basicAddReturn) @@ -854,85 +856,15 @@ TEST_F(AvmExecutionTests, setAndCastOpcodes) validate_trace(std::move(trace), public_inputs); } -// Positive test with TO_RADIX_BE. -TEST_F(AvmExecutionTests, toRadixBeOpcodeBytes) -{ - std::string bytecode_hex = - to_hex(OpCode::SET_8) + // opcode SET - "00" // Indirect flag - "00" // dst_offset - + to_hex(AvmMemoryTag::U32) + "00" // val - + to_hex(OpCode::SET_8) + // opcode SET - "00" // Indirect flag - "01" // dst_offset - + to_hex(AvmMemoryTag::U32) + "01" // val - + to_hex(OpCode::CALLDATACOPY) + // opcode CALLDATACOPY - "00" // Indirect flag - "0000" // cd_offset - "0001" // copy_size - "0001" // dst_offset - + to_hex(OpCode::SET_8) + // opcode SET for indirect src - "00" // Indirect flag - "11" // dst_offset 17 - + to_hex(AvmMemoryTag::U32) + "01" // value 1 (i.e. where the src from calldata is copied) - + to_hex(OpCode::SET_8) + // opcode SET for indirect dst - "00" // Indirect flag - "15" // dst_offset 21 - + to_hex(AvmMemoryTag::U32) + "05" // value 5 (i.e. where the dst will be written to) - + to_hex(OpCode::SET_8) + // opcode SET for indirect dst - "00" // Indirect flag - "80" // radix_offset 80 - + to_hex(AvmMemoryTag::U32) + "02" // value 2 (i.e. radix 2 - perform bitwise decomposition) - + to_hex(OpCode::TORADIXBE) + // opcode TO_RADIX_BE - "03" // Indirect flag - "0011" // src_offset 17 (indirect) - "0015" // dst_offset 21 (indirect) - "0080" // radix_offset 80 (direct) - "0100" // limbs: 256 - "00" // output_bits: false - + to_hex(OpCode::SET_16) + // opcode SET (for return size) - "00" // Indirect flag - "0200" // dst_offset=512 - + to_hex(AvmMemoryTag::U32) + // - "0100" // val: 256 - + to_hex(OpCode::RETURN) + // opcode RETURN - "00" // Indirect flag - "0005" // ret offset 5 - "0200"; // ret size offset 512 - - auto bytecode = hex_to_bytes(bytecode_hex); - auto [instructions, error] = Deserialization::parse_bytecode_statically(bytecode); - ASSERT_TRUE(is_ok(error)); - - // Assign a vector that we will mutate internally in gen_trace to store the return values; - std::vector returndata; - ExecutionHints execution_hints; - auto trace = - gen_trace(bytecode, std::vector{ FF::modulus - FF(1) }, public_inputs, returndata, execution_hints); - - // Find the first row enabling the TORADIXBE selector - // Expected output is bitwise decomposition of MODULUS - 1..could hardcode the result but it's a bit long - size_t num_limbs = 256; - std::vector expected_output(num_limbs); - // Extract each bit. - for (size_t i = 0; i < num_limbs; i++) { - auto byte_index = num_limbs - i - 1; - FF expected_limb = (FF::modulus - 1) >> i & 1; - expected_output[byte_index] = expected_limb; - } - EXPECT_EQ(returndata, expected_output); - - validate_trace(std::move(trace), public_inputs, { FF::modulus - FF(1) }, returndata); -} - -// Positive test with TO_RADIX_BE. -TEST_F(AvmExecutionTests, toRadixBeOpcodeBitsMode) +namespace { +std::vector gen_bytecode_radix(bool is_bit_mode) { + const std::string bit_mode_hex = is_bit_mode ? "01" : "00"; std::string bytecode_hex = to_hex(OpCode::SET_8) + // opcode SET "00" // Indirect flag "00" // dst_offset - + to_hex(AvmMemoryTag::U32) // - + "00" // val + + to_hex(AvmMemoryTag::U32) + // + "00" // val + to_hex(OpCode::SET_8) + // opcode SET "00" // Indirect flag "01" // dst_offset @@ -953,18 +885,28 @@ TEST_F(AvmExecutionTests, toRadixBeOpcodeBitsMode) "15" // dst_offset 21 + to_hex(AvmMemoryTag::U32) + // "05" // value 5 (i.e. where the dst will be written to) - + to_hex(OpCode::SET_8) + // opcode SET for indirect dst + + to_hex(OpCode::SET_8) + // opcode SET (direct radix_offset) "00" // Indirect flag "80" // radix_offset 80 + to_hex(AvmMemoryTag::U32) + // "02" // value 2 (i.e. radix 2 - perform bitwise decomposition) + + to_hex(OpCode::SET_16) + // opcode SET (direct number_limbs_offset) + "00" // Indirect flag + "0090" // number_limbs_offset 0x90 + + to_hex(AvmMemoryTag::U32) + // + "0100" // value 256 + + to_hex(OpCode::SET_8) + // opcode SET (direct output_bits_offset) + "00" // Indirect flag + "95" // output_bits_offset 0x95 + + to_hex(AvmMemoryTag::U1) + // + bit_mode_hex // bit 1 (true for bits mode) + to_hex(OpCode::TORADIXBE) + // opcode TO_RADIX_BE - "03" // Indirect flag + "0011" // Indirect flag "0011" // src_offset 17 (indirect) - "0015" // dst_offset 21 (indirect) "0080" // radix_offset 80 (direct) - "0100" // limbs: 256 - "01" // output_bits: true + "0090" // num_limbs_offset (direct) + "0095" // output_bits_offset (direct) + "0015" // dst_offset 21 (indirect) + to_hex(OpCode::SET_16) + // opcode SET (for return size) "00" // Indirect flag "0200" // dst_offset=512 @@ -975,7 +917,15 @@ TEST_F(AvmExecutionTests, toRadixBeOpcodeBitsMode) "0005" // ret offset 5 "0200"; // ret size offset 512 - auto bytecode = hex_to_bytes(bytecode_hex); + return hex_to_bytes(bytecode_hex); +} +} // namespace + +// Positive test for TORADIXBE opcode parametrized by a boolean toggling bit vs bytes mode. +TEST_P(AvmExecutionTestsToRadix, ParamTest) +{ + const bool is_bit_mode = GetParam(); + auto bytecode = gen_bytecode_radix(is_bit_mode); auto [instructions, error] = Deserialization::parse_bytecode_statically(bytecode); ASSERT_TRUE(is_ok(error)); @@ -1000,6 +950,9 @@ TEST_F(AvmExecutionTests, toRadixBeOpcodeBitsMode) validate_trace(std::move(trace), public_inputs, { FF::modulus - FF(1) }, returndata); } +// Run the test for TORADIXBE in bit mode and then in bytes mode. +INSTANTIATE_TEST_SUITE_P(AvmExecutionTests, AvmExecutionTestsToRadix, testing::ValuesIn({ true, false })); + // // Positive test with SHA256COMPRESSION. TEST_F(AvmExecutionTests, sha256CompressionOpcode) { diff --git a/barretenberg/cpp/src/barretenberg/vm/avm/trace/deserialization.cpp b/barretenberg/cpp/src/barretenberg/vm/avm/trace/deserialization.cpp index ba6a60397ae..2478ba93dd3 100644 --- a/barretenberg/cpp/src/barretenberg/vm/avm/trace/deserialization.cpp +++ b/barretenberg/cpp/src/barretenberg/vm/avm/trace/deserialization.cpp @@ -179,12 +179,12 @@ const std::unordered_map> OPCODE_WIRE_FORMAT = { OperandType::INDIRECT8, OperandType::UINT16, OperandType::UINT16, OperandType::UINT16, OperandType::UINT16 } }, // Gadget - Conversion { OpCode::TORADIXBE, - { OperandType::INDIRECT8, + { OperandType::INDIRECT16, OperandType::UINT16, OperandType::UINT16, OperandType::UINT16, OperandType::UINT16, - OperandType::UINT8 } }, + OperandType::UINT16 } }, }; const std::unordered_map OPERAND_TYPE_SIZE = { diff --git a/barretenberg/cpp/src/barretenberg/vm/avm/trace/execution.cpp b/barretenberg/cpp/src/barretenberg/vm/avm/trace/execution.cpp index 6b8f7ae0b0e..ba9b71cef20 100644 --- a/barretenberg/cpp/src/barretenberg/vm/avm/trace/execution.cpp +++ b/barretenberg/cpp/src/barretenberg/vm/avm/trace/execution.cpp @@ -974,12 +974,12 @@ AvmError Execution::execute_enqueued_call(AvmTraceBuilder& trace_builder, // Conversions case OpCode::TORADIXBE: - error = trace_builder.op_to_radix_be(std::get(inst.operands.at(0)), + error = trace_builder.op_to_radix_be(std::get(inst.operands.at(0)), std::get(inst.operands.at(1)), std::get(inst.operands.at(2)), std::get(inst.operands.at(3)), std::get(inst.operands.at(4)), - std::get(inst.operands.at(5))); + std::get(inst.operands.at(5))); break; default: diff --git a/barretenberg/cpp/src/barretenberg/vm/avm/trace/trace.cpp b/barretenberg/cpp/src/barretenberg/vm/avm/trace/trace.cpp index 3874a9cab8c..8aadc08d280 100644 --- a/barretenberg/cpp/src/barretenberg/vm/avm/trace/trace.cpp +++ b/barretenberg/cpp/src/barretenberg/vm/avm/trace/trace.cpp @@ -4429,35 +4429,50 @@ AvmError AvmTraceBuilder::op_variable_msm(uint8_t indirect, * * @param indirect A byte encoding information about indirect/direct memory access. * @param src_offset An index in memory pointing to the input of the To_Radix_BE conversion. - * @param dst_offset An index in memory pointing to the output of the To_Radix_BE conversion. * @param radix_offset An index in memory pointing to the strict upper bound of each converted limb, i.e., 0 <= limb * < radix. - * @param num_limbs The number of limbs to the value into. - * @param output_bits Should the output be U1s instead of U8s? + * @param num_limbs_offset Offset pointing to the number of limbs to the value into. + * @param output_bits_offset Offset pointing to a boolean telling whether bits (U1) or bytes (U8) are in the output + * @param dst_offset An index in memory pointing to the output of the To_Radix_BE conversion. */ -AvmError AvmTraceBuilder::op_to_radix_be(uint8_t indirect, +AvmError AvmTraceBuilder::op_to_radix_be(uint16_t indirect, uint32_t src_offset, - uint32_t dst_offset, uint32_t radix_offset, - uint32_t num_limbs, - uint8_t output_bits) + uint32_t num_limbs_offset, + uint32_t output_bits_offset, + uint32_t dst_offset) { // We keep the first encountered error AvmError error = AvmError::NO_ERROR; auto clk = static_cast(main_trace.size()) + 1; - // write output as bits or bytes - AvmMemoryTag w_in_tag = output_bits > 0 ? AvmMemoryTag::U1 // bits mode - : AvmMemoryTag::U8; - - auto [resolved_addrs, res_error] = Addressing<3>::fromWire(indirect, call_ptr) - .resolve({ src_offset, dst_offset, radix_offset }, mem_trace_builder); - auto [resolved_src_offset, resolved_dst_offset, resolved_radix_offset] = resolved_addrs; + auto [resolved_addrs, res_error] = + Addressing<5>::fromWire(indirect, call_ptr) + .resolve({ src_offset, radix_offset, num_limbs_offset, output_bits_offset, dst_offset }, mem_trace_builder); + auto [resolved_src_offset, + resolved_radix_offset, + resolved_num_limbs_offset, + resolved_output_bits_offset, + resolved_dst_offset] = resolved_addrs; error = res_error; + if (is_ok(error) && !check_tag(AvmMemoryTag::U32, resolved_radix_offset) && + !check_tag(AvmMemoryTag::U32, resolved_num_limbs_offset) && + !check_tag(AvmMemoryTag::U1, resolved_output_bits_offset)) { + error = AvmError::CHECK_TAG_ERROR; + } + + const auto num_limbs = static_cast(unconstrained_read_from_memory(resolved_num_limbs_offset)); + // Constrain gas cost gas_trace_builder.constrain_gas(clk, OpCode::TORADIXBE, num_limbs); + const auto output_bits = static_cast(unconstrained_read_from_memory(resolved_output_bits_offset)); + + // write output as bits or bytes + AvmMemoryTag w_in_tag = output_bits > 0 ? AvmMemoryTag::U1 // bits mode + : AvmMemoryTag::U8; + auto read_src = constrained_read_from_memory( call_ptr, clk, resolved_src_offset, AvmMemoryTag::FF, w_in_tag, IntermRegister::IA); // TODO(8603): once instructions can have multiple different tags for reads, constrain the radix's read @@ -4465,10 +4480,6 @@ AvmError AvmTraceBuilder::op_to_radix_be(uint8_t indirect, // auto read_radix = constrained_read_from_memory( // call_ptr, clk, resolved_radix_offset, AvmMemoryTag::U32, AvmMemoryTag::U32, IntermRegister::IB); - if (is_ok(error) && !check_tag(AvmMemoryTag::U32, resolved_radix_offset)) { - error = AvmError::CHECK_TAG_ERROR; - } - auto read_radix = unconstrained_read_from_memory(resolved_radix_offset); FF input = read_src.val; diff --git a/barretenberg/cpp/src/barretenberg/vm/avm/trace/trace.hpp b/barretenberg/cpp/src/barretenberg/vm/avm/trace/trace.hpp index b30e7d2c104..840de69fb1b 100644 --- a/barretenberg/cpp/src/barretenberg/vm/avm/trace/trace.hpp +++ b/barretenberg/cpp/src/barretenberg/vm/avm/trace/trace.hpp @@ -220,12 +220,12 @@ class AvmTraceBuilder { uint32_t output_offset, uint32_t point_length_offset); // Conversions - AvmError op_to_radix_be(uint8_t indirect, + AvmError op_to_radix_be(uint16_t indirect, uint32_t src_offset, - uint32_t dst_offset, uint32_t radix_offset, - uint32_t num_limbs, - uint8_t output_bits); + uint32_t num_limbs_offset, + uint32_t output_bits_offset, + uint32_t dst_offset); std::vector finalize(bool apply_end_gas_assertions = false); void reset(); diff --git a/noir/noir-repo/acvm-repo/acir/codegen/acir.cpp b/noir/noir-repo/acvm-repo/acir/codegen/acir.cpp index e94f36535d2..2ae15f9f5a3 100644 --- a/noir/noir-repo/acvm-repo/acir/codegen/acir.cpp +++ b/noir/noir-repo/acvm-repo/acir/codegen/acir.cpp @@ -424,8 +424,9 @@ namespace Program { struct ToRadix { Program::MemoryAddress input; Program::MemoryAddress radix; - Program::HeapArray output; - bool output_bits; + Program::MemoryAddress output_pointer; + Program::MemoryAddress num_limbs; + Program::MemoryAddress output_bits; friend bool operator==(const ToRadix&, const ToRadix&); std::vector bincodeSerialize() const; @@ -3898,7 +3899,8 @@ namespace Program { inline bool operator==(const BlackBoxOp::ToRadix &lhs, const BlackBoxOp::ToRadix &rhs) { if (!(lhs.input == rhs.input)) { return false; } if (!(lhs.radix == rhs.radix)) { return false; } - if (!(lhs.output == rhs.output)) { return false; } + if (!(lhs.output_pointer == rhs.output_pointer)) { return false; } + if (!(lhs.num_limbs == rhs.num_limbs)) { return false; } if (!(lhs.output_bits == rhs.output_bits)) { return false; } return true; } @@ -3925,7 +3927,8 @@ 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); + serde::Serializable::serialize(obj.output_pointer, serializer); + serde::Serializable::serialize(obj.num_limbs, serializer); serde::Serializable::serialize(obj.output_bits, serializer); } @@ -3935,7 +3938,8 @@ Program::BlackBoxOp::ToRadix serde::Deserializable Program::BlackBoxOp::ToRadix obj; obj.input = serde::Deserializable::deserialize(deserializer); obj.radix = serde::Deserializable::deserialize(deserializer); - obj.output = serde::Deserializable::deserialize(deserializer); + obj.output_pointer = serde::Deserializable::deserialize(deserializer); + obj.num_limbs = serde::Deserializable::deserialize(deserializer); obj.output_bits = serde::Deserializable::deserialize(deserializer); return obj; } 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 f185b36e6c8..be9ba20ed49 100644 --- a/noir/noir-repo/acvm-repo/brillig/src/black_box.rs +++ b/noir/noir-repo/acvm-repo/brillig/src/black_box.rs @@ -102,7 +102,8 @@ pub enum BlackBoxOp { ToRadix { input: MemoryAddress, radix: MemoryAddress, - output: HeapArray, - output_bits: bool, + output_pointer: MemoryAddress, + num_limbs: MemoryAddress, + output_bits: MemoryAddress, }, } 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 79aea2adf76..9ebbbd3f087 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 @@ -310,21 +310,27 @@ pub(crate) fn evaluate_black_box memory.write_slice(memory.read_ref(output.pointer), &state); Ok(()) } - BlackBoxOp::ToRadix { input, radix, output, output_bits } => { + 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 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 mut input = BigUint::from_bytes_be(&input.to_be_bytes()); let radix = BigUint::from_bytes_be(&radix.to_be_bytes()); - let mut limbs: Vec> = vec![MemoryValue::default(); output.size]; + let mut limbs: Vec> = vec![MemoryValue::default(); num_limbs]; - for i in (0..output.size).rev() { + for i in (0..num_limbs).rev() { let limb = &input % &radix; - if *output_bits { + if output_bits { limbs[i] = MemoryValue::new_integer( if limb.is_zero() { 0 } else { 1 }, IntegerBitSize::U1, @@ -336,7 +342,7 @@ pub(crate) fn evaluate_black_box input /= &radix; } - memory.write_slice(memory.read_ref(output.pointer), &limbs); + memory.write_slice(memory.read_ref(*output_pointer), &limbs); Ok(()) } 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 ba89823ef13..42c13a3c235 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 @@ -75,23 +75,27 @@ impl BrilligContext< assert!(source_field.bit_size == F::max_num_bits()); assert!(radix.bit_size == 32); + let bits_register = self.make_constant_instruction(output_bits.into(), 1); self.codegen_initialize_array(target_array); - - let heap_array = self.codegen_brillig_array_to_heap_array(target_array); + let pointer = self.codegen_make_array_items_pointer(target_array); + let num_limbs = self.make_usize_constant_instruction(target_array.size.into()); // Perform big-endian ToRadix self.black_box_op_instruction(BlackBoxOp::ToRadix { input: source_field.address, radix: radix.address, - output: heap_array, - output_bits, + output_pointer: pointer, + num_limbs: num_limbs.address, + output_bits: bits_register.address, }); if little_endian { let items_len = self.make_usize_constant_instruction(target_array.size.into()); - self.codegen_array_reverse(heap_array.pointer, items_len.address); + self.codegen_array_reverse(pointer, items_len.address); self.deallocate_single_addr(items_len); } - self.deallocate_register(heap_array.pointer); + self.deallocate_register(pointer); + self.deallocate_single_addr(bits_register); + self.deallocate_single_addr(num_limbs); } } 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 ef1b5432128..283c0d67eb8 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 @@ -397,13 +397,14 @@ impl DebugShow { output ); } - BlackBoxOp::ToRadix { input, radix, output, output_bits: _ } => { + BlackBoxOp::ToRadix { input, radix, output_pointer, num_limbs, output_bits: _ } => { debug_println!( self.enable_debug_trace, - " TO_RADIX {} {} -> {}", + " TO_RADIX {} {} {} -> {}", input, radix, - output + num_limbs, + output_pointer ); } } diff --git a/yarn-project/simulator/src/avm/opcodes/conversion.test.ts b/yarn-project/simulator/src/avm/opcodes/conversion.test.ts index 8db134f6d09..873ab29db5b 100644 --- a/yarn-project/simulator/src/avm/opcodes/conversion.test.ts +++ b/yarn-project/simulator/src/avm/opcodes/conversion.test.ts @@ -1,5 +1,5 @@ import { type AvmContext } from '../avm_context.js'; -import { Field, type Uint1, type Uint8, Uint32 } from '../avm_memory_types.js'; +import { Field, Uint1, type Uint8, Uint32 } from '../avm_memory_types.js'; import { initContext } from '../fixtures/index.js'; import { Addressing, AddressingMode } from './addressing_mode.js'; import { ToRadixBE } from './conversion.js'; @@ -15,20 +15,20 @@ describe('Conversion Opcodes', () => { it('Should (de)serialize correctly', () => { const buf = Buffer.from([ ToRadixBE.opcode, // opcode - 1, // indirect + ...Buffer.from('0001', 'hex'), // indirect ...Buffer.from('1234', 'hex'), // inputStateOffset - ...Buffer.from('2345', 'hex'), // outputStateOffset - ...Buffer.from('3456', 'hex'), // radixOffset - ...Buffer.from('0100', 'hex'), // numLimbs - ...Buffer.from('01', 'hex'), // outputBits + ...Buffer.from('2345', 'hex'), // radixOffset + ...Buffer.from('3456', 'hex'), // numLimbsOffset + ...Buffer.from('4567', 'hex'), // outputBitsOffset + ...Buffer.from('5678', 'hex'), // outputStateOffset ]); const inst = new ToRadixBE( - /*indirect=*/ 1, + /*indirect=*/ 0x0001, /*srcOffset=*/ 0x1234, - /*dstOffset=*/ 0x2345, - /*radixOffset=*/ 0x3456, - /*numLimbs=*/ 256, - /*outputBits=*/ 1, + /*radixOffset=*/ 0x2345, + /*numLimbsOffset=*/ 0x3456, + /*outputBitsOffset=*/ 0x4567, + /*dstOffset=*/ 0x5678, ); expect(ToRadixBE.deserialize(buf)).toEqual(inst); @@ -42,21 +42,27 @@ describe('Conversion Opcodes', () => { const srcOffset = 0; const dstOffset = 20; const radixOffset = 1; - const numLimbs = 10; // only the first 10 bits - const outputBits = 0; // false, output as bytes + const numLimbs = new Uint32(10); // only the first 10 bits + const numLimbsOffset = 100; + const outputBits = new Uint1(0); // false, output as bytes + const outputBitsOffset = 200; context.machineState.memory.set(srcOffset, arg); context.machineState.memory.set(radixOffset, radix); + context.machineState.memory.set(numLimbsOffset, numLimbs); + context.machineState.memory.set(outputBitsOffset, outputBits); - await new ToRadixBE(indirect, srcOffset, dstOffset, radixOffset, numLimbs, outputBits).execute(context); + await new ToRadixBE(indirect, srcOffset, radixOffset, numLimbsOffset, outputBitsOffset, dstOffset).execute( + context, + ); const resultBuffer: Buffer = Buffer.concat( - context.machineState.memory.getSliceAs(dstOffset, numLimbs).map(byte => byte.toBuffer()), + context.machineState.memory.getSliceAs(dstOffset, numLimbs.toNumber()).map(byte => byte.toBuffer()), ); // The expected result is the first 10 bits of the input // Reverse before slice because still only care about the first `numLimb` bytes. // Then reverse back since we want big endian (as the original string is). - const expectedResults = '1011101010100'.split('').reverse().slice(0, numLimbs).reverse().map(Number); - for (let i = 0; i < numLimbs; i++) { + const expectedResults = '1011101010100'.split('').reverse().slice(0, numLimbs.toNumber()).reverse().map(Number); + for (let i = 0; i < numLimbs.toNumber(); i++) { expect(resultBuffer.readUInt8(i)).toEqual(expectedResults[i]); } }); @@ -68,21 +74,27 @@ describe('Conversion Opcodes', () => { const srcOffset = 0; const dstOffset = 20; const radixOffset = 1; - const numLimbs = 10; // only the first 10 bits - const outputBits = 1; // true, output as bits + const numLimbs = new Uint32(10); // only the first 10 bits + const numLimbsOffset = 100; + const outputBits = new Uint1(1); // true, output as bits + const outputBitsOffset = 200; context.machineState.memory.set(srcOffset, arg); context.machineState.memory.set(radixOffset, radix); + context.machineState.memory.set(numLimbsOffset, numLimbs); + context.machineState.memory.set(outputBitsOffset, outputBits); - await new ToRadixBE(indirect, srcOffset, dstOffset, radixOffset, numLimbs, outputBits).execute(context); + await new ToRadixBE(indirect, srcOffset, radixOffset, numLimbsOffset, outputBitsOffset, dstOffset).execute( + context, + ); const resultBuffer: Buffer = Buffer.concat( - context.machineState.memory.getSliceAs(dstOffset, numLimbs).map(byte => byte.toBuffer()), + context.machineState.memory.getSliceAs(dstOffset, numLimbs.toNumber()).map(byte => byte.toBuffer()), ); // The expected result is the first 10 bits of the input // Reverse before slice because still only care about the first `numLimb` bytes. // Then reverse back since we want big endian (as the original string is). - const expectedResults = '1011101010100'.split('').reverse().slice(0, numLimbs).reverse().map(Number); - for (let i = 0; i < numLimbs; i++) { + const expectedResults = '1011101010100'.split('').reverse().slice(0, numLimbs.toNumber()).reverse().map(Number); + for (let i = 0; i < numLimbs.toNumber(); i++) { expect(resultBuffer.readUInt8(i)).toEqual(expectedResults[i]); } }); @@ -91,29 +103,41 @@ describe('Conversion Opcodes', () => { const arg = new Field(Buffer.from('1234567890abcdef', 'hex')); const indirect = new Addressing([ /*srcOffset=*/ AddressingMode.INDIRECT, - /*dstOffset*/ AddressingMode.INDIRECT, /*radixOffset*/ AddressingMode.INDIRECT, + /*numLimbsOffset*/ AddressingMode.INDIRECT, + /*outputBitsOffset*/ AddressingMode.INDIRECT, + /*dstOffset*/ AddressingMode.INDIRECT, ]).toWire(); const srcOffset = 0; - const srcOffsetReal = 10; + const srcOffsetReal = 1000; const dstOffset = 2; - const dstOffsetReal = 30; + const dstOffsetReal = 2000; const radixOffset = 3; const radix = new Uint32(1 << 8); // Byte decomposition - const radixOffsetReal = 50; + const radixOffsetReal = 3000; + const numLimbsOffset = 4; + const numLimbsOffsetReal = 4000; + const numLimbs = new Uint32(32); // 256-bit decomposition + const outputBitsOffset = 5; + const outputBitsOffsetReal = 5000; + const outputBits = new Uint1(0); // false, output as bytes context.machineState.memory.set(srcOffset, new Uint32(srcOffsetReal)); context.machineState.memory.set(dstOffset, new Uint32(dstOffsetReal)); context.machineState.memory.set(radixOffset, new Uint32(radixOffsetReal)); + context.machineState.memory.set(numLimbsOffset, new Uint32(numLimbsOffsetReal)); + context.machineState.memory.set(outputBitsOffset, new Uint32(outputBitsOffsetReal)); context.machineState.memory.set(srcOffsetReal, arg); context.machineState.memory.set(radixOffsetReal, radix); + context.machineState.memory.set(numLimbsOffsetReal, numLimbs); + context.machineState.memory.set(outputBitsOffsetReal, outputBits); - const numLimbs = 32; // 256-bit decomposition - const outputBits = 0; // false, output as bytes - await new ToRadixBE(indirect, srcOffset, dstOffset, radixOffset, numLimbs, outputBits).execute(context); + await new ToRadixBE(indirect, srcOffset, radixOffset, numLimbsOffset, outputBitsOffset, dstOffset).execute( + context, + ); const resultBuffer: Buffer = Buffer.concat( - context.machineState.memory.getSliceAs(dstOffsetReal, numLimbs).map(byte => byte.toBuffer()), + context.machineState.memory.getSliceAs(dstOffsetReal, numLimbs.toNumber()).map(byte => byte.toBuffer()), ); // The expected result is the input (padded to 256 bits) const expectedResults = '1234567890abcdef' @@ -121,7 +145,7 @@ describe('Conversion Opcodes', () => { .split('') .map(a => parseInt(a, 16)); // Checking the value in each byte of the buffer is correct - for (let i = 0; i < numLimbs; i++) { + for (let i = 0; i < numLimbs.toNumber(); i++) { // Compute the expected byte's numerical value from its two hex digits expect(resultBuffer.readUInt8(i)).toEqual(expectedResults[2 * i] * 16 + expectedResults[2 * i + 1]); } diff --git a/yarn-project/simulator/src/avm/opcodes/conversion.ts b/yarn-project/simulator/src/avm/opcodes/conversion.ts index f7a954d5e6c..3699c6f75b1 100644 --- a/yarn-project/simulator/src/avm/opcodes/conversion.ts +++ b/yarn-project/simulator/src/avm/opcodes/conversion.ts @@ -12,57 +12,62 @@ export class ToRadixBE extends Instruction { // Informs (de)serialization. See Instruction.deserialize. static readonly wireFormat: OperandType[] = [ OperandType.UINT8, // Opcode - OperandType.UINT8, // Indirect + OperandType.UINT16, // Indirect OperandType.UINT16, // src memory address - OperandType.UINT16, // dst memory address OperandType.UINT16, // radix memory address - OperandType.UINT16, // number of limbs (Immediate) - OperandType.UINT8, // output is in "bits" mode (Immediate - Uint1 still takes up a whole byte) + OperandType.UINT16, // number of limbs address + OperandType.UINT16, // output is in "bits" mode memory address (boolean/Uint1 is stored) + OperandType.UINT16, // dst memory address ]; constructor( private indirect: number, private srcOffset: number, - private dstOffset: number, private radixOffset: number, - private numLimbs: number, - private outputBits: number, // effectively a bool + private numLimbsOffset: number, + private outputBitsOffset: number, + private dstOffset: number, ) { super(); } public async execute(context: AvmContext): Promise { const memory = context.machineState.memory.track(this.type); - const operands = [this.srcOffset, this.dstOffset, this.radixOffset]; + const operands = [this.srcOffset, this.radixOffset, this.numLimbsOffset, this.outputBitsOffset, this.dstOffset]; const addressing = Addressing.fromWire(this.indirect, operands.length); - const [srcOffset, dstOffset, radixOffset] = addressing.resolve(operands, memory); - context.machineState.consumeGas(this.gasCost(this.numLimbs)); + const [srcOffset, radixOffset, numLimbsOffset, outputBitsOffset, dstOffset] = addressing.resolve(operands, memory); // The radix gadget only takes in a Field memory.checkTag(TypeTag.FIELD, srcOffset); memory.checkTag(TypeTag.UINT32, radixOffset); + memory.checkTag(TypeTag.UINT32, numLimbsOffset); + memory.checkTag(TypeTag.UINT1, outputBitsOffset); + + const numLimbs = memory.get(numLimbsOffset).toNumber(); + context.machineState.consumeGas(this.gasCost(numLimbs)); + const outputBits = memory.get(outputBitsOffset).toNumber(); let value: bigint = memory.get(srcOffset).toBigInt(); const radix: bigint = memory.get(radixOffset).toBigInt(); - if (this.numLimbs < 1) { - throw new InstructionExecutionError(`ToRadixBE instruction's numLimbs should be > 0 (was ${this.numLimbs})`); + if (numLimbs < 1) { + throw new InstructionExecutionError(`ToRadixBE instruction's numLimbs should be > 0 (was ${numLimbs})`); } if (radix > 256) { throw new InstructionExecutionError(`ToRadixBE instruction's radix should be <= 256 (was ${radix})`); } const radixBN: bigint = BigInt(radix); - const limbArray = new Array(this.numLimbs); + const limbArray = new Array(numLimbs); - for (let i = this.numLimbs - 1; i >= 0; i--) { + for (let i = numLimbs - 1; i >= 0; i--) { const limb = value % radixBN; limbArray[i] = limb; value /= radixBN; } - const outputType = this.outputBits != 0 ? Uint1 : Uint8; + const outputType = outputBits != 0 ? Uint1 : Uint8; const res = limbArray.map(byte => new outputType(byte)); memory.setSlice(dstOffset, res); - memory.assert({ reads: 2, writes: this.numLimbs, addressing }); + memory.assert({ reads: 4, writes: numLimbs, addressing }); } }