8-bit ALU in Verilog

Question

I'm an EE student who's taken a a couple digital logic/design courses, but they were focused on schematic representation, so I'm teaching myself Verilog to implement what I've learned.

For a basic "Hello World" project, I've implemented a basic 8-bit ALU with addition/subtraction with carry/borrow, NOT, AND, OR, XOR, and carry/zero/sign flags.

I've implemented it on an FPGA and everything works, but I want to make sure I'm not falling for any beginner traps which can lead to dramatic/undesired results in the final implementation of the circuit.

module ALU(A, B, Cin, Op, Out, Cout, Zero, Sign);
 input[7:0] A, B;
 input Cin;
 //011-A+B, 111-A-B, 000-NOT A, 001-A AND B, 010-A OR B, 100-A XOR B
 input[2:0] Op;
 output reg[7:0] Out;
 output Cout;
 output reg Zero;
 output reg Sign;
 
 wire FinalCin; //Final value of Cin fed to ALU after XOR
 wire[7:0] FinalB; //Final value of B fed to ALU after XOR
 wire[7:0] AddResult;
 wire[7:0] LogicResult;
 
 Adder DEV_ADD(A, FinalB, FinalCin, AddResult, Cout);
 LogicUnit DEV_LOG(A, B, Op, LogicResult);
 
 
 assign FinalCin = Cin ^ Op[2]; //Cin XOR Subtract Line
 assign FinalB = B ^ {8{Op[2]}}; //XOR B with Subtract Line
 
 
 always @ (Op or AddResult or LogicResult) begin
 if(Op == 3'b011 || Op == 3'b111)
 Out <= AddResult;
 else if(Op == 3'b000 || Op == 3'b001 || Op == 3'b010 || Op == 3'b100)
 Out <= LogicResult;
 else
 Out <= 8'b00000000;
 end
 
 
 always @ (Out) begin
 if(Out == 8'b00000000)
 Zero <= 1'b1;
 else
 Zero <= 1'b0;
 
 Sign <= Out[7];
 end
endmodule

Adder Module:

module Adder(A, B, Cin, S, Cout);
 input[7:0] A, B;
 input Cin;
 output wire[7:0] S;
 output wire Cout;
 
 assign {Cout, S} = A + B + Cin;
endmodule

Logic Unit:

module LogicUnit(A, B, Op, Out);
 input[7:0] A, B;
 input[2:0] Op; //000-NOT A, 001-A AND B, 010-A OR B, 100-A XOR B
 output reg[7:0] Out;
 
 always @ (Op or A or B) begin
 case(Op)
 3'b000: Out <= ~A;
 3'b001: Out <= A & B;
 3'b010: Out <= A | B;
 3'b100: Out <= A ^ B;
 endcase 
 end
 
endmodule

Answer 1

Design Issue:

From a design perspective I see one issue in LogicUnit. Out is an inferred latch because it is not assigned in all conditions of Op. The risks associated with these latches and how to use them property (when needed) have already been discussed on other Stack Exchange sites; such as here on Electrical Engineering Stack Exchange and here on Stack Overflow. Synthesis considers all possibilities. Depending on your setup it may or may not recognize Op>4 as out of bounds. It is safer to give it an explicit assignment and allow the synthesizer to choose how to optimize.

In your situation a latch should not be inferred. The easiest way to resolve this is to ass a default statement:

always @* begin
 case(Op)
 3'b000: Out = ~A;
 3'b001: Out = A & B;
 3'b010: Out = A | B;
 3'b100: Out = A ^ B;
 default: Out = A ^ B; // <<-- or any expression to a known value
 endcase
end

You may have noticed I also changes the <= to =. This has to do with coding style recommendations and suggestions that I will cover next.

Coding style:

Blocking vs Non-Blocking Assignments:

The only major coding style concerning I see is that you are using non-blocking assignments (<=) for all assignments in your always blocks. Non-blocking assignments should be used for flops (edge-sensitive) or latches (level-sensitive). Blocking assignments (=) should be used for combinational logic; which is what your logic is.

Blocking and non-blocking update the LHS at different regions in the scheduler. Blocking is immediate evaluation and assignment. Non-blocking is immediate evaluation with postponed assignment. Non-blocking was introduced to resolve a race condition when evaluating and assigning synchronous logic. Using non-blocking inappropriately, such as using it everywhere, can introduce another form of race conditions.

It is recommended to keep blocking and non-blocking assignments in separate always blocks. There are technically legal ways to safely mix them, but most avoid it all together

FYI, by using blocking assignment you could merge the always blocks that assign Out and Zero.

Sensitivity list:

What you have is legal and 100% complaint with the original Verilog IEEE standard (IEEE1364-1995). However automatic sensitivity (@* or @(*)) was added in IEEE1364-2001 and is the preferred and recommended way of declaring conbinational and latching logic with Verilog. @* use the RHS varabiles and nets used in the always-block to determine what signals it needs to be sensitive to. It does not look for signals used inside the body of tasks or functions (signals in the port list are fine) used with in the always block. This is a limitation, but it is also not recommended to have synthesizable tasks/functions reference non-port signals.

@* does more that reduce typing. By allowing verilog to manage the sencitity list for you, it reduces the risk of forgetting a signal (behavior vs synthesis mismatch), having unnecessary signals (possible simulation overhead), and typos (compiling errors or will use the wrong signal).

In summary use @* (or the synonymous @(*)).

Module Instances:

You are using connect by order which is okay for small modules with simple connectivity and whose port list isn't under constant editing. If you use connect by name, the port connections are explicit and it will not matter if the port order is re-arranged. It also helps any human readers of your code understand how the ports are intended to connect.

Adder DEV_ADD( .A(A), .B(FinalB), .Cin(FinalCin), .S(AddResult), .Cout(Cout) );

Module header:

Your module header is is complaint IEEE1364-2001 non-ANSI style (History: in IEEE1364-1995, declaring something to be an output and a reg had to be done in separate statements). I have heard of one case where some tools do not support non-ANSI header with one-line output reg. I have know idea how prevalent this issue is as most new code is written with the ANSI header style. Legacy code and some generated code is written as non-ANSI that is comparable with IEEE1364-1995. ANSI header style is currently the most popular because it is the simplest and cleanest way of managing ports. It becomes more useful as the port list grows and when it is edited often.

You don't need to change your code if you do want to. Be aware if you ever run into an issue with another simulator, synthesizes, or other tool that takes in Verilog. A compiling issue could be because some developers skipped support for one-line output reg for non-ANSI headers. It is technically a bug for the tool but it is unlikely to get much attention. The developers are more focused in adding/fixing/improving higher demand features. Plus there are two work arounds: ANSI and 1995's non-ANSI.

Here is an example of you ALU headed in the three flavors.

ANSI style (popular): IEEE1364-2001 ==> IEEE1800-2012(SystemVerilog)

module ALU( // PORT ORDER, DIRECTION, AND TYPE
 input [7:0] A, B,
 input Cin,
 //011-A+B, 111-A-B, 000-NOT A, 001-A AND B, 010-A OR B, 100-A XOR B
 input [2:0] Op,
 output reg [7:0] Out,
 output Cout,
 output reg Zero,
 output reg Sign );

non-ANSI style (1995 style): IEEE1364-1995 ==> IEEE1800-2012(SystemVerilog)

module ALU(A, B, Cin, Op, Out, Cout, Zero, Sign); // PORT ORDER
 // PORT DIRECTION
 input [7:0] A, B;
 input Cin;
 //011-A+B, 111-A-B, 000-NOT A, 001-A AND B, 010-A OR B, 100-A XOR B
 input [2:0] Op;
 output [7:0] Out;
 output Cout;
 output Zero;
 output Sign;
 // OUTPUT TYPE
 reg [7:0] Out;
 reg Zero;
 reg Sign;

non-ANSI style (2001 style): IEEE1364-2001 ==> IEEE1800-2012(SystemVerilog)

module ALU(A, B, Cin, Op, Out, Cout, Zero, Sign); // PORT ORDER
 // PORT DIRECTION AND TYPE
 input [7:0] A, B;
 input Cin;
 //011-A+B, 111-A-B, 000-NOT A, 001-A AND B, 010-A OR B, 100-A XOR B
 input [2:0] Op;
 output reg [7:0] Out;
 output Cout;
 output reg Zero;
 output reg Sign;

Answer 2

The previous answer is thorough and makes many great points, which I will not repeat. Here are some more coding style suggestions.

Partitioning

It is great that you partitioned the design using module hierarchy. However, there is no need for Adder to be a separate module because addition is such a trivial operation in Verilog. You can simply use the single assign line directly in the top module.

It is also good that you separated code into multiple always blocks. The always block for Zero and Sign is not needed for such simple logic. They can just be simple continuous assignments:

assign Zero = (Out == 8'b00000000);
assign Sign = Out[7];

In this case, there is no need to declare these signals using the reg type:

output Zero;
output Sign;

Readability

Constants with a large number of digits like:

8'b00000000

are easier to read using an underscore separator:

8'b0000_0000

In this case, since all the digits are the same, the replicated concatenation operator can be used (as you already used it elsewhere)

{8{1'b0}}

Parameters

This comment is useful:

//011-A+B, 111-A-B, 000-NOT A, 001-A AND B, 010-A OR B, 100-A XOR B

But, it is a little hard to read being all on one line, and it is far from the code where it applies.

In Verilog, constants are typically replaced by parameter types. For example:

// ALU functions
localparam ADD = 3'b011;
localparam SUB = 3'b111;
localparam NOT = 3'b000;
localparam AND = 3'b001;
localparam OR = 3'b010;
localparam XOR = 3'b100;
always @ (Op or AddResult or LogicResult) begin
 if (Op == AND || Op == SUB)
 Out <= AddResult;
 else if (Op == NOT || Op == AND || Op == OR || Op == XOR)
 Out <= LogicResult;

DRY

Lines like this:

if (Op == AND || Op == SUB)

have repeated code (Op == ). This can be simplified using the inside operator. This syntax requires you to enable SystemVerilog features in your simulator, if they are not already enabled by default:

if (Op inside {AND, SUB})