feat: Change execute to take &Instruction

It seems wrong to take Instruction, since InstructionExecutor doesn't take
ownership of the instruction; it just borrows it to execute.

Should also reduce the amount of data cloned (some copying still takes place
because of record creation).

crates/circuits/mod-builder/src/core_chip.rs

@@ -231,7 +231,7 @@ where
             inputs.push(input);
         }
 
-        let Instruction { opcode, .. } = instruction.clone();
+        let Instruction { opcode, .. } = instruction;
         let local_opcode_idx = opcode.local_opcode_idx(self.air.offset);
         let mut flags = vec![];
 

crates/toolchain/instructions/src/program.rs

@@ -167,11 +167,10 @@ impl<F: Field> Program<F> {
     pub fn get_instruction_and_debug_info(
         &self,
         index: usize,
-    ) -> Option<(Instruction<F>, Option<DebugInfo>)> {
+    ) -> Option<&(Instruction<F>, Option<DebugInfo>)> {
         self.instructions_and_debug_infos
             .get(index)
-            .cloned()
-            .flatten()
+            .and_then(|x| x.as_ref())
     }
 
     pub fn push_instruction_and_debug_info(

crates/vm/derive/src/lib.rs

@@ -38,7 +38,7 @@ pub fn instruction_executor_derive(input: TokenStream) -> TokenStream {
                     fn execute(
                         &mut self,
                         memory: &mut ::openvm_circuit::system::memory::MemoryController<F>,
-                        instruction: ::openvm_circuit::arch::instructions::instruction::Instruction<F>,
+                        instruction: &::openvm_circuit::arch::instructions::instruction::Instruction<F>,
                         from_state: ::openvm_circuit::arch::ExecutionState<u32>,
                     ) -> ::openvm_circuit::arch::Result<::openvm_circuit::arch::ExecutionState<u32>> {
                         self.0.execute(memory, instruction, from_state)
@@ -92,7 +92,7 @@ pub fn instruction_executor_derive(input: TokenStream) -> TokenStream {
                     fn execute(
                         &mut self,
                         memory: &mut ::openvm_circuit::system::memory::MemoryController<#first_ty_generic>,
-                        instruction: ::openvm_circuit::arch::instructions::instruction::Instruction<#first_ty_generic>,
+                        instruction: &::openvm_circuit::arch::instructions::instruction::Instruction<#first_ty_generic>,
                         from_state: ::openvm_circuit::arch::ExecutionState<u32>,
                     ) -> ::openvm_circuit::arch::Result<::openvm_circuit::arch::ExecutionState<u32>> {
                         match self {

crates/vm/src/arch/execution.rs

@@ -66,7 +66,7 @@ pub trait InstructionExecutor<F> {
     fn execute(
         &mut self,
         memory: &mut MemoryController<F>,
-        instruction: Instruction<F>,
+        instruction: &Instruction<F>,
         from_state: ExecutionState<u32>,
     ) -> Result<ExecutionState<u32>>;
 
@@ -79,7 +79,7 @@ impl<F, C: InstructionExecutor<F>> InstructionExecutor<F> for RefCell<C> {
     fn execute(
         &mut self,
         memory: &mut MemoryController<F>,
-        instruction: Instruction<F>,
+        instruction: &Instruction<F>,
         prev_state: ExecutionState<u32>,
     ) -> Result<ExecutionState<u32>> {
         self.borrow_mut().execute(memory, instruction, prev_state)
@@ -94,7 +94,7 @@ impl<F, C: InstructionExecutor<F>> InstructionExecutor<F> for Rc<RefCell<C>> {
     fn execute(
         &mut self,
         memory: &mut MemoryController<F>,
-        instruction: Instruction<F>,
+        instruction: &Instruction<F>,
         prev_state: ExecutionState<u32>,
     ) -> Result<ExecutionState<u32>> {
         self.borrow_mut().execute(memory, instruction, prev_state)

crates/vm/src/arch/integration_api.rs

@@ -214,16 +214,16 @@ where
     fn execute(
         &mut self,
         memory: &mut MemoryController<F>,
-        instruction: Instruction<F>,
+        instruction: &Instruction<F>,
         from_state: ExecutionState<u32>,
     ) -> Result<ExecutionState<u32>> {
-        let (reads, read_record) = self.adapter.preprocess(memory, &instruction)?;
+        let (reads, read_record) = self.adapter.preprocess(memory, instruction)?;
         let (output, core_record) =
             self.core
-                .execute_instruction(&instruction, from_state.pc, reads)?;
+                .execute_instruction(instruction, from_state.pc, reads)?;
         let (to_state, write_record) =
             self.adapter
-                .postprocess(memory, &instruction, from_state, output, &read_record)?;
+                .postprocess(memory, instruction, from_state, output, &read_record)?;
         self.records.push((read_record, write_record, core_record));
         Ok(to_state)
     }

crates/vm/src/arch/segment.rs

@@ -1,5 +1,9 @@
 use backtrace::Backtrace;
-use openvm_instructions::{exe::FnBounds, instruction::DebugInfo, program::Program};
+use openvm_instructions::{
+    exe::FnBounds,
+    instruction::{DebugInfo, Instruction},
+    program::Program,
+};
 use openvm_stark_backend::{
     config::{Domain, StarkGenericConfig},
     p3_commit::PolynomialSpace,
@@ -10,7 +14,8 @@ use openvm_stark_backend::{
 };
 
 use super::{
-    ExecutionError, Streams, SystemConfig, VmChipComplex, VmComplexTraceHeights, VmConfig,
+    ExecutionError, Streams, SystemBase, SystemConfig, VmChipComplex, VmComplexTraceHeights,
+    VmConfig,
 };
 #[cfg(feature = "bench-metrics")]
 use crate::metrics::VmMetrics;
@@ -106,77 +111,91 @@ impl<F: PrimeField32, VC: VmConfig<F>> ExecutionSegment<F, VC> {
         let mut did_terminate = false;
 
         loop {
-            let (instruction, debug_info) =
-                self.chip_complex.program_chip_mut().get_instruction(pc)?;
-            tracing::trace!("pc: {pc:#x} | time: {timestamp} | {:?}", instruction);
-
             #[allow(unused_variables)]
-            let (dsl_instr, trace) = debug_info.map_or(
-                (None, None),
-                |DebugInfo {
-                     dsl_instruction,
-                     trace,
-                 }| (Some(dsl_instruction), trace),
-            );
+            let (opcode, dsl_instr) = {
+                let Self {
+                    chip_complex,
+                    #[cfg(feature = "bench-metrics")]
+                    metrics,
+                    ..
+                } = self;
+                let SystemBase {
+                    program_chip,
+                    memory_controller,
+                    ..
+                } = &mut chip_complex.base;
+
+                let (instruction, debug_info) = program_chip.get_instruction(pc)?;
+                tracing::trace!("pc: {pc:#x} | time: {timestamp} | {:?}", instruction);
 
-            let opcode = instruction.opcode;
-            if opcode == VmOpcode::with_default_offset(SystemOpcode::TERMINATE) {
-                did_terminate = true;
-                self.chip_complex.connector_chip_mut().end(
-                    ExecutionState::new(pc, timestamp),
-                    Some(instruction.c.as_canonical_u32()),
+                #[allow(unused_variables)]
+                let (dsl_instr, trace) = debug_info.as_ref().map_or(
+                    (None, None),
+                    |DebugInfo {
+                         dsl_instruction,
+                         trace,
+                     }| (Some(dsl_instruction), trace.as_ref()),
                 );
-                break;
-            }
 
-            // Some phantom instruction handling is more convenient to do here than in PhantomChip.
-            if opcode == VmOpcode::with_default_offset(SystemOpcode::PHANTOM) {
-                // Note: the discriminant is the lower 16 bits of the c operand.
-                let discriminant = instruction.c.as_canonical_u32() as u16;
-                let phantom = SysPhantom::from_repr(discriminant);
-                tracing::trace!("pc: {pc:#x} | system phantom: {phantom:?}");
-                match phantom {
-                    Some(SysPhantom::DebugPanic) => {
-                        if let Some(mut backtrace) = prev_backtrace {
-                            backtrace.resolve();
-                            eprintln!("openvm program failure; backtrace:\n{:?}", backtrace);
-                        } else {
-                            eprintln!("openvm program failure; no backtrace");
+                let &Instruction { opcode, c, .. } = instruction;
+                if opcode == VmOpcode::with_default_offset(SystemOpcode::TERMINATE) {
+                    did_terminate = true;
+                    self.chip_complex.connector_chip_mut().end(
+                        ExecutionState::new(pc, timestamp),
+                        Some(c.as_canonical_u32()),
+                    );
+                    break;
+                }
+
+                // Some phantom instruction handling is more convenient to do here than in PhantomChip.
+                if opcode == VmOpcode::with_default_offset(SystemOpcode::PHANTOM) {
+                    // Note: the discriminant is the lower 16 bits of the c operand.
+                    let discriminant = c.as_canonical_u32() as u16;
+                    let phantom = SysPhantom::from_repr(discriminant);
+                    tracing::trace!("pc: {pc:#x} | system phantom: {phantom:?}");
+                    match phantom {
+                        Some(SysPhantom::DebugPanic) => {
+                            if let Some(mut backtrace) = prev_backtrace {
+                                backtrace.resolve();
+                                eprintln!("openvm program failure; backtrace:\n{:?}", backtrace);
+                            } else {
+                                eprintln!("openvm program failure; no backtrace");
+                            }
+                            return Err(ExecutionError::Fail { pc });
                         }
-                        return Err(ExecutionError::Fail { pc });
-                    }
-                    Some(SysPhantom::CtStart) =>
-                    {
-                        #[cfg(feature = "bench-metrics")]
-                        self.metrics
-                            .cycle_tracker
-                            .start(dsl_instr.clone().unwrap_or("Default".to_string()))
-                    }
-                    Some(SysPhantom::CtEnd) =>
-                    {
-                        #[cfg(feature = "bench-metrics")]
-                        self.metrics
-                            .cycle_tracker
-                            .end(dsl_instr.clone().unwrap_or("Default".to_string()))
+                        Some(SysPhantom::CtStart) =>
+                        {
+                            #[cfg(feature = "bench-metrics")]
+                            metrics
+                                .cycle_tracker
+                                .start(dsl_instr.cloned().unwrap_or("Default".to_string()))
+                        }
+                        Some(SysPhantom::CtEnd) =>
+                        {
+                            #[cfg(feature = "bench-metrics")]
+                            metrics
+                                .cycle_tracker
+                                .end(dsl_instr.cloned().unwrap_or("Default".to_string()))
+                        }
+                        _ => {}
                     }
-                    _ => {}
                 }
-            }
-            prev_backtrace = trace;
+                prev_backtrace = trace.cloned();
 
-            let memory_controller = &mut self.chip_complex.base.memory_controller;
-            if let Some(executor) = self.chip_complex.inventory.get_mut_executor(&opcode) {
-                let next_state = InstructionExecutor::execute(
-                    executor,
-                    memory_controller,
-                    instruction,
-                    ExecutionState::new(pc, timestamp),
-                )?;
-                assert!(next_state.timestamp > timestamp);
-                pc = next_state.pc;
-                timestamp = next_state.timestamp;
-            } else {
-                return Err(ExecutionError::DisabledOperation { pc, opcode });
+                if let Some(executor) = chip_complex.inventory.get_mut_executor(&opcode) {
+                    let next_state = InstructionExecutor::execute(
+                        executor,
+                        memory_controller,
+                        instruction,
+                        ExecutionState::new(pc, timestamp),
+                    )?;
+                    assert!(next_state.timestamp > timestamp);
+                    pc = next_state.pc;
+                    timestamp = next_state.timestamp;
+                } else {
+                    return Err(ExecutionError::DisabledOperation { pc, opcode });
+                };
+                (opcode, dsl_instr.cloned())
             };
 
             #[cfg(feature = "bench-metrics")]

crates/vm/src/arch/testing/mod.rs

@@ -93,7 +93,7 @@ impl<F: PrimeField32> VmChipTestBuilder<F> {
     pub fn execute<E: InstructionExecutor<F>>(
         &mut self,
         executor: &mut E,
-        instruction: Instruction<F>,
+        instruction: &Instruction<F>,
     ) {
         let initial_pc = self.next_elem_size_u32();
         self.execute_with_pc(executor, instruction, initial_pc);
@@ -102,7 +102,7 @@ impl<F: PrimeField32> VmChipTestBuilder<F> {
     pub fn execute_with_pc<E: InstructionExecutor<F>>(
         &mut self,
         executor: &mut E,
-        instruction: Instruction<F>,
+        instruction: &Instruction<F>,
         initial_pc: u32,
     ) {
         let initial_state = ExecutionState {
@@ -114,7 +114,7 @@ impl<F: PrimeField32> VmChipTestBuilder<F> {
         let final_state = executor
             .execute(
                 &mut *self.memory.controller.borrow_mut(),
-                instruction.clone(),
+                instruction,
                 initial_state,
             )
             .expect("Expected the execution not to fail");

crates/vm/src/arch/testing/program/mod.rs

@@ -32,7 +32,7 @@ impl<F: PrimeField32> ProgramTester<F> {
         }
     }
 
-    pub fn execute(&mut self, instruction: Instruction<F>, initial_state: &ExecutionState<u32>) {
+    pub fn execute(&mut self, instruction: &Instruction<F>, initial_state: &ExecutionState<u32>) {
         self.records.push(ProgramExecutionCols {
             pc: F::from_canonical_u32(initial_state.pc),
             opcode: instruction.opcode.to_field(),

crates/vm/src/system/phantom/mod.rs

@@ -123,10 +123,10 @@ impl<F: PrimeField32> InstructionExecutor<F> for PhantomChip<F> {
     fn execute(
         &mut self,
         memory: &mut MemoryController<F>,
-        instruction: Instruction<F>,
+        instruction: &Instruction<F>,
         from_state: ExecutionState<u32>,
     ) -> Result<ExecutionState<u32>, ExecutionError> {
-        let Instruction {
+        let &Instruction {
             opcode, a, b, c, ..
         } = instruction;
         assert_eq!(opcode, self.air.phantom_opcode);

crates/vm/src/system/phantom/tests.rs

@@ -29,7 +29,7 @@ fn test_nops_and_terminate() {
     let mut state: ExecutionState<F> = ExecutionState::new(F::ZERO, F::ONE);
     let num_nops = 5;
     for _ in 0..num_nops {
-        tester.execute_with_pc(&mut chip, nop.clone(), state.pc.as_canonical_u32());
+        tester.execute_with_pc(&mut chip, &nop, state.pc.as_canonical_u32());
         let new_state = tester.execution.records.last().unwrap().final_state;
         assert_eq!(state.pc + F::from_canonical_usize(4), new_state.pc);
         assert_eq!(state.timestamp + F::ONE, new_state.timestamp);

crates/vm/src/system/program/mod.rs

@@ -72,7 +72,7 @@ impl<F: PrimeField64> ProgramChip<F> {
     pub fn get_instruction(
         &mut self,
         pc: u32,
-    ) -> Result<(Instruction<F>, Option<DebugInfo>), ExecutionError> {
+    ) -> Result<&(Instruction<F>, Option<DebugInfo>), ExecutionError> {
         let pc_index = self.get_pc_index(pc)?;
         self.execution_frequencies[pc_index] += 1;
         self.program

docs/crates/vm.md

@@ -11,7 +11,7 @@ pub trait InstructionExecutor<F> {
     /// current instance. May internally store records of this call for later trace generation.
     fn execute(
         &mut self,
-        instruction: Instruction<F>,
+        instruction: &Instruction<F>,
         from_state: ExecutionState<u32>,
   ) -> Result<ExecutionState<u32>>;
 }

extensions/algebra/circuit/src/fp2_chip/addsub.rs

@@ -189,8 +189,8 @@ mod tests {
             y_limbs,
             chip.0.core.air.offset + Fp2Opcode::SUB as usize,
         );
-        tester.execute(&mut chip, instruction1);
-        tester.execute(&mut chip, instruction2);
+        tester.execute(&mut chip, &instruction1);
+        tester.execute(&mut chip, &instruction2);
         let tester = tester.build().load(chip).load(bitwise_chip).finalize();
         tester.simple_test().expect("Verification failed");
     }

extensions/algebra/circuit/src/fp2_chip/muldiv.rs

@@ -233,8 +233,8 @@ mod tests {
             y_limbs,
             chip.0.core.air.offset + Fp2Opcode::DIV as usize,
         );
-        tester.execute(&mut chip, instruction1);
-        tester.execute(&mut chip, instruction2);
+        tester.execute(&mut chip, &instruction1);
+        tester.execute(&mut chip, &instruction2);
         let tester = tester.build().load(chip).load(bitwise_chip).finalize();
         tester.simple_test().expect("Verification failed");
     }

extensions/algebra/circuit/src/modular_chip/addsub.rs

@@ -155,7 +155,7 @@ where
     ) -> Result<(AdapterRuntimeContext<F, I>, Self::Record)> {
         let num_limbs = self.air.expr.canonical_num_limbs();
         let limb_bits = self.air.expr.canonical_limb_bits();
-        let Instruction { opcode, .. } = instruction.clone();
+        let Instruction { opcode, .. } = instruction;
         let local_opcode_idx = opcode.local_opcode_idx(self.air.offset);
         let data: DynArray<_> = reads.into();
         let data = data.0;

extensions/algebra/circuit/src/modular_chip/muldiv.rs

@@ -170,7 +170,7 @@ where
     ) -> Result<(AdapterRuntimeContext<F, I>, Self::Record)> {
         let num_limbs = self.air.expr.canonical_num_limbs();
         let limb_bits = self.air.expr.canonical_limb_bits();
-        let Instruction { opcode, .. } = instruction.clone();
+        let Instruction { opcode, .. } = instruction;
         let local_opcode_idx = opcode.local_opcode_idx(self.air.offset);
         let data: DynArray<_> = reads.into();
         let data = data.0;