From 99d1b87143be881d6dc49d35c2c1253ba951c72a Mon Sep 17 00:00:00 2001 From: naure Date: Mon, 16 Sep 2024 00:31:43 +0200 Subject: [PATCH] hide-step: Simplify tests and keep details of StepRecord private (#226) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Aurélien Nicolas --- ceno_emul/src/addr.rs | 1 + ceno_emul/src/tracer.rs | 52 +++++-- ceno_zkvm/src/instructions/riscv/addsub.rs | 132 +++++------------- ceno_zkvm/src/instructions/riscv/constants.rs | 3 +- 4 files changed, 78 insertions(+), 110 deletions(-) diff --git a/ceno_emul/src/addr.rs b/ceno_emul/src/addr.rs index 0a8de8ad9..aaa4ef48f 100644 --- a/ceno_emul/src/addr.rs +++ b/ceno_emul/src/addr.rs @@ -17,6 +17,7 @@ use std::{fmt, ops}; pub const WORD_SIZE: usize = 4; +pub const PC_STEP_SIZE: usize = 4; // Type aliases to clarify the code without wrapper types. pub type Word = u32; diff --git a/ceno_emul/src/tracer.rs b/ceno_emul/src/tracer.rs index d419a2296..1391c33eb 100644 --- a/ceno_emul/src/tracer.rs +++ b/ceno_emul/src/tracer.rs @@ -3,7 +3,7 @@ use std::{collections::HashMap, fmt, mem}; use crate::{ addr::{ByteAddr, Cycle, RegIdx, Word, WordAddr}, rv32im::DecodedInstruction, - CENO_PLATFORM, + CENO_PLATFORM, PC_STEP_SIZE, }; /// An instruction and its context in an execution trace. That is concrete values of registers and memory. @@ -17,18 +17,18 @@ use crate::{ /// - Any of `rs1 / rs2 / rd` **may be `x0`**. The trace handles this like any register, including the value that was _supposed_ to be stored. The circuits must handle this case: either **store `0` or skip `x0` operations**. /// /// - Any pair of `rs1 / rs2 / rd` **may be the same**. Then, one op will point to the other op in the same instruction but a different subcycle. The circuits may follow the operations **without special handling** of repeated registers. -#[derive(Clone, Debug, Default)] +#[derive(Clone, Debug, Default, PartialEq, Eq)] pub struct StepRecord { - pub cycle: Cycle, - pub pc: Change, - pub insn_code: Word, + cycle: Cycle, + pc: Change, + insn_code: Word, - pub rs1: Option, - pub rs2: Option, + rs1: Option, + rs2: Option, - pub rd: Option, + rd: Option, - pub memory_op: Option, + memory_op: Option, } #[derive(Clone, Debug, PartialEq, Eq, Hash)] @@ -53,6 +53,38 @@ pub type ReadOp = MemOp; pub type WriteOp = MemOp>; impl StepRecord { + pub fn new_r_instruction( + cycle: Cycle, + pc: ByteAddr, + insn_code: Word, + rs1_read: Word, + rs2_read: Word, + rd: Change, + ) -> StepRecord { + let insn = DecodedInstruction::new(insn_code); + StepRecord { + cycle, + pc: Change::new(pc, pc + PC_STEP_SIZE), + insn_code, + rs1: Some(ReadOp { + addr: CENO_PLATFORM.register_vma(insn.rs1() as RegIdx).into(), + value: rs1_read, + previous_cycle: 0, + }), + rs2: Some(ReadOp { + addr: CENO_PLATFORM.register_vma(insn.rs2() as RegIdx).into(), + value: rs2_read, + previous_cycle: 0, + }), + rd: Some(WriteOp { + addr: CENO_PLATFORM.register_vma(insn.rd() as RegIdx).into(), + value: rd, + previous_cycle: 0, + }), + memory_op: None, + } + } + pub fn cycle(&self) -> Cycle { self.cycle } @@ -210,7 +242,7 @@ impl Tracer { } } -#[derive(Copy, Clone, Default)] +#[derive(Copy, Clone, Default, PartialEq, Eq)] pub struct Change { pub before: T, pub after: T, diff --git a/ceno_zkvm/src/instructions/riscv/addsub.rs b/ceno_zkvm/src/instructions/riscv/addsub.rs index 993cc5b6a..5525fdfec 100644 --- a/ceno_zkvm/src/instructions/riscv/addsub.rs +++ b/ceno_zkvm/src/instructions/riscv/addsub.rs @@ -284,14 +284,14 @@ impl Instruction for SubInstruction { #[cfg(test)] mod test { - use ceno_emul::{Change, ReadOp, StepRecord, WriteOp, CENO_PLATFORM}; + use ceno_emul::{Change, StepRecord}; use goldilocks::GoldilocksExt2; use itertools::Itertools; use multilinear_extensions::mle::IntoMLEs; use crate::{ circuit_builder::{CircuitBuilder, ConstraintSystem}, - instructions::{riscv::constants::PC_STEP_SIZE, Instruction}, + instructions::Instruction, scheme::mock_prover::{MockProver, MOCK_PC_ADD, MOCK_PC_SUB, MOCK_PROGRAM}, }; @@ -316,30 +316,14 @@ mod test { let (raw_witin, _) = AddInstruction::assign_instances( &config, cb.cs.num_witin as usize, - vec![StepRecord { - cycle: 3, - pc: Change::new(MOCK_PC_ADD, MOCK_PC_ADD + PC_STEP_SIZE), - insn_code: MOCK_PROGRAM[0], - rs1: Some(ReadOp { - addr: CENO_PLATFORM.register_vma(2).into(), - value: 11u32, - previous_cycle: 0, - }), - rs2: Some(ReadOp { - addr: CENO_PLATFORM.register_vma(3).into(), - value: 0xfffffffeu32, - previous_cycle: 0, - }), - rd: Some(WriteOp { - addr: CENO_PLATFORM.register_vma(4).into(), - value: Change { - before: 0u32, - after: 11u32.wrapping_add(0xfffffffeu32), - }, - previous_cycle: 0, - }), - ..Default::default() - }], + vec![StepRecord::new_r_instruction( + 3, + MOCK_PC_ADD, + MOCK_PROGRAM[0], + 11, + 0xfffffffe, + Change::new(0, 11_u32.wrapping_add(0xfffffffe)), + )], ) .unwrap(); @@ -374,30 +358,14 @@ mod test { let (raw_witin, _) = AddInstruction::assign_instances( &config, cb.cs.num_witin as usize, - vec![StepRecord { - cycle: 3, - pc: Change::new(MOCK_PC_ADD, MOCK_PC_ADD + PC_STEP_SIZE), - insn_code: MOCK_PROGRAM[0], - rs1: Some(ReadOp { - addr: CENO_PLATFORM.register_vma(2).into(), - value: u32::MAX - 1, - previous_cycle: 0, - }), - rs2: Some(ReadOp { - addr: CENO_PLATFORM.register_vma(3).into(), - value: u32::MAX - 1, - previous_cycle: 0, - }), - rd: Some(WriteOp { - addr: CENO_PLATFORM.register_vma(4).into(), - value: Change { - before: 0u32, - after: (u32::MAX - 1).wrapping_add(u32::MAX - 1), - }, - previous_cycle: 0, - }), - ..Default::default() - }], + vec![StepRecord::new_r_instruction( + 3, + MOCK_PC_ADD, + MOCK_PROGRAM[0], + u32::MAX - 1, + u32::MAX - 1, + Change::new(0, (u32::MAX - 1).wrapping_add(u32::MAX - 1)), + )], ) .unwrap(); @@ -432,30 +400,14 @@ mod test { let (raw_witin, _) = SubInstruction::assign_instances( &config, cb.cs.num_witin as usize, - vec![StepRecord { - cycle: 3, - pc: Change::new(MOCK_PC_SUB, MOCK_PC_SUB + PC_STEP_SIZE), - insn_code: MOCK_PROGRAM[1], - rs1: Some(ReadOp { - addr: CENO_PLATFORM.register_vma(2).into(), - value: 11u32, - previous_cycle: 0, - }), - rs2: Some(ReadOp { - addr: CENO_PLATFORM.register_vma(3).into(), - value: 2u32, - previous_cycle: 0, - }), - rd: Some(WriteOp { - addr: CENO_PLATFORM.register_vma(4).into(), - value: Change { - before: 0u32, - after: 11u32.wrapping_sub(2u32), - }, - previous_cycle: 0, - }), - ..Default::default() - }], + vec![StepRecord::new_r_instruction( + 3, + MOCK_PC_SUB, + MOCK_PROGRAM[1], + 11, + 2, + Change::new(0, 11_u32.wrapping_sub(2)), + )], ) .unwrap(); @@ -490,30 +442,14 @@ mod test { let (raw_witin, _) = SubInstruction::assign_instances( &config, cb.cs.num_witin as usize, - vec![StepRecord { - cycle: 3, - pc: Change::new(MOCK_PC_SUB, MOCK_PC_SUB + PC_STEP_SIZE), - insn_code: MOCK_PROGRAM[1], - rs1: Some(ReadOp { - addr: CENO_PLATFORM.register_vma(2).into(), - value: 3u32, - previous_cycle: 0, - }), - rs2: Some(ReadOp { - addr: CENO_PLATFORM.register_vma(3).into(), - value: 11u32, - previous_cycle: 0, - }), - rd: Some(WriteOp { - addr: CENO_PLATFORM.register_vma(4).into(), - value: Change { - before: 0u32, - after: 3u32.wrapping_sub(11u32), - }, - previous_cycle: 0, - }), - ..Default::default() - }], + vec![StepRecord::new_r_instruction( + 3, + MOCK_PC_SUB, + MOCK_PROGRAM[1], + 3, + 11, + Change::new(0, 3_u32.wrapping_sub(11)), + )], ) .unwrap(); diff --git a/ceno_zkvm/src/instructions/riscv/constants.rs b/ceno_zkvm/src/instructions/riscv/constants.rs index 813f2b340..e8c12cdb0 100644 --- a/ceno_zkvm/src/instructions/riscv/constants.rs +++ b/ceno_zkvm/src/instructions/riscv/constants.rs @@ -1,8 +1,7 @@ use std::fmt; use crate::uint::UInt; - -pub(crate) const PC_STEP_SIZE: usize = 4; +pub use ceno_emul::PC_STEP_SIZE; pub const OPCODE_OP: usize = 0x33; pub const FUNCT3_ADD_SUB: usize = 0;