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;