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.

• u185619: i am not sure what you mean by "It is not necessary to use a default there." I tried removing the default case, and lint target complained saying: Latch inferred for signal 'out' (not all control paths of combinational always assign a value) : ... Suggest use of always_latch for intentional latches 31 | always_comb begin
• toolic: @u185619: Keep in mind that this is a mere suggestion. It is up to you to decide what recommendations to adopt in this or any answer. It could be that your lint tool does not strictly comply with the IEEE Std. Regardless, I copied more information from the linked Stack Overflow answer into this answer.
• user1908704: XLEN is part of the RISC-V spec - it's the width of the datapath (32 bit, 64 bit etc). Anyone familiar with the spec will recognise it, but fair enough that it could be worth commenting for those unfamiliar.