Skip to content

Commit

Permalink
4304 - additional review comments addressed
Browse files Browse the repository at this point in the history
  • Loading branch information
@jeanmon
jeanmon committed Feb 6, 2024
1 parent 357e51d commit 840750e
Show file tree
Hide file tree
Showing 5 changed files with 29 additions and 30 deletions.
28 changes: 14 additions & 14 deletions barretenberg/cpp/src/barretenberg/vm/avm_trace/AvmMini_alu_trace.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -47,11 +47,11 @@ std::vector<AvmMiniAluTraceBuilder::AluTraceEntry> AvmMiniAluTraceBuilder::final
*/
FF AvmMiniAluTraceBuilder::add(FF const& a, FF const& b, AvmMemoryTag in_tag, uint32_t const clk)
{
FF c{};
FF c = 0;
bool carry = false;
uint8_t alu_u8_r0{};
uint8_t alu_u8_r1{};
std::array<uint16_t, 8> alu_u16_reg{};
uint8_t alu_u8_r0 = 0;
uint8_t alu_u8_r1 = 0;
std::array<uint16_t, 8> alu_u16_reg;

uint128_t a_u128{ a };
uint128_t b_u128{ b };
Expand Down Expand Up @@ -136,11 +136,11 @@ FF AvmMiniAluTraceBuilder::add(FF const& a, FF const& b, AvmMemoryTag in_tag, ui
*/
FF AvmMiniAluTraceBuilder::sub(FF const& a, FF const& b, AvmMemoryTag in_tag, uint32_t const clk)
{
FF c{};
FF c = 0;
bool carry = false;
uint8_t alu_u8_r0{};
uint8_t alu_u8_r1{};
std::array<uint16_t, 8> alu_u16_reg{};
uint8_t alu_u8_r0 = 0;
uint8_t alu_u8_r1 = 0;
std::array<uint16_t, 8> alu_u16_reg;
uint128_t a_u128{ a };
uint128_t b_u128{ b };
uint128_t c_u128 = a_u128 - b_u128;
Expand Down Expand Up @@ -220,12 +220,12 @@ FF AvmMiniAluTraceBuilder::sub(FF const& a, FF const& b, AvmMemoryTag in_tag, ui
*/
FF AvmMiniAluTraceBuilder::mul(FF const& a, FF const& b, AvmMemoryTag in_tag, uint32_t const clk)
{
FF c{};
FF c = 0;
bool carry = false;
uint8_t alu_u8_r0{};
uint8_t alu_u8_r1{};
uint8_t alu_u8_r0 = 0;
uint8_t alu_u8_r1 = 0;

std::array<uint16_t, 8> alu_u16_reg{};
std::array<uint16_t, 8> alu_u16_reg;

uint128_t a_u128{ a };
uint128_t b_u128{ b };
Expand Down Expand Up @@ -258,8 +258,8 @@ FF AvmMiniAluTraceBuilder::mul(FF const& a, FF const& b, AvmMemoryTag in_tag, ui
uint128_t c_u128 = a_u128 * b_u128;

// Decompose a_u128 and b_u128 over 8 16-bit registers.
std::array<uint16_t, 8> alu_u16_reg_a{};
std::array<uint16_t, 8> alu_u16_reg_b{};
std::array<uint16_t, 8> alu_u16_reg_a;
std::array<uint16_t, 8> alu_u16_reg_b;
uint128_t a_trunc_128 = a_u128;
uint128_t b_trunc_128 = b_u128;

Expand Down
9 changes: 4 additions & 5 deletions barretenberg/cpp/src/barretenberg/vm/avm_trace/AvmMini_deserialization.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -110,7 +110,6 @@ std::vector<Instruction> Deserialization::parse(std::vector<uint8_t> const& byte
inst_format = { OperandType::TAG, OperandType::UINT128, OperandType::UINT32 };
break;
default: // This branch is guarded above.
inst_format = {};
std::cerr << "This code branch must have been guarded by the tag validation. \n";
assert(false);
}
Expand Down Expand Up @@ -140,28 +139,28 @@ std::vector<Instruction> Deserialization::parse(std::vector<uint8_t> const& byte
operands.emplace_back(bytecode.at(pos));
break;
case OperandType::UINT16: {
uint16_t operand_u16{};
uint16_t operand_u16 = 0;
uint8_t const* pos_ptr = &bytecode.at(pos);
serialize::read(pos_ptr, operand_u16);
operands.emplace_back(operand_u16);
break;
}
case OperandType::UINT32: {
uint32_t operand_u32{};
uint32_t operand_u32 = 0;
uint8_t const* pos_ptr = &bytecode.at(pos);
serialize::read(pos_ptr, operand_u32);
operands.emplace_back(operand_u32);
break;
}
case OperandType::UINT64: {
uint64_t operand_u64{};
uint64_t operand_u64 = 0;
uint8_t const* pos_ptr = &bytecode.at(pos);
serialize::read(pos_ptr, operand_u64);
operands.emplace_back(operand_u64);
break;
}
case OperandType::UINT128: {
uint128_t operand_u128{};
uint128_t operand_u128 = 0;
uint8_t const* pos_ptr = &bytecode.at(pos);
serialize::read(pos_ptr, operand_u128);
operands.emplace_back(operand_u128);
Expand Down
6 changes: 3 additions & 3 deletions barretenberg/cpp/src/barretenberg/vm/avm_trace/AvmMini_execution.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,7 @@ HonkProof Execution::run_and_prove(std::vector<uint8_t> const& bytecode, std::ve
*/
std::vector<Row> Execution::gen_trace(std::vector<Instruction> const& instructions, std::vector<FF> const& calldata)
{
AvmMiniTraceBuilder trace_builder{};
AvmMiniTraceBuilder trace_builder;

// Copied version of pc maintained in trace builder. The value of pc is evolving based
// on opcode logic and therefore is not maintained here. However, the next opcode in the execution
Expand Down Expand Up @@ -104,8 +104,8 @@ std::vector<Row> Execution::gen_trace(std::vector<Instruction> const& instructio
break;
// Machine State - Memory
case OpCode::SET: {
uint32_t dst_offset{};
uint128_t val{};
uint32_t dst_offset = 0;
uint128_t val = 0;
AvmMemoryTag in_tag = std::get<AvmMemoryTag>(inst.operands.at(0));
dst_offset = std::get<uint32_t>(inst.operands.at(2));

Expand Down
4 changes: 2 additions & 2 deletions barretenberg/cpp/src/barretenberg/vm/avm_trace/AvmMini_trace.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -377,7 +377,7 @@ std::vector<FF> AvmMiniTraceBuilder::return_op(uint32_t ret_offset, uint32_t ret
{
if (ret_size == 0) {
halt();
return std::vector<FF>{};
return {};
}

// We parallelize loading memory operations in chunk of 3, i.e., 1 per intermediate register.
Expand Down Expand Up @@ -604,7 +604,7 @@ std::vector<Row> AvmMiniTraceBuilder::finalize()
// Fill the rest with zeros.
size_t zero_rows_num = AVM_TRACE_SIZE - main_trace_size - 1;
while (zero_rows_num-- > 0) {
main_trace.push_back(Row{});
main_trace.push_back({});
}

main_trace.at(main_trace_size - 1).avmMini_last = FF(1);
Expand Down
12 changes: 6 additions & 6 deletions barretenberg/cpp/src/barretenberg/vm/tests/AvmMini_execution.test.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -84,9 +84,9 @@ TEST_F(AvmMiniExecutionTests, basicAddReturn)
EXPECT_EQ(std::get<uint32_t>(operands.at(0)), 0);
EXPECT_EQ(std::get<uint32_t>(operands.at(1)), 0);

auto trace = Execution::gen_trace(instructions, std::vector<FF>{});
auto trace = Execution::gen_trace(instructions);

gen_proof_and_validate(bytecode, std::move(trace), std::vector<FF>{});
gen_proof_and_validate(bytecode, std::move(trace), {});
}

// Positive test for SET and SUB opcodes
Expand Down Expand Up @@ -148,7 +148,7 @@ TEST_F(AvmMiniExecutionTests, setAndSubOpcodes)
auto row = std::ranges::find_if(trace.begin(), trace.end(), [](Row r) { return r.avmMini_sel_op_sub == 1; });
EXPECT_EQ(row->avmMini_ic, 10000); // 47123 - 37123 = 10000

gen_proof_and_validate(bytecode, std::move(trace), std::vector<FF>{});
gen_proof_and_validate(bytecode, std::move(trace), {});
}

// Positive test for multiple MUL opcodes
Expand Down Expand Up @@ -227,7 +227,7 @@ TEST_F(AvmMiniExecutionTests, powerWithMulOpcodes)
trace.begin(), trace.end(), [](Row r) { return r.avmMini_sel_op_mul == 1 && r.avmMini_pc == 13; });
EXPECT_EQ(row->avmMini_ic, 244140625); // 5^12 = 244140625

gen_proof_and_validate(bytecode, std::move(trace), std::vector<FF>{});
gen_proof_and_validate(bytecode, std::move(trace), {});
}

// Positive test about a single internal_call and internal_return
Expand Down Expand Up @@ -290,7 +290,7 @@ TEST_F(AvmMiniExecutionTests, simpleInternalCall)
auto row = std::ranges::find_if(trace.begin(), trace.end(), [](Row r) { return r.avmMini_sel_op_add == 1; });
EXPECT_EQ(row->avmMini_ic, 345567789);

gen_proof_and_validate(bytecode, std::move(trace), std::vector<FF>{});
gen_proof_and_validate(bytecode, std::move(trace), {});
}

// Positive test with some nested internall calls
Expand Down Expand Up @@ -370,7 +370,7 @@ TEST_F(AvmMiniExecutionTests, nestedInternalCalls)
EXPECT_EQ(row->avmMini_ic, 187);
EXPECT_EQ(row->avmMini_pc, 4);

gen_proof_and_validate(bytecode, std::move(trace), std::vector<FF>{});
gen_proof_and_validate(bytecode, std::move(trace), {});
}

// Positive test with JUMP and CALLDATACOPY
Expand Down

0 comments on commit 840750e

Please sign in to comment.