Proposal for Decoder Improvement: Enhancing Readability and Maintainability using `package` and `case inside`

[dokunira]

I have reviewed the implementation of rvcore.

I would like to propose an improvement for the decoder implementation in rvcore_simple.sv and instructions_defines.svh that could further enhance future maintainability.

1. Current Architecture

• In instructions_defines.svh, instruction opcodes and funct3/7 are defined as `define macros.
• Helper macros like IS_ADD(inst) are also defined using `define.
• The decoder logic in rvcore_simple.sv is implemented as a long chain of if-else if statements using these macros.

2. Potential Issues

While this approach works, there are several potential issues:

• Readability and Maintainability: A long if-else chain makes it difficult to see the correspondence between instructions and control signals at a glance, increasing the risk of errors when adding or modifying instructions.
• Global Scope Pollution: `define is a simple text substitution without file scope. This can cause unintended name collisions.
• Lack of Type Safety: Macros are typeless, so they do not benefit from SystemVerilog's strict type checking.

3. Improvement Proposal: Utilizing package and case inside

These issues can be resolved by using SystemVerilog's package, localparam, and case inside.

Step 1: Centralize instruction patterns AND control definitions in packages

Migrate the contents of instructions_defines.svh and control_defines.svh to packages. This unifies the management of constants and improves type safety for control signals.

1. riscv_patterns_pkg: Define "Don't Care" fields and instruction patterns.
2. riscv_ctrl_pkg: Convert control signal definitions (ALU ops, mux selectors) from macros to typedef enum or typed parameters.

(Example 1) riscv_patterns_pkg.sv (from instructions_defines.svh)

package riscv_patterns_pkg;

  // Opcodes
  localparam logic [6:0] OPC_OP     = 7'b0110011;
  localparam logic [6:0] OPC_LOAD   = 7'b0000011;
  // ... Define other opcodes similarly

  // funct3
  localparam logic [2:0] F3_ADD_SUB = 3'b000;
  localparam logic [2:0] F3_LW      = 3'b010;
  // ... Define other funct3 similarly
  
  // funct7
  localparam logic [6:0] F7_ADD     = 7'b0000000;
  localparam logic [6:0] F7_SUB     = 7'b0100000;
  // ... Define other funct7 similarly

  // --- Wildcard Constants ---
  localparam logic [4:0]  RD_X    = 5'b?????;
  localparam logic [4:0]  RS1_X   = 5'b?????;
  localparam logic [4:0]  RS2_X   = 5'b?????;
  localparam logic [11:0] IMM12_X = 12'b????????????;
  localparam logic [19:0] IMM20_X = 20'b????????????????????;

  // --- Instruction Pattern Definitions ---

  // R-Type: {funct7, rs2, rs1, funct3, rd, opcode}
  localparam logic [31:0] PAT_ADD = {F7_ADD, RS2_X, RS1_X, F3_ADD_SUB, RD_X, OPC_OP};
  localparam logic [31:0] PAT_SUB = {F7_SUB, RS2_X, RS1_X, F3_ADD_SUB, RD_X, OPC_OP};
  // ... Define other R-Type patterns similarly

  // I-Type: {imm[11:0], rs1, funct3, rd, opcode}
  localparam logic [31:0] PAT_LW  = {IMM12_X, RS1_X, F3_LW, RD_X, OPC_LOAD};
  // ... Define other I-Type patterns similarly

  // S-Type
  // ... Define other S-Type patterns similarly

  // B-Type
  // ... Define other B-Type patterns similarly

  // U-Type
  // ... Define other U-Type patterns similarly

  // J-Type
  // ... Define other J-Type patterns similarly

  // System
  // ... Define System instruction patterns similarly

endpackage

(Example 2) riscv_ctrl_pkg.sv (from control_defines.svh)

package riscv_ctrl_pkg;
  // ALU Operations (Typed enum instead of macros)
  typedef enum logic [4:0] {
    ALU_X   = 5'd0,
    ALU_ADD = 5'd1,
    ALU_SUB = 5'd2
    // ...
  } alu_op_t;

  // Operand 1 Selectors
  typedef enum logic [1:0] {
    OP1_RS1 = 2'b00,
    OP1_PC  = 2'b01
    // ...
  } op1_sel_t;
  
  // ... Define other control signals similarly
endpackage

Step 2: Rewrite the decoder using case inside

Rewrite the decoder logic (always_comb) in rvcore_simple.sv using case inside.

(Example) Decoder logic in rvcore_simple.sv

import riscv_patterns_pkg::*; // Import patterns
import riscv_ctrl_pkg::*;     // Import control types

always_comb begin
  // Default values (reset all signals)
  exe_fun = ALU_X;
  op1_sel = OP1_X;
  op2_sel = OP2_X;
  mem_wen = MEN_X;
  rf_wen  = REN_X;
  wb_sel  = WB_X;
  csr_cmd = CSR_X;

  // Decode instructions using case inside
  case (inst) inside
    
    // R-Type
    PAT_ADD: begin
      exe_fun = ALU_ADD;
      op1_sel = OP1_RS1;
      op2_sel = OP2_RS2;
      mem_wen = MEN_X;
      rf_wen  = REN_S;
      wb_sel  = WB_ALU;
      csr_cmd = CSR_X;
    end
    PAT_SUB: begin
      exe_fun = ALU_SUB;
      op1_sel = OP1_RS1;
      op2_sel = OP2_RS2;
      mem_wen = MEN_X;
      rf_wen  = REN_S;
      wb_sel  = WB_ALU;
      csr_cmd = CSR_X;
    end
    // ... Add other R-Type instructions similarly

    // I-Type
    PAT_LW: begin
      exe_fun = ALU_ADD;
      op1_sel = OP1_RS1;
      op2_sel = OP2_IMI;
      mem_wen = MEN_X;
      rf_wen  = REN_S;
      wb_sel  = WB_MEM;
      csr_cmd = CSR_X;
    end
    // ... Add other I-Type instructions similarly

    // ... Add all other instruction patterns similarly ...

    default: begin
      // Explicit default values (Undefined instructions or NOP)
      exe_fun = ALU_X;
      op1_sel = OP1_X;
      op2_sel = OP2_X;
      mem_wen = MEN_X;
      rf_wen  = REN_X;
      wb_sel  = WB_X;
      csr_cmd = CSR_X;
    end
  endcase
end

4. Expected Benefits

• Improved Readability: Using case inside provides a clear, table-like view of the correspondence between instruction patterns and control signals.
• Improved Safety: Eliminates scope pollution caused by `define and allows SystemVerilog's type checking to function more strictly.

I hope this refactoring proposal serves as a useful reference for the future development of rvcore.