From 8dd220ae127baf6cc5a31d8ab7ffdeeb161f6109 Mon Sep 17 00:00:00 2001 From: Tom French <15848336+TomAFrench@users.noreply.github.com> Date: Wed, 26 Jul 2023 19:49:33 +0800 Subject: [PATCH] chore(acir)!: Remove `Block`, `RAM` and `ROM` opcodes (#457) --- acir/src/circuit/opcodes.rs | 37 +---------- acir/src/circuit/opcodes/block.rs | 67 -------------------- acir/src/circuit/opcodes/memory_operation.rs | 28 ++++++++ acvm/src/compiler/mod.rs | 4 -- acvm/src/compiler/transformers/fallback.rs | 7 +- acvm/src/pwg/{block.rs => memory_op.rs} | 12 ++-- acvm/src/pwg/mod.rs | 17 ++--- 7 files changed, 44 insertions(+), 128 deletions(-) delete mode 100644 acir/src/circuit/opcodes/block.rs create mode 100644 acir/src/circuit/opcodes/memory_operation.rs rename acvm/src/pwg/{block.rs => memory_op.rs} (92%) diff --git a/acir/src/circuit/opcodes.rs b/acir/src/circuit/opcodes.rs index 7d6e5efb2..ea6b5fac3 100644 --- a/acir/src/circuit/opcodes.rs +++ b/acir/src/circuit/opcodes.rs @@ -6,10 +6,10 @@ use crate::native_types::{Expression, Witness}; use serde::{Deserialize, Serialize}; mod black_box_function_call; -mod block; +mod memory_operation; pub use black_box_function_call::{BlackBoxFuncCall, FunctionInput}; -pub use block::{BlockId, MemOp, MemoryBlock}; +pub use memory_operation::{BlockId, MemOp}; #[derive(Clone, PartialEq, Eq, Serialize, Deserialize)] pub enum Opcode { @@ -19,24 +19,8 @@ pub enum Opcode { /// Often used for exposing more efficient implementations of SNARK-unfriendly computations. BlackBoxFuncCall(BlackBoxFuncCall), Directive(Directive), - /// Abstract read/write operations on a block of data. In particular; - /// - It does not require an initialization phase - /// - Operations do not need to be constant, they can be any expression which resolves to 0 or 1. - Block(MemoryBlock), - /// Same as Block, but it starts with an initialization phase and then have only read operation - /// - init: write operations with index from 0..MemoryBlock.len - /// - after MemoryBlock.len; all operations are read - /// - /// ROM can be more efficiently handled because we do not need to check for the operation value (which is always 0). - ROM(MemoryBlock), - /// Same as ROM, but can have read or write operations - /// - init = write operations with index 0..MemoryBlock.len - /// - after MemoryBlock.len, all operations are constant expressions (0 or 1) - // TODO(#319): Review this comment and generalize it to be useful for other backends. - // RAM is required for acvm-backend-barretenberg as dynamic memory implementation in Barretenberg requires an initialization phase and can only handle constant values for operations. - RAM(MemoryBlock), Brillig(Brillig), - /// Atomic operation on a memory block + /// Atomic operation on a block of memory MemoryOp { block_id: BlockId, op: MemOp, @@ -70,9 +54,6 @@ impl Opcode { Opcode::Arithmetic(_) => "arithmetic", Opcode::Directive(directive) => directive.name(), Opcode::BlackBoxFuncCall(g) => g.name(), - Opcode::Block(_) => "block", - Opcode::RAM(_) => "ram", - Opcode::ROM(_) => "rom", Opcode::Brillig(_) => "brillig", Opcode::MemoryOp { .. } => "mem", Opcode::MemoryInit { .. } => "init memory block", @@ -171,18 +152,6 @@ impl std::fmt::Display for Opcode { witnesses.last().unwrap().witness_index() ), }, - Opcode::Block(block) => { - write!(f, "BLOCK ")?; - write!(f, "(id: {}, len: {}) ", block.id.0, block.trace.len()) - } - Opcode::ROM(block) => { - write!(f, "ROM ")?; - write!(f, "(id: {}, len: {}) ", block.id.0, block.trace.len()) - } - Opcode::RAM(block) => { - write!(f, "RAM ")?; - write!(f, "(id: {}, len: {}) ", block.id.0, block.trace.len()) - } Opcode::Brillig(brillig) => { write!(f, "BRILLIG: ")?; writeln!(f, "inputs: {:?}", brillig.inputs)?; diff --git a/acir/src/circuit/opcodes/block.rs b/acir/src/circuit/opcodes/block.rs deleted file mode 100644 index a7f9ddca4..000000000 --- a/acir/src/circuit/opcodes/block.rs +++ /dev/null @@ -1,67 +0,0 @@ -use crate::native_types::{Expression, Witness}; -use acir_field::FieldElement; -use serde::{Deserialize, Serialize}; - -#[derive(Clone, PartialEq, Eq, Serialize, Deserialize, Hash, Copy, Default)] -pub struct BlockId(pub u32); - -/// Operation on a block -/// We can either write or read at a block index -#[derive(Clone, PartialEq, Eq, Serialize, Deserialize, Debug)] -pub struct MemOp { - /// Can be 0 (read) or 1 (write) - pub operation: Expression, - pub index: Expression, - pub value: Expression, -} - -impl MemOp { - /// Creates a `MemOp` which reads from memory at `index` and inserts the read value - /// into the [`WitnessMap`][crate::native_types::WitnessMap] at `witness` - pub fn read_at_mem_index(index: Expression, witness: Witness) -> Self { - MemOp { operation: Expression::zero(), index, value: witness.into() } - } - - /// Creates a `MemOp` which writes the [`Expression`] `value` into memory at `index`. - pub fn write_to_mem_index(index: Expression, value: Expression) -> Self { - MemOp { operation: Expression::one(), index, value } - } -} - -/// Represents operations on a block of length len of data -#[derive(Clone, PartialEq, Eq, Serialize, Deserialize)] -pub struct MemoryBlock { - /// Id of the block - pub id: BlockId, - /// Length of the memory block - pub len: u32, - /// Trace of memory operations - pub trace: Vec, -} - -impl MemoryBlock { - /// Returns the initialization vector of the MemoryBlock - pub fn init_phase(&self) -> Vec { - let mut init = Vec::new(); - for i in 0..self.len as usize { - assert_eq!( - self.trace[i].operation, - Expression::one(), - "Block initialization requires a write" - ); - let index = self.trace[i] - .index - .to_const() - .expect("Non-const index during Block initialization"); - if index != FieldElement::from(i as i128) { - todo!( - "invalid index when initializing a block, we could try to sort the init phase" - ); - } - let value = self.trace[i].value.clone(); - assert!(value.is_degree_one_univariate(), "Block initialization requires a witness"); - init.push(value); - } - init - } -} diff --git a/acir/src/circuit/opcodes/memory_operation.rs b/acir/src/circuit/opcodes/memory_operation.rs new file mode 100644 index 000000000..9e45dc4ee --- /dev/null +++ b/acir/src/circuit/opcodes/memory_operation.rs @@ -0,0 +1,28 @@ +use crate::native_types::{Expression, Witness}; +use serde::{Deserialize, Serialize}; + +#[derive(Clone, PartialEq, Eq, Serialize, Deserialize, Hash, Copy, Default)] +pub struct BlockId(pub u32); + +/// Operation on a block of memory +/// We can either write or read at an index in memory +#[derive(Clone, PartialEq, Eq, Serialize, Deserialize, Debug)] +pub struct MemOp { + /// Can be 0 (read) or 1 (write) + pub operation: Expression, + pub index: Expression, + pub value: Expression, +} + +impl MemOp { + /// Creates a `MemOp` which reads from memory at `index` and inserts the read value + /// into the [`WitnessMap`][crate::native_types::WitnessMap] at `witness` + pub fn read_at_mem_index(index: Expression, witness: Witness) -> Self { + MemOp { operation: Expression::zero(), index, value: witness.into() } + } + + /// Creates a `MemOp` which writes the [`Expression`] `value` into memory at `index`. + pub fn write_to_mem_index(index: Expression, value: Expression) -> Self { + MemOp { operation: Expression::one(), index, value } + } +} diff --git a/acvm/src/compiler/mod.rs b/acvm/src/compiler/mod.rs index 80e9cfb77..fc787fde9 100644 --- a/acvm/src/compiler/mod.rs +++ b/acvm/src/compiler/mod.rs @@ -199,10 +199,6 @@ pub fn compile( new_opcode_labels.push(opcode_label[index]); transformed_gates.push(opcode.clone()); } - - Opcode::Block(_) | Opcode::ROM(_) | Opcode::RAM(_) => { - unimplemented!("Stepwise execution is not compatible with {}", opcode.name()) - } Opcode::Brillig(brillig) => { for output in &brillig.outputs { match output { diff --git a/acvm/src/compiler/transformers/fallback.rs b/acvm/src/compiler/transformers/fallback.rs index 7bc530add..cfc28353d 100644 --- a/acvm/src/compiler/transformers/fallback.rs +++ b/acvm/src/compiler/transformers/fallback.rs @@ -21,12 +21,7 @@ impl FallbackTransformer { for (idx, opcode) in acir.opcodes.into_iter().enumerate() { match &opcode { - Opcode::Arithmetic(_) - | Opcode::Directive(_) - | Opcode::Brillig(_) - | Opcode::Block(_) - | Opcode::ROM(_) - | Opcode::RAM(_) => { + Opcode::Arithmetic(_) | Opcode::Directive(_) | Opcode::Brillig(_) => { // directive, arithmetic expression or blocks are handled by acvm new_opcode_labels.push(opcode_labels[idx]); acir_supported_opcodes.push(opcode); diff --git a/acvm/src/pwg/block.rs b/acvm/src/pwg/memory_op.rs similarity index 92% rename from acvm/src/pwg/block.rs rename to acvm/src/pwg/memory_op.rs index a5cbeb4e6..4250b8b7a 100644 --- a/acvm/src/pwg/block.rs +++ b/acvm/src/pwg/memory_op.rs @@ -11,15 +11,13 @@ use super::{arithmetic::ArithmeticSolver, get_value, insert_value, witness_to_va type MemoryIndex = u32; -/// Maintains the state for solving Block opcode -/// block_value is the value of the Block at the solved_operations step -/// solved_operations is the number of solved elements in the block +/// Maintains the state for solving [`MemoryInit`][`acir::circuit::Opcode::MemoryInit`] and [`MemoryOp`][`acir::circuit::Opcode::MemoryOp`] opcodes. #[derive(Default)] -pub(super) struct BlockSolver { +pub(super) struct MemoryOpSolver { block_value: HashMap, } -impl BlockSolver { +impl MemoryOpSolver { fn write_memory_index(&mut self, index: MemoryIndex, value: FieldElement) { self.block_value.insert(index, value); } @@ -100,7 +98,7 @@ mod tests { FieldElement, }; - use super::BlockSolver; + use super::MemoryOpSolver; #[test] fn test_solver() { @@ -117,7 +115,7 @@ mod tests { MemOp::read_at_mem_index(FieldElement::one().into(), Witness(4)), ]; - let mut block_solver = BlockSolver::default(); + let mut block_solver = MemoryOpSolver::default(); block_solver.init(&init, &initial_witness).unwrap(); for op in trace { diff --git a/acvm/src/pwg/mod.rs b/acvm/src/pwg/mod.rs index 136ed7c92..366a26513 100644 --- a/acvm/src/pwg/mod.rs +++ b/acvm/src/pwg/mod.rs @@ -11,8 +11,8 @@ use acir::{ use blackbox_solver::BlackBoxResolutionError; use self::{ - arithmetic::ArithmeticSolver, block::BlockSolver, brillig::BrilligSolver, - directives::solve_directives, + arithmetic::ArithmeticSolver, brillig::BrilligSolver, directives::solve_directives, + memory_op::MemoryOpSolver, }; use crate::{BlackBoxFunctionSolver, Language}; @@ -26,7 +26,7 @@ mod brillig; mod directives; // black box functions mod blackbox; -mod block; +mod memory_op; pub use brillig::ForeignCallWaitInfo; @@ -98,8 +98,8 @@ pub struct ACVM { backend: B, - /// Stores the solver for each [block][`Opcode::Block`] opcode. This persists their internal state to prevent recomputation. - block_solvers: HashMap, + /// Stores the solver for memory operations acting on blocks of memory disambiguated by [block][`BlockId`]. + block_solvers: HashMap, /// A list of opcodes which are to be executed by the ACVM. opcodes: Vec, @@ -227,9 +227,6 @@ impl ACVM { res => res.map(|_| ()), } } - Opcode::Block(_) | Opcode::ROM(_) | Opcode::RAM(_) => { - panic!("Block, ROM and RAM opcodes are not supported by stepwise ACVM") - } }; match resolution { Ok(()) => { @@ -350,8 +347,8 @@ pub fn default_is_opcode_supported(language: Language) -> fn(&Opcode) -> bool { // The ones which are not supported, the acvm compiler will // attempt to transform into supported gates. If these are also not available // then a compiler error will be emitted. - fn plonk_is_supported(opcode: &Opcode) -> bool { - !matches!(opcode, Opcode::Block(_)) + fn plonk_is_supported(_opcode: &Opcode) -> bool { + true } match language {