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

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 ## 1 Answer 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