Skip to content

Commit

Permalink
Remove next references in lookups / permutations (#2141)
Browse files Browse the repository at this point in the history
Currently, when we run the Goldilocks RISC-V machine with `--linker-mode
bus` results in:

`Double application of "'" on: main_binary::operation_id` because of
#2140. This PR hot-fixes it, so we can run benchmarks using the bus.
Only adds 3 witness columns (1 in binary, 2 in shift).
  • Loading branch information
georgwiese authored and leonardoalt committed Nov 26, 2024
1 parent 6ea63a7 commit bfaeff4
Show file tree
Hide file tree
Showing 4 changed files with 34 additions and 4 deletions.
15 changes: 15 additions & 0 deletions riscv-executor/src/submachines.rs
Original file line number Diff line number Diff line change
Expand Up @@ -266,15 +266,18 @@ impl SubmachineKind for BinaryMachine {
trace.set_current_row("A_byte", (a1 as u32).into());
trace.set_current_row("B_byte", (b1 as u32).into());
trace.set_current_row("C_byte", (c1 as u32).into());
trace.set_current_row("operation_id_next", op_id);
} else {
trace.set_final_row("A_byte", (a1 as u32).into());
trace.set_final_row("B_byte", (b1 as u32).into());
trace.set_final_row("C_byte", (c1 as u32).into());
trace.set_final_row("operation_id_next", op_id);
}

// 4 rows for each binary operation
trace.push_row();
trace.set_current_row("operation_id", op_id);
trace.set_current_row("operation_id_next", op_id);
trace.set_current_row("A_byte", (a2 as u32).into());
trace.set_current_row("B_byte", (b2 as u32).into());
trace.set_current_row("C_byte", (c2 as u32).into());
Expand All @@ -284,6 +287,7 @@ impl SubmachineKind for BinaryMachine {

trace.push_row();
trace.set_current_row("operation_id", op_id);
trace.set_current_row("operation_id_next", op_id);
trace.set_current_row("A_byte", (a3 as u32).into());
trace.set_current_row("B_byte", (b3 as u32).into());
trace.set_current_row("C_byte", (c3 as u32).into());
Expand All @@ -293,6 +297,7 @@ impl SubmachineKind for BinaryMachine {

trace.push_row();
trace.set_current_row("operation_id", op_id);
trace.set_current_row("operation_id_next", op_id);
trace.set_current_row("A_byte", (a4 as u32).into());
trace.set_current_row("B_byte", (b4 as u32).into());
trace.set_current_row("C_byte", (c4 as u32).into());
Expand Down Expand Up @@ -352,43 +357,53 @@ impl SubmachineKind for ShiftMachine {
if trace.len() > 0 {
trace.set_current_row("A_byte", (b1 as u32).into());
trace.set_current_row("C_part", (((b1 as u32) << shl) >> shr).into());
trace.set_current_row("operation_id_next", op_id);
trace.set_current_row("B_next", b);
} else {
trace.set_final_row("A_byte", (b1 as u32).into());
trace.set_final_row("C_part", (((b1 as u32) << shl) >> shr).into());
trace.set_final_row("operation_id_next", op_id);
trace.set_final_row("B_next", b);
}

// 4 rows for each shift operation
trace.push_row();
trace.set_current_row("operation_id", op_id);
trace.set_current_row("operation_id_next", op_id);
trace.set_current_row("A_byte", (b2 as u32).into());
let c_part_factor = (b2 as u32) << 8;
let c_part = ((c_part_factor << shl) >> shr).into();
trace.set_current_row("C_part", c_part);
let a_row = a.u() & 0xff;
trace.set_current_row("A", a_row.into());
trace.set_current_row("B", b);
trace.set_current_row("B_next", b);
trace.set_current_row("C", ((a_row << shl) >> shr).into());
//
trace.push_row();
trace.set_current_row("operation_id", op_id);
trace.set_current_row("operation_id_next", op_id);
trace.set_current_row("A_byte", (b3 as u32).into());
let c_part_factor = (b3 as u32) << 16;
let c_part = ((c_part_factor << shl) >> shr).into();
trace.set_current_row("C_part", c_part);
let a_row = a.u() & 0xffff;
trace.set_current_row("A", a_row.into());
trace.set_current_row("B", b);
trace.set_current_row("B_next", b);
trace.set_current_row("C", ((a_row << shl) >> shr).into());
//
trace.push_row();
trace.set_current_row("operation_id", op_id);
trace.set_current_row("operation_id_next", op_id);
trace.set_current_row("A_byte", (b4 as u32).into());
let c_part_factor = (b4 as u32) << 24;
let c_part = ((c_part_factor << shl) >> shr).into();
trace.set_current_row("C_part", c_part);
let a_row = a.u() & 0xffffff;
trace.set_current_row("A", a_row.into());
trace.set_current_row("B", b);
trace.set_current_row("B_next", b);
trace.set_current_row("C", ((a_row << shl) >> shr).into());
// latch row
trace.push_row();
Expand Down
8 changes: 6 additions & 2 deletions riscv/tests/common/mod.rs
Original file line number Diff line number Diff line change
@@ -1,7 +1,9 @@
use mktemp::Temp;
use powdr_number::{BabyBearField, FieldElement, GoldilocksField, KnownField, KoalaBearField};
use powdr_pipeline::{
test_util::{run_pilcom_with_backend_variant, test_plonky3_pipeline, BackendVariant},
test_util::{
run_pilcom_with_backend_variant, test_mock_backend, test_plonky3_pipeline, BackendVariant,
},
Pipeline,
};
use powdr_riscv::CompilerOptions;
Expand Down Expand Up @@ -32,6 +34,8 @@ pub fn verify_riscv_asm_string<T: FieldElement, S: serde::Serialize + Send + Syn
// Compute the witness once for all tests that follow.
pipeline.compute_witness().unwrap();

test_mock_backend(pipeline.clone());

// verify with PILCOM
if T::known_field().unwrap() == KnownField::GoldilocksField {
let pipeline_gl: Pipeline<GoldilocksField> =
Expand Down Expand Up @@ -72,7 +76,7 @@ pub fn verify_riscv_asm_string<T: FieldElement, S: serde::Serialize + Send + Syn
pipeline.rollback_from_witness();
let executor_trace: Vec<_> = execution.trace.into_iter().collect();
let pipeline = pipeline.add_external_witness_values(executor_trace);
test_plonky3_pipeline::<T>(pipeline);
test_mock_backend(pipeline);
}
}

Expand Down
7 changes: 6 additions & 1 deletion std/machines/large_field/binary.asm
Original file line number Diff line number Diff line change
Expand Up @@ -35,5 +35,10 @@ machine Binary(byte_binary: ByteBinary) with
B' = B * (1 - latch) + B_byte * FACTOR;
C' = C * (1 - latch) + C_byte * FACTOR;

link => C_byte = byte_binary.run(operation_id', A_byte, B_byte);
// TODO: Currently, the bus linker does not support next references in operations and links.
// We add an extra witness column to make the Goldilocks RISC-V machine work for now.
// This will be fixed with #2140.
col witness operation_id_next;
operation_id' = operation_id_next;
link => C_byte = byte_binary.run(operation_id_next, A_byte, B_byte);
}
8 changes: 7 additions & 1 deletion std/machines/large_field/shift.asm
Original file line number Diff line number Diff line change
Expand Up @@ -67,5 +67,11 @@ machine Shift(byte_shift: ByteShift) with
unchanged_until(B, latch);
C' = C * (1 - latch) + C_part;

link => C_part = byte_shift.run(operation_id', A_byte, B', FACTOR_ROW);
// TODO: Currently, the bus linker does not support next references in operations and links.
// We add an extra witness columns to make the Goldilocks RISC-V machine work for now.
// This will be fixed with #2140.
col witness operation_id_next, B_next;
operation_id' = operation_id_next;
B' = B_next;
link => C_part = byte_shift.run(operation_id_next, A_byte, B_next, FACTOR_ROW);
}

0 comments on commit bfaeff4

Please sign in to comment.