4-stage pipelined RV32I CPU in Verilog

Question

This is a simple 4-stage pipeline that partially implements the RV32I ISA.

All instructions are supported, except jalr, those relating to memory (l*, l*u, s*, fence and fence.i), or system calls (sbreak and scall).

The pipeline stages are more or less the classic RISC ones without the memory access stage (i.e. fetch instruction, decode and fetch operands, calculate result, write result).

My ultimate goal is to have a simple CPU with somewhat decent performance for synthesis to an FPGA (I'd like to reach 150-200MHz eventually). This is the first major hardware design project that I have attempted, so I'm fairly sure that I have made a bunch of beginner mistakes.

`define ALU_ADD   0
`define ALU_SUB   1
`define ALU_AND   2
`define ALU_OR    3
`define ALU_XOR   4
`define ALU_SLL   5
`define ALU_SRL   6
`define ALU_SRA   7
`define ALU_SEQ   8
`define ALU_SNE   9
`define ALU_SLT  10
`define ALU_SGE  11
`define ALU_SLTU 12
`define ALU_SGEU 13

`define OPCODE_OP     7'b0110011
`define OPCODE_OP_IMM 7'b0010011
`define OPCODE_LUI    7'b0110111
`define OPCODE_AUIPC  7'b0010111
`define OPCODE_JAL    7'b1101111
`define OPCODE_JALR   7'b1100111
`define OPCODE_BRANCH 7'b1100011
`define OPCODE_SYSTEM 7'b1110011

`define FUNCT3_ADD_SUB 3'b000
`define FUNCT3_SLL     3'b001
`define FUNCT3_SLT     3'b010
`define FUNCT3_SLTU    3'b011
`define FUNCT3_XOR     3'b100
`define FUNCT3_SRL_SRA 3'b101
`define FUNCT3_OR      3'b110
`define FUNCT3_AND     3'b111

`define FUNCT3_BEQ  3'b000
`define FUNCT3_BNE  3'b001
`define FUNCT3_BLT  3'b100
`define FUNCT3_BGE  3'b101
`define FUNCT3_BLTU 3'b110
`define FUNCT3_BGEU 3'b111

`define SYSTEM_RDCYCLE    20'b11000000000000000010
`define SYSTEM_RDCYCLEH   20'b11001000000000000010
`define SYSTEM_RDTIME     20'b11000000000100000010
`define SYSTEM_RDTIMEH    20'b11001000000100000010
`define SYSTEM_RDINSTRET  20'b11000000001000000010
`define SYSTEM_RDINSTRETH 20'b11001000001000000010

module alu (input [3:0]       operation,
            input [31:0]      s1,
            input [31:0]      s2,
            output reg [31:0] d
            );
   wire [4:0]                 shamt = s2[4:0];
   always @ *
     case(operation)
       `ALU_ADD:  d = s1 + s2;
       `ALU_SUB:  d = s1 - s2;
       `ALU_AND:  d = s1 & s2;
       `ALU_OR:   d = s1 | s2;
       `ALU_XOR:  d = s1 ^ s2;
       `ALU_SLL:  d = s1 << shamt;
       `ALU_SRL:  d = s1 >> shamt;
       `ALU_SRA:  d = $signed(s1) >>> shamt;
       `ALU_SEQ:  d = s1 == s2 ? 1 : 0;
       `ALU_SNE:  d = s1 == s2 ? 0 : 1;
       `ALU_SLT:  d = $signed(s1) < $signed(s2) ? 1 : 0;
       `ALU_SGE:  d = $signed(s1) < $signed(s2) ? 0 : 1;
       `ALU_SLTU: d = s1 < s2 ? 1 : 0;
       `ALU_SGEU: d = s1 < s2 ? 0 : 1;
       default:   d = 0;
     endcase
endmodule

module decoder(input [31:0]      insn,
               input [31:0]      pc,
               input [63:0]      cycle,
               input [63:0]      instret,

               output [4:0]      rd,
               output reg        s1_is_imm,
               output [4:0]      rs1,
               output reg [31:0] s1_imm,
               output reg        s2_is_imm,
               output [4:0]      rs2,
               output reg [31:0] s2_imm,
               output reg [3:0]  op_alu,
               output reg        is_jump,
               output reg        is_branch,
               output [31:0]     jump_target
               );
   wire [6:0]                    opcode = insn[ 6: 0];
   wire [2:0]                    funct3 = insn[14:12];
   wire [6:0]                    funct7 = insn[31:25];
   wire [31:0]                   imm12  = {{21{insn[31]}}, insn[30:20]};
   wire [31:0]                   imm20  = {insn[31:12], 12'b0};
   wire [31:0]                   imm12b = {{20{insn[31]}}, insn[7], insn[30:25], insn[11:8], 1'b0};
   wire [31:0]                   imm20j = {{12{insn[31]}}, insn[19:12], insn[20], insn[30:21], 1'b0};
   reg                           rd_write_disable;

   assign rd = rd_write_disable ? 0 : insn[11:7];
   assign rs2 = insn[24:20];
   assign rs1 = insn[19:15];
   assign jump_target = pc + (is_branch ? imm12b : imm20j);

   always @ * begin
      rd_write_disable = 0;
      s1_imm = 0;
      s1_is_imm = 0;
      s2_imm = 0;
      s2_is_imm = 0;
      is_jump = 0;
      is_branch = 0;
      op_alu = `ALU_ADD;
      case(opcode)
        `OPCODE_OP: begin
           case(funct3)
             `FUNCT3_ADD_SUB: op_alu = funct7[5] ? `ALU_SUB : `ALU_ADD;
             `FUNCT3_SLL:     op_alu = `ALU_SLL;
             `FUNCT3_SLT:     op_alu = `ALU_SLT;
             `FUNCT3_SLTU:    op_alu = `ALU_SLTU;
             `FUNCT3_XOR:     op_alu = `ALU_XOR;
             `FUNCT3_SRL_SRA: op_alu = funct7[5] ? `ALU_SRA : `ALU_SRL;
             `FUNCT3_OR:      op_alu = `ALU_OR;
             `FUNCT3_AND:     op_alu = `ALU_AND;
           endcase
        end
        `OPCODE_OP_IMM: begin
           s2_imm = imm12;
           s2_is_imm = 1;
           case(funct3)
             `FUNCT3_ADD_SUB: op_alu = `ALU_ADD;
             `FUNCT3_SLT:     op_alu = `ALU_SLT;
             `FUNCT3_SLTU:    op_alu = `ALU_SLTU;
             `FUNCT3_XOR:     op_alu = `ALU_XOR;
             `FUNCT3_OR:      op_alu = `ALU_OR;
             `FUNCT3_AND:     op_alu = `ALU_AND;
             `FUNCT3_SLL:     op_alu = `ALU_SLL;
             `FUNCT3_SRL_SRA: op_alu = funct7[5] ? `ALU_SRA : `ALU_SRL;
           endcase
        end
        `OPCODE_LUI: begin
           s1_imm = imm20;
           s1_is_imm = 1;
           s2_is_imm = 1;
        end
        `OPCODE_AUIPC: begin
           s1_imm = imm20;
           s1_is_imm = 1;
           s2_imm = pc;
           s2_is_imm = 1;
        end
        `OPCODE_JAL: begin
           s1_is_imm = 1;
           s1_imm = pc;
           s2_is_imm = 1;
           s2_imm = 4;
           is_jump = 1;
        end
        // `OPCODE_JALR: TODO
        `OPCODE_BRANCH: begin
           is_jump = 1;
           is_branch = 1;
           rd_write_disable = 1;
           case(funct3)
             `FUNCT3_BEQ:  op_alu = `ALU_SEQ;
             `FUNCT3_BNE:  op_alu = `ALU_SNE;
             `FUNCT3_BLT:  op_alu = `ALU_SLT;
             `FUNCT3_BGE:  op_alu = `ALU_SGE;
             `FUNCT3_BLTU: op_alu = `ALU_SLTU;
             `FUNCT3_BGEU: op_alu = `ALU_SGEU;
           endcase
        end
        `OPCODE_SYSTEM: begin
           s1_is_imm = 1;
           s2_is_imm = 1;
           case(insn[31:12])
             `SYSTEM_RDCYCLE:    s1_imm = cycle[31:0];
             `SYSTEM_RDCYCLEH:   s1_imm = cycle[63:32];
             `SYSTEM_RDTIME:     s1_imm = cycle[31:0];
             `SYSTEM_RDTIMEH:    s1_imm = cycle[63:32];
             `SYSTEM_RDINSTRET:  s1_imm = instret[31:0];
             `SYSTEM_RDINSTRETH: s1_imm = instret[63:32];
           endcase
        end
      endcase
   end
endmodule

module ezpipe (input         clk,
               input         reset,
               output [31:0] ibus_addr,
               input [31:0]  ibus_data
               // output reg [31:0] dbus_addr,
               // output reg [31:0] dbus_data_wr,
               // input [31:0]      dbus_data_rd,
               // input [31:0]      dbus_data_ready,
               // output reg        dbus_rd,
               // output reg        dbus_wr
               );
   /* registers and counters */
   reg [31:0]                regs [1:31];
   reg [31:0]                pc;
   reg [63:0]                cycle;
   reg [63:0]                instret;
   // There is no counter for RDTIME/RDTIMEH, those instructions just use the cycle register.

   /* pipeline registers */
   // from FETCH to DECODE
   reg [31:0]                f_insn;
   reg [31:0]                f_pc;
   reg                       f_valid;
   // from DECODE to EXECUTE
   reg [31:0]                d_s1;
   reg [31:0]                d_s2;
   reg [3:0]                 d_op_alu;
   reg [4:0]                 d_rd;
   reg                       d_is_jump;
   reg                       d_is_branch;
   reg [31:0]                d_jump_target;
   reg                       d_valid;
   // from EXECUTE to WRITE
   reg [4:0]                 e_rd;
   reg [31:0]                e_d;
   reg                       e_is_jump;
   reg                       e_is_branch;
   reg [31:0]                e_jump_target;
   reg                       e_valid;

   /* instances */
   wire [4:0]                dec_rd;
   wire [4:0]                dec_rs1;
   wire [31:0]               dec_s1_imm;
   wire                      dec_s1_is_imm;
   wire [4:0]                dec_rs2;
   wire [31:0]               dec_s2_imm;
   wire                      dec_s2_is_imm;
   wire [3:0]                dec_op_alu;
   wire                      dec_is_jump;
   wire                      dec_is_branch;
   wire [31:0]               dec_jump_target;
   decoder dec(.pc(f_pc),
               .insn(f_insn),
               .cycle(cycle),
               .instret(instret),
               .op_alu(dec_op_alu),
               .rd(dec_rd),
               .rs1(dec_rs1),
               .s1_is_imm(dec_s1_is_imm),
               .s1_imm(dec_s1_imm),
               .rs2(dec_rs2),
               .s2_is_imm(dec_s2_is_imm),
               .s2_imm(dec_s2_imm),
               .is_jump(dec_is_jump),
               .is_branch(dec_is_branch),
               .jump_target(dec_jump_target)
               );

   wire [31:0]               alu_d;
   alu alu(.s1(d_s1),
           .s2(d_s2),
           .operation(d_op_alu),
           .d(alu_d)
           );

   assign ibus_addr = pc;

   /* the actual pipeline */
   reg                       jumping;
   reg                       stall;
   always @ * begin
      // does the decoded instruction depend on a instruction in the d_* or e_* registers?
      stall = 0;
      if(d_valid && |d_rd) begin
         if(|dec_rs1 && !dec_s1_is_imm && dec_rs1==d_rd)
           stall = 1;
         if(|dec_rs2 && !dec_s2_is_imm && dec_rs2==d_rd)
           stall = 1;
      end
      if(e_valid && |e_rd) begin
         if(|dec_rs1 && !dec_s1_is_imm && dec_rs1==e_rd)
           stall = 1;
         if(|dec_rs2 && !dec_s2_is_imm && dec_rs2==e_rd)
           stall = 1;
      end
      // is there a taken branch/jump sitting in the e_* registers?
      jumping = 0;
      if(e_valid)
        if(e_is_jump) begin
           if(e_is_branch)
             jumping = e_d[0];
           else
             jumping = 1;
        end
   end

   always @(posedge clk) begin
      if(reset) begin
         pc <= 0;
         f_valid <= 0;
         d_valid <= 0;
         e_valid <= 0;
         cycle <= 0;
         instret <= 0;
      end else begin
         cycle <= cycle + 1;

         /* FETCH */
         f_valid <= !jumping;
         if(!stall) begin
            f_insn <= ibus_data;
            f_pc <= pc;
            pc <= pc + 4;
         end else begin
            // don't fetch a new instruction when we can't complete the one in the D stage
         end

         /* DECODE */
         if(!stall) begin
            // fetch operands
            if(dec_s1_is_imm) d_s1 <= dec_s1_imm;
            else              d_s1 <= |dec_rs1 ? regs[dec_rs1] : 0;
            if(dec_s2_is_imm) d_s2 <= dec_s2_imm;
            else              d_s2 <= |dec_rs2 ? regs[dec_rs2] : 0;
            // store decoded instruction
            d_rd <= dec_rd;
            d_op_alu <= dec_op_alu;
            d_jump_target <= dec_jump_target;
            d_is_branch <= dec_is_branch;
            d_is_jump <= dec_is_jump;
            d_valid <= f_valid && !jumping;
         end else begin
            // can't issue this instruction yet; send a bubble down the pipeline
            d_valid <= 0;
         end

         /* EXECUTE */
         // store ALU result
         e_d <= alu_d;
         // send remaining info down the pipeline
         e_rd <= d_rd;
         e_is_jump <= d_is_jump;
         e_is_branch <= d_is_branch;
         e_jump_target <= d_jump_target;
         e_valid <= d_valid && !jumping;

         /* WRITE */
         if(e_valid) begin
            if(jumping)
              pc <= e_jump_target;
            if(|e_rd)
              regs[e_rd] <= e_d;
            instret <= instret + 1;
         end
      end
   end
endmodule

My main questions are:

• Have I made any rookie mistakes or committed major sins against Verilog style?
• Are there any ways I can improve the maximum clock/overall performance, without complicating the code too much?
• Will explicitly using the undefined value (x) in places where the values do not matter actually help the synthesis tool to generate less logic? (An example would be the default case in the ALU.)

Answer 1

• Have I made any rookie mistakes or committed major sins against Verilog style?

Overall it is coded very well and easy to read, no Verilog sins (unlikely to synthesize if there were). Clean Verilog-2001 syntax utilizing ANSI style header and @*.

The only potential error I could spot (without building a testbench) is with f_pc, regs, e_*, and most d_* registers are not assigned in the reset condition. On FPGA this will typically initialize to 0, but will not be reset if reset comes any time later. Typically flops with resets and flops without reset are assigned in separate always blocks.

To make live a little easier with accidental missing resets, there is an Emacs has a plug-in called Verilog-mode which can generate reset assignments with /*AUTORESET*/; as well as other expansion features. Vim can utilize it to with wrapper script; something similar may exist for other editors.

I would suggest making sure all numeric literals have explicit width and radix (ex the `ALU_* values should start with 4'd, cycle <= cycle + 1'b1;, pc <= pc + 4'd4). It will not change anything but can reduce warning (especially in lint tools).

• Are there any ways I can improve the maximum clock/overall performance, without complicating the code too much?

Look are the timing report to get an idea where the bottleneck(s) are.

If the bottleneck is related to decoding the muxes, then consider one-hot parallel decoding. This will require more gates but can save timing.

If the bottleneck is related to some heavy computation, then consider moving some of the logic to an earlier stage; having the data ready even if it will ignored. This will also take up more gates. It is also likely to make the code more complicated then intended, but if needed then it is needed.

There is a point of diminishing return and more tweaks can become departmental. Adding too much logic make routing more challenging which can also impact timing/performance. And if the design gets to big, it won't fix on the FPGA. The synthesis report should give some clues to this.

• Will explicitly using the undefined value (x) in places where the values do not matter actually help the synthesis tool to generate less logic? (An example would be the default case in the ALU.)

It sometimes can, but in my experience it is can cause more challenges then benefits. As the X propagates in simulation, it will eval as false in an condition statements. There is no X in hardware, it will be seen as 1 or 0, so it could take a different branch when evaluated in any condition. There are X propagation simulation tools/add-ons/plug-ins that can help, but they cost money.

If the testbench is robust then randomization could be used an X-prop alternative (ex: d = `ifdef SYNTHESIS 32'dx `else $random(...) `endif ;).

Assigning it to a known value normally doesn't have negative impact and makes debugging a bit easier.

---

Other comments:

Consider a two-always block coding style by keeping the synchronous assignment simple and moving the algorithmic logic for FETCH, DECODE, EXECUTE, and WRITE into a combinational block. This would separating the present state and next state values. It is a bit of this is personal choice and the opinion of person you were taught by (as well as the teacher of the teacher). This paper by Cliff Cummings (as well as other papers) was a major influence for my coding style and many of my colleagues.

Consider enabling SystemVerilog if your FPGA supports it. Use a package and enums replace the macros (macros have change of name collision with bigger projects, especally when using code from other people). Be more explicit with intention by always_ff and always_comb. Part of the decoder could be simplified using structs and unions.