RISCV ALU Implementation in SystemVerilog

Question

This will be first of many things I will submit during my journey of self (re)learning CPU design.

I have ALU implementation for RiscV base instruction. I am not fully sure what I am looking for, but that can include one or more of the following:

• Anything that is a big "No No!"
• Anything I did that is not idiomatic SystemVerilog.
• Anything that most SystemVerilog users would add to their workflow but I didn't.
• Anything related to performance that I am not aware of.
• Anything you would do differently for any reason.

So my ALU is split into basically 4 files.

The rtl/alu.sv

parameter int AluOpWidth = 4;
typedef enum logic [AluOpWidth-1:0]  {
    Add     = AluOpWidth'(0),
    Slt     = AluOpWidth'(1),
    SltU    = AluOpWidth'(2),
    BitAnd  = AluOpWidth'(3),
    BitOr   = AluOpWidth'(4),
    BitXor  = AluOpWidth'(5),
    Sll     = AluOpWidth'(6),
    Slr     = AluOpWidth'(7),
    Sub     = AluOpWidth'(8),
    Sra     = AluOpWidth'(9)
 } AluOps;

module alu#(parameter int XLEN = 32) (
    input AluOps op,
    input logic [XLEN-1: 0]lhs,
    input logic [XLEN-1: 0]rhs,
    output logic [XLEN-1: 0] out
    );

    // signed variant of lhs
    logic signed [XLEN-1: 0]lhsi;
    // signed variant of rhs
    logic signed [XLEN-1: 0]rhsi;

    // get Signed variant of all inputs
    assign rhsi = rhs;
    assign lhsi = lhs;

    always_comb begin
        unique case(op)
        Add: out = lhs + rhs;
        Slt: out = {{XLEN-1{1'b0}}, (lhsi < rhsi)};
        SltU: out = {{XLEN-1{1'b0}}, (lhs < rhs)};
        BitAnd: out = lhs & rhs;
        BitOr: out = lhs | rhs;
        BitXor: out = lhs ^ rhs;
        Sll: out = lhs << rhs;
        Slr: out = lhs >> rhs;
        Sub: out = lhs - rhs;
        Sra: out = lhsi >>> rhs;
        default: out = {XLEN{1'b0}};
        endcase
    end
endmodule

The test benchmark tb/alu.tb.sv

module alu_tb;
    parameter XLEN = 8;
    logic [XLEN-1: 0]lhs, rhs, expected, out;
    AluOps op;
    alu#(.XLEN(XLEN)) dut(op, lhs, rhs, out);
    integer file, ret;
    initial begin
        $dumpfile("alu.vcd");
        $dumpvars(0, alu_tb);
        file = $fopen("test_vectors.txt", "r");
        if (file == 0) begin
            $display("Error: Could not open test_vectors.txt");
            $fatal;
        end
        while (!$feof(file)) begin
            ret = $fscanf(file, "%h %h %h %h\n", op, lhs, rhs, expected);
            if (ret != 4) begin
                $display("Invalid test vector format");
                $fatal;
            end
            #10;
            if (expected !== out) begin
                $display("Test Failed: op = %h, lhs = %h, rhs = %h, expected = %h, out = %h",
                    op, lhs, rhs, expected, out);
                $fatal;
            end else begin
                $display("Test passed: op = %h, lhs = %h, rhs = %h, expected = %h",
                    op, lhs, rhs, out);
            end
        end
        $finish;
    end
endmodule

The alu.core file:

CAPI=2:
name: learning::alu
description: General Purpose Parametrizable ALU

filesets:
  rtl:
    files:
      - rtl/alu.sv
    file_type: systemVerilogSource
  tb:
    files:
      - tb/alu.tb.sv
    file_type: systemVerilogSource
  gen_tests:
    files: [tb/gen_tests.py : {file_type : user, copyto : gen_tests.py}]

targets:
  default: &default
    filesets: [rtl]
  lint:
    <<: *default
    default_tool : verilator
    filesets_append: [tb]
    tools:
      verilator :
         mode : lint-only
    toplevel : alu
  sim:
    <<: *default
    description: Simulate the design
    default_tool: icarus
    filesets_append: [tb, gen_tests]
    toplevel: alu_tb
    hooks:
      pre_run: [gen_tests]
    tools:
      icarus:
        iverilog_options:
          - -g2012
scripts:
  gen_tests:
    cmd : ["./gen_tests.py"]

And: ./gen_tests.py which basically exhaust the whole search space for 8 bits ALU. This file does not really matter. It will generate file that looks like this:

0 0 0 0
0 0 1 1
0 0 2 2
...
...
8 d2 4d 85
...
...
9 ff fe ff
9 ff ff ff

Answer 1

Overview

You did a good job of partitioning the code into design and testbench modules. The code layout is good, with consistent use of indentation and descriptive identifiers. It is great you made use of parameters and enumerated types for constants.

Design

It is great you used ANSI-style port declarations to avoid unnecessary repetitive port lists.

You also take advantage of the features offered by the always_comb keyword:

• clearly shows the intent of the design
• extra automatic checks

XLEN is not very descriptive. I suggest either changing it to something more meaningful or adding a comment to explain what it means.

It is customary for parameters to use all capital letters. For example, change:

AluOpWidth

to:

ALU_OP_WIDTH

The same applies for enums:

ADD
SLT
etc.

The case-items are typically indented one level inside a case statement:

always_comb begin
    unique case(op)
        Add     : out = lhs + rhs;
        Slt     : out = {{XLEN-1{1'b0}}, (lhsi < rhsi)};
        SltU    : out = {{XLEN-1{1'b0}}, (lhs < rhs)};
        BitAnd  : out = lhs & rhs;
        BitOr   : out = lhs | rhs;
        BitXor  : out = lhs ^ rhs;
        Sll     : out = lhs << rhs;
        Slr     : out = lhs >> rhs;
        Sub     : out = lhs - rhs;
        Sra     : out = lhsi >>> rhs;
        default : out = {XLEN{1'b0}};
    endcase
end

I also aligned the colons to make the code a little easier to understand.

Refer to this unique case default discussion. It is not necessary to use a default there, according to IEEE Std 1800-2023, section 12.5.3 unique-case, unique0-case, and priority-case which has a detailed description of unique case. Here is one quote:

NOTE — By specifying unique or priority, it is not necessary to code a default case to trap unexpected case values.

Testbench

It is great that the code is self-checking.

I suggest using port connections-by-name for the dut instance:

alu #(.XLEN(XLEN)) dut (
   .lhs  (lhs),
   .rhs  (rhs),
   .out  (out),
   .op   (op)
);

While it is more verbose, it avoids one of the most common Verilog errors.

You could simplify the code by combining $display and $fatal statements:

        $display("Error: Could not open test_vectors.txt");
        $fatal;

into a single statement:

        $fatal(2, "Error: Could not open test_vectors.txt");

You could replace the "passed" $display with $info. The advantage is that you will see the simulation time automatically, which is helpful for debugging.

I suggest replacing $fatal with $error for the "Failed" message. It is common to allow the simulation to continue beyond the first failure. It is also common to use a plusarg option to control the number of allowed errors via the simulation command line.

It looks like your input test_vectors.txt file format matches the standard $readmemh file input format. Generally, $readmemh is simpler than $fopen, $fscan, etc., although you would not have the fine-grained input checking you have in your code.

Consider using the 2-state bit type instead of the 4-state logic type for the dut instance input signals:

bit [XLEN-1: 0] lhs, rhs;

There is typically no need to drive inputs as x or z, and the 2-state types can offer simulation performance benefits as the design and testbench get larger.

As a long-term goal, consider replacing the input stimulus/checking file and Python code with SystemVerilog code. SV was designed for efficient constrained random stimulus generation, more so than other languages.