perf: avoid extra copies of immediate args (#942)

lambdaclass · May 3, 2023 · c17aa23 · c17aa23
1 parent 21b6427
commit c17aa23
Show file tree

Hide file tree

Showing 9 changed files with 124 additions and 268 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -1006,6 +1006,10 @@
         ids.remainder.d2 = remainder_split[2]
     ```
 
+* BREAKING CHANGE: optimization for instruction decoding [#942](https://github.com/lambdaclass/cairo-rs/pull/942):
+    * Avoids copying immediate arguments to the `Instruction` structure, as they get inferred from the offset anyway
+    * Breaking: removal of the field `Instruction::imm`
+
 * Add missing `\n` character in traceback string [#997](https://github.com/lambdaclass/cairo-rs/pull/997)
     * BugFix: Add missing `\n` character after traceback lines when the filename is missing ("Unknown Location")
 

diff --git a/src/types/instruction.rs b/src/types/instruction.rs
@@ -15,7 +15,6 @@ pub struct Instruction {
     pub off0: isize,
     pub off1: isize,
     pub off2: isize,
-    pub imm: Option<Felt252>,
     pub dst_register: Register,
     pub op0_register: Register,
     pub op1_addr: Op1Addr,
@@ -75,20 +74,20 @@ pub enum Opcode {
 
 impl Instruction {
     pub fn size(&self) -> usize {
-        match self.imm {
-            Some(_) => 2,
-            None => 1,
+        match self.op1_addr {
+            Op1Addr::Imm => 2,
+            _ => 1,
         }
     }
 }
 
-// Returns True if the given instruction looks like a call instruction.
-pub(crate) fn is_call_instruction(encoded_instruction: &Felt252, imm: Option<&Felt252>) -> bool {
-    let encoded_i64_instruction: i64 = match encoded_instruction.to_i64() {
+// Returns True if the given instruction looks like a call instruction
+pub(crate) fn is_call_instruction(encoded_instruction: &Felt252) -> bool {
+    let encoded_i64_instruction = match encoded_instruction.to_u64() {
         Some(num) => num,
         None => return false,
     };
-    let instruction = match decode_instruction(encoded_i64_instruction, imm) {
+    let instruction = match decode_instruction(encoded_i64_instruction) {
         Ok(inst) => inst,
         Err(_) => return false,
     };
@@ -110,27 +109,28 @@ mod tests {
     #[cfg_attr(target_arch = "wasm32", wasm_bindgen_test)]
     fn is_call_instruction_true() {
         let encoded_instruction = Felt252::new(1226245742482522112_i64);
-        assert!(is_call_instruction(
-            &encoded_instruction,
-            Some(&Felt252::new(2))
-        ));
+        assert!(is_call_instruction(&encoded_instruction));
     }
+
     #[test]
     #[cfg_attr(target_arch = "wasm32", wasm_bindgen_test)]
     fn is_call_instruction_false() {
         let encoded_instruction = Felt252::new(4612671187288031229_i64);
-        assert!(!is_call_instruction(&encoded_instruction, None));
+        assert!(!is_call_instruction(&encoded_instruction));
+    }
+
+    #[test]
+    #[cfg_attr(target_arch = "wasm32", wasm_bindgen_test)]
+    fn is_call_instruction_invalid() {
+        let encoded_instruction = Felt252::new(1u64 << 63);
+        assert!(!is_call_instruction(&encoded_instruction));
     }
 
     #[test]
     #[cfg_attr(target_arch = "wasm32", wasm_bindgen_test)]
     fn instruction_size() {
         let encoded_instruction = Felt252::new(1226245742482522112_i64);
-        let instruction = decode_instruction(
-            encoded_instruction.to_i64().unwrap(),
-            Some(&Felt252::new(2)),
-        )
-        .unwrap();
+        let instruction = decode_instruction(encoded_instruction.to_u64().unwrap()).unwrap();
         assert_eq!(instruction.size(), 2);
     }
 }
diff --git a/src/utils.rs b/src/utils.rs
@@ -53,6 +53,7 @@ pub fn from_relocatable_to_indexes(relocatable: Relocatable) -> (usize, usize) {
 pub mod test_utils {
     use crate::types::exec_scope::ExecutionScopes;
     use crate::types::relocatable::MaybeRelocatable;
+    use crate::vm::trace::trace_entry::TraceEntry;
 
     #[macro_export]
     macro_rules! bigint {
@@ -354,23 +355,13 @@ pub mod test_utils {
     }
     pub(crate) use non_continuous_ids_data;
 
-    macro_rules! trace_check {
-        ( $trace: expr, [ $( ($off_pc:expr, $off_ap:expr, $off_fp:expr) ),+ ] ) => {
-            let mut index = -1;
-            $(
-                index += 1;
-                assert_eq!(
-                    $trace[index as usize],
-                    TraceEntry {
-                        pc: $off_pc,
-                        ap: $off_ap,
-                        fp: $off_fp,
-                    }
-                );
-            )*
-        };
+    #[track_caller]
+    pub(crate) fn trace_check(actual: &[TraceEntry], expected: &[(usize, usize, usize)]) {
+        assert_eq!(actual.len(), expected.len());
+        for (entry, expected) in actual.iter().zip(expected.iter()) {
+            assert_eq!(&(entry.pc, entry.ap, entry.fp), expected);
+        }
     }
-    pub(crate) use trace_check;
 
     macro_rules! exec_scopes_ref {
         () => {
@@ -669,7 +660,7 @@ mod test {
                 fp: 7,
             },
         ];
-        trace_check!(trace, [(2, 7, 1), (5, 1, 0), (9, 5, 7)]);
+        trace_check(&trace, &[(2, 7, 1), (5, 1, 0), (9, 5, 7)]);
     }
 
     #[test]

diff --git a/src/vm/context/run_context.rs b/src/vm/context/run_context.rs
@@ -118,7 +118,6 @@ mod tests {
             off0: 1,
             off1: 2,
             off2: 3,
-            imm: None,
             dst_register: Register::AP,
             op0_register: Register::FP,
             op1_addr: Op1Addr::AP,
@@ -147,7 +146,6 @@ mod tests {
             off0: 1,
             off1: 2,
             off2: 3,
-            imm: None,
             dst_register: Register::FP,
             op0_register: Register::AP,
             op1_addr: Op1Addr::AP,
@@ -177,7 +175,6 @@ mod tests {
             off0: 1,
             off1: 2,
             off2: 3,
-            imm: None,
             dst_register: Register::AP,
             op0_register: Register::AP,
             op1_addr: Op1Addr::AP,
@@ -206,7 +203,6 @@ mod tests {
             off0: 1,
             off1: 2,
             off2: 3,
-            imm: None,
             dst_register: Register::FP,
             op0_register: Register::FP,
             op1_addr: Op1Addr::AP,
@@ -235,7 +231,6 @@ mod tests {
             off0: 1,
             off1: 2,
             off2: 3,
-            imm: None,
             dst_register: Register::FP,
             op0_register: Register::AP,
             op1_addr: Op1Addr::FP,
@@ -264,7 +259,6 @@ mod tests {
             off0: 1,
             off1: 2,
             off2: 3,
-            imm: None,
             dst_register: Register::FP,
             op0_register: Register::AP,
             op1_addr: Op1Addr::AP,
@@ -293,7 +287,6 @@ mod tests {
             off0: 1,
             off1: 2,
             off2: 1,
-            imm: None,
             dst_register: Register::FP,
             op0_register: Register::AP,
             op1_addr: Op1Addr::Imm,
@@ -322,7 +315,6 @@ mod tests {
             off0: 1,
             off1: 2,
             off2: 3,
-            imm: None,
             dst_register: Register::FP,
             op0_register: Register::AP,
             op1_addr: Op1Addr::Imm,
@@ -354,7 +346,6 @@ mod tests {
             off0: 1,
             off1: 2,
             off2: 1,
-            imm: None,
             dst_register: Register::FP,
             op0_register: Register::AP,
             op1_addr: Op1Addr::Op0,
@@ -385,7 +376,6 @@ mod tests {
             off0: 1,
             off1: 2,
             off2: 1,
-            imm: None,
             dst_register: Register::FP,
             op0_register: Register::AP,
             op1_addr: Op1Addr::Op0,
@@ -418,7 +408,6 @@ mod tests {
             off0: 1,
             off1: 2,
             off2: 3,
-            imm: None,
             dst_register: Register::FP,
             op0_register: Register::AP,
             op1_addr: Op1Addr::Op0,