refactored from() method for MemoryOp array impl (#591)

* refactored from() method for MemoryOp array impl * removed instruction type code * removed duplicate RV32IM::JAL instruction * ran cargo fmt * removed instruction_type variable * removed duplicate SW and replaced with SB
a16z · Feb 20, 2025 · c93148d · c93148d
1 parent 783da5d
commit c93148d
Showing 1 changed file with 91 additions and 139 deletions.
diff --git a/common/src/rv_trace.rs b/common/src/rv_trace.rs
@@ -45,8 +45,6 @@ fn sum_u64_i32(a: u64, b: i32) -> u64 {
 
 impl From<&RVTraceRow> for [MemoryOp; MEMORY_OPS_PER_INSTRUCTION] {
     fn from(val: &RVTraceRow) -> Self {
-        let instruction_type = val.instruction.opcode.instruction_type();
-
         let rs1_read = || MemoryOp::Read(val.instruction.rs1.unwrap());
         let rs2_read = || MemoryOp::Read(val.instruction.rs2.unwrap());
         let rd_write = || {
@@ -85,74 +83,112 @@ impl From<&RVTraceRow> for [MemoryOp; MEMORY_OPS_PER_INSTRUCTION] {
         // 6: byte_3
         // If any are empty a no_op is inserted.
 
-        // Validation: Number of ops should be a multiple of 7
-        match instruction_type {
-            RV32InstructionFormat::R => [rs1_read(), rs2_read(), rd_write(), MemoryOp::noop_read()],
-            RV32InstructionFormat::U => [
+        match val.instruction.opcode {
+            RV32IM::ADD
+            | RV32IM::SUB
+            | RV32IM::XOR
+            | RV32IM::OR
+            | RV32IM::AND
+            | RV32IM::SLL
+            | RV32IM::SRL
+            | RV32IM::SRA
+            | RV32IM::SLT
+            | RV32IM::SLTU
+            | RV32IM::MUL
+            | RV32IM::MULH
+            | RV32IM::MULHU
+            | RV32IM::MULHSU
+            | RV32IM::MULU
+            | RV32IM::DIV
+            | RV32IM::DIVU
+            | RV32IM::REM
+            | RV32IM::REMU => [rs1_read(), rs2_read(), rd_write(), MemoryOp::noop_read()],
+
+            RV32IM::LUI | RV32IM::AUIPC | RV32IM::VIRTUAL_ADVICE => [
                 MemoryOp::noop_read(),
                 MemoryOp::noop_read(),
                 rd_write(),
                 MemoryOp::noop_read(),
             ],
-            RV32InstructionFormat::I => match val.instruction.opcode {
-                RV32IM::VIRTUAL_ASSERT_HALFWORD_ALIGNMENT => [
-                    rs1_read(),
-                    MemoryOp::noop_read(),
-                    MemoryOp::noop_write(),
-                    MemoryOp::noop_read(),
-                ],
-                RV32IM::ADDI
-                | RV32IM::SLLI
-                | RV32IM::SRLI
-                | RV32IM::SRAI
-                | RV32IM::ANDI
-                | RV32IM::ORI
-                | RV32IM::XORI
-                | RV32IM::SLTI
-                | RV32IM::SLTIU
-                | RV32IM::JALR
-                | RV32IM::VIRTUAL_MOVE
-                | RV32IM::VIRTUAL_MOVSIGN => [
-                    rs1_read(),
-                    MemoryOp::noop_read(),
-                    rd_write(),
-                    MemoryOp::noop_read(),
-                ],
-                RV32IM::LW => [
-                    rs1_read(),
-                    MemoryOp::noop_read(),
-                    rd_write(),
-                    MemoryOp::Read(rs1_offset()),
-                ],
-                RV32IM::FENCE => [
-                    MemoryOp::noop_read(),
-                    MemoryOp::noop_read(),
-                    MemoryOp::noop_write(),
-                    MemoryOp::noop_read(),
-                ],
-                _ => unreachable!("{val:?}"),
-            },
-            RV32InstructionFormat::S => match val.instruction.opcode {
-                RV32IM::SW => [
-                    rs1_read(),
-                    rs2_read(),
-                    MemoryOp::noop_write(),
-                    MemoryOp::Write(rs1_offset(), ram_write_value()),
-                ],
-                _ => unreachable!("{val:?}"),
-            },
-            RV32InstructionFormat::UJ => [
+
+            RV32IM::VIRTUAL_ASSERT_HALFWORD_ALIGNMENT => [
+                rs1_read(),
                 MemoryOp::noop_read(),
+                MemoryOp::noop_write(),
+                MemoryOp::noop_read(),
+            ],
+
+            RV32IM::ADDI
+            | RV32IM::SLLI
+            | RV32IM::SRLI
+            | RV32IM::SRAI
+            | RV32IM::ANDI
+            | RV32IM::ORI
+            | RV32IM::XORI
+            | RV32IM::SLTI
+            | RV32IM::SLTIU
+            | RV32IM::JALR
+            | RV32IM::VIRTUAL_MOVE
+            | RV32IM::VIRTUAL_MOVSIGN => [
+                rs1_read(),
+                MemoryOp::noop_read(),
+                rd_write(),
+                MemoryOp::noop_read(),
+            ],
+
+            RV32IM::LW => [
+                rs1_read(),
                 MemoryOp::noop_read(),
                 rd_write(),
+                MemoryOp::Read(rs1_offset()),
+            ],
+            RV32IM::FENCE => [
+                MemoryOp::noop_read(),
+                MemoryOp::noop_read(),
+                MemoryOp::noop_write(),
                 MemoryOp::noop_read(),
             ],
-            RV32InstructionFormat::SB => [
+
+            RV32IM::SB | RV32IM::SH | RV32IM::SW => [
+                rs1_read(),
+                rs2_read(),
+                MemoryOp::noop_write(),
+                MemoryOp::Write(rs1_offset(), ram_write_value()),
+            ],
+
+            // RV32IM::LB | RV32IM::LH | RV32IM::LBU | RV32IM::LHU => [
+            RV32IM::JAL => [
+                MemoryOp::noop_read(),
+                MemoryOp::noop_read(),
+                rd_write(),
+                MemoryOp::noop_read(),
+            ],
+
+            RV32IM::BEQ
+            | RV32IM::BNE
+            | RV32IM::BLT
+            | RV32IM::BGE
+            | RV32IM::BLTU
+            | RV32IM::BGEU
+            | RV32IM::VIRTUAL_ASSERT_EQ
+            | RV32IM::VIRTUAL_ASSERT_LTE
+            | RV32IM::VIRTUAL_ASSERT_VALID_DIV0
+            | RV32IM::VIRTUAL_ASSERT_VALID_SIGNED_REMAINDER
+            | RV32IM::VIRTUAL_ASSERT_VALID_UNSIGNED_REMAINDER => [
                 rs1_read(),
                 rs2_read(),
                 MemoryOp::noop_write(),
                 MemoryOp::noop_read(),
             ],
+
+            RV32IM::ECALL => [
+                MemoryOp::noop_read(),
+                MemoryOp::noop_read(),
+                MemoryOp::noop_write(),
+                MemoryOp::Write(rs1_offset(), ram_write_value()),
+            ],
+
+            _ => unreachable!("{val:?}"),
         }
     }
 }
@@ -488,90 +524,6 @@ impl FromStr for RV32IM {
     }
 }
 
-#[derive(Debug, PartialEq)]
-pub enum RV32InstructionFormat {
-    R,
-    I,
-    S,
-    SB,
-    U,
-    UJ,
-}
-
-impl RV32IM {
-    #[rustfmt::skip] // keep matches pretty
-    pub fn instruction_type(&self) -> RV32InstructionFormat {
-        match self {
-            RV32IM::ADD    |
-            RV32IM::SUB    |
-            RV32IM::XOR    |
-            RV32IM::OR     |
-            RV32IM::AND    |
-            RV32IM::SLL    |
-            RV32IM::SRL    |
-            RV32IM::SRA    |
-            RV32IM::SLT    |
-            RV32IM::SLTU   |
-            RV32IM::MUL    |
-            RV32IM::MULH   |
-            RV32IM::MULHU  |
-            RV32IM::MULHSU |
-            RV32IM::MULU   |
-            RV32IM::DIV    |
-            RV32IM::DIVU   |
-            RV32IM::REM    |
-            RV32IM::REMU => RV32InstructionFormat::R,
-
-            RV32IM::ADDI         |
-            RV32IM::XORI         |
-            RV32IM::ORI          |
-            RV32IM::ANDI         |
-            RV32IM::SLLI         |
-            RV32IM::SRLI         |
-            RV32IM::SRAI         |
-            RV32IM::SLTI         |
-            RV32IM::FENCE        |
-            RV32IM::SLTIU        |
-            RV32IM::VIRTUAL_MOVE |
-            RV32IM::VIRTUAL_ASSERT_HALFWORD_ALIGNMENT |
-            RV32IM::VIRTUAL_MOVSIGN => RV32InstructionFormat::I,
-
-            RV32IM::LB  |
-            RV32IM::LH  |
-            RV32IM::LW  |
-            RV32IM::LBU |
-            RV32IM::LHU |
-            RV32IM::JALR => RV32InstructionFormat::I,
-
-            RV32IM::SB |
-            RV32IM::SH |
-            RV32IM::SW => RV32InstructionFormat::S,
-
-            RV32IM::BEQ  |
-            RV32IM::BNE  |
-            RV32IM::BLT  |
-            RV32IM::BGE  |
-            RV32IM::BLTU |
-            RV32IM::BGEU |
-            RV32IM::VIRTUAL_ASSERT_EQ     |
-            RV32IM::VIRTUAL_ASSERT_LTE    |
-            RV32IM::VIRTUAL_ASSERT_VALID_DIV0    |
-            RV32IM::VIRTUAL_ASSERT_VALID_SIGNED_REMAINDER    |
-            RV32IM::VIRTUAL_ASSERT_VALID_UNSIGNED_REMAINDER => RV32InstructionFormat::SB,
-
-            RV32IM::LUI   |
-            RV32IM::AUIPC |
-            RV32IM::VIRTUAL_ADVICE=> RV32InstructionFormat::U,
-
-            RV32IM::JAL => RV32InstructionFormat::UJ,
-
-            RV32IM::ECALL  |
-            RV32IM::EBREAK |
-            RV32IM::UNIMPL => unimplemented!(),
-        }
-    }
-}
-
 #[allow(clippy::too_long_first_doc_paragraph)]
 /// Represented as a "peripheral device" in the RISC-V emulator, this captures
 /// all reads from the reserved memory address space for program inputs and all writes