3
\$\begingroup\$

I have been learning SystemVerilog before I go back to school and decided to try and implement a Carry Lookahead Adder. As far as I can tell, it works correctly though I haven't tested extensively, just the tests you see on the testbench. I would like some feedback on best practices, things to avoid, and any other criticisms of the code and design since I am learning out of a textbook and don't have anyone to ask.

Design

`timescale 1ns / 1ps

module full_adder(
    input logic a, b, cin,
    output logic g, p, s
    );
    
    assign g = a & b;
    assign p = (a ^ b) & cin;
    assign s = cin ^ (a ^ b);
    
endmodule

module four_bit_cla_logic(
    input logic cin,
    input logic [3:0] g, p,
    output logic [3:0] c,
    output logic gg, pg);   

    assign c[0] = g[0] | (p[0] & cin);
    assign c[1] = g[1] | (p[1] & g[0]) | (p[1] & p[0] & cin);
    assign c[2] = g[2] | (p[2] & g[1]) | (p[2] & p[1] & g[0]) | (p[2] & p[1] & p[0] & cin);
    assign c[3] = g[3] | (p[3] & g[2]) | (p[3] & p[2] & g[1]) | (p[3] & p[2] & p[1] & g[0]) | (p[3] & p[2] & p[1] & p[0] & cin);
    assign gg = g[3] | (p[3] & g[2]) | (p[3] & p[2] & g[1]) | (p[3] & p[2] & p[1] & g[0]);
    assign pg = (p[3] & p[2] & p[1] & p[0]);

endmodule

module four_bit_cla(
    input logic [3:0] a, b,
    input logic cin,
    output logic [3:0] s,
    output logic cout, gg, pg);
    
    logic [3:0] g, p, c;
    full_adder fa0(a[0], b[0], cin, g[0], p[0], s[0]);
    full_adder fa[3:1](a[3:1], b[3:1], c[2:0], g[3:1], p[3:1], s[3:1]);
    four_bit_cla_logic cla(cin, g[3:0], p[3:0], c[3:0], gg, pg);
    assign cout = c[3];
    
endmodule

Testbench (Shamelessly stolen from my textbook and modified for my modules - I've got more learning to do here)

module testbench();
    logic clk, reset;
    logic [3:0] a, b, s, sexp;
    logic cin, cout, gg, pg, coutexp, ggexp, pgexp;
    logic [31:0] vectornum, errors;
    logic [15:0] testvectors[10000:0];
    // instantiate device under test
    four_bit_cla dut(a[3:0], b[3:0], cin, s[3:0], cout, gg, pg);
    // generate clock
    always
    begin
        clk = 1; #5; clk = 0; #5;
    end
    // at start of test, load vectors
    // and pulse reset
    initial
    begin
        $readmemb("testvectors.tv", testvectors);
        vectornum = 0; errors = 0;
        reset = 1; #27; reset = 0;
    end
    // apply test vectors on rising edge of clk
    always @(posedge clk)
    begin
        #1; {a[3:0], b[3:0], cin, sexp[3:0], coutexp, ggexp, pgexp} = testvectors[vectornum];
    end
    // check results on falling edge of clk
    always @(negedge clk)
    if (~reset) begin // skip during reset
        if ({{s[3:0]}, cout, gg, pg} !== {{sexp[3:0]}, coutexp, ggexp, pgexp}) begin // check result
        $display("Error: inputs = %b", {{a[3:0]}, {b[3:0]}, cin});
        $display(" outputs = %b (%b expected)", {{s[3:0]}, cout, gg, pg}, {{sexp[3:0]}, coutexp, ggexp, pgexp});
        errors = errors + 1;
        end
        vectornum = vectornum + 1;
        if (testvectors[vectornum] === 16'bx) begin
        $display("%d tests completed with %d errors",
        vectornum, errors);
        $finish;
        end
    end
endmodule

Test Vectors

1111_1111_0_1110_1_1_0
1001_1100_0_0101_1_1_0
1010_0101_1_0000_1_0_1
0000_0000_0_0000_0_0_0
0000_0000_1_0001_0_0_0
1111_1111_1_1111_1_1_0
1101_1000_0_0101_1_1_0
0110_1110_1_0101_1_1_0
0101_1010_1_0000_1_0_1
0001_1001_1_1011_0_0_0
\$\endgroup\$

1 Answer 1

2
\$\begingroup\$

The layout of your code is good, and you did a good job partitioning the design from the testbench.

In your design, you could take advantage of the "reduction" operators to make your code more compact and less prone to errors. For example, you could change:

assign pg = (p[3] & p[2] & p[1] & p[0]);

to:

assign pg = &p;

Here, & acts as a reduction-AND operator which ANDs all the bits of p together, returning a single bit value.

When you instantiate modules, you could use connections-by-name instead of connections-by-order. For example, change:

four_bit_cla dut(a[3:0], b[3:0], cin, s[3:0], cout, gg, pg);

to:

four_bit_cla dut (
    .a     (a),
    .b     (b),
    .cin   (cin),
    .cout  (cout),
    .gg    (gg),
    .pg    (pg),
    .s     (s)
);

This is much less error prone as the number of ports increases; connection errors are a common Verilog pitfall. Connections-by-name is also self-documenting.

In the testbench, it is better for debugging if you add $time to your $display messages.

You could use the auto-increment operator. Change:

                    errors = errors + 1;

to:

                    errors++;

Also, you can use 2-state types rather than 4-state when you don't need to model x or z values. Change:

    logic [31:0] errors;

to:

    int errors;

You can even omit the initialization since all 2-state types default to 0. There is less to type, and 2-state could result in better simulation performance.

You should use more indentation levels in your checker code.

always @(negedge clk)
if (~reset) begin // skip during reset
    if ({{s[3:0]}, cout, gg, pg} !== {{sexp[3:0]}, coutexp, ggexp, pgexp}) begin // check result
        $display($time, " Error: inputs = %b", {{a[3:0]}, {b[3:0]}, cin});
        $display(" outputs = %b (%b expected)", {{s[3:0]}, cout, gg, pg}, {{sexp[3:0]}, coutexp, ggexp, pgexp});
        errors++;
    end
    vectornum++;
    if (testvectors[vectornum] === 16'bx) begin
        $display($time, " %d tests completed with %d errors", vectornum, errors);
        $finish;
    end
end
\$\endgroup\$
0

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.