SystemVerilog implementation of an N-bit prefix adder logic design

Question

I want some feedback about my code (anything is welcome). It is working, but it feels like a clumsy implementation. Because I am self-learning from a book without an answers section, it becomes difficult to know how to proceed.

module prefix_adder #(parameter N=4)
  (input logic [2**N-1:0] a, b, 
  input logic cin, 
  output logic [2**N-1:0] s);
  genvar i, j;
  logic [2**N-1:0] g[N:0], p[N:0];
  assign p[0][0] = 0;
  assign g[0][0] = cin;
  generate
    for (j=0; j<(N**2-1); j++) begin
      assign p[0][j+1] = a[j] | b[j];
      assign g[0][j+1] = a[j] & b[j];
    end
  endgenerate
  generate
    for(i=1; i<=N; i++) begin
      for (j=0; j<N**2; j++) begin
        if(j%2**i <= 2**(i+1) && j%2**i >= 2**(i-1)) begin
          assign p[i][j] = p[i-1][j] & p[i-1][j-1-(j%(2**i))+2**(i-1)];
          assign g[i][j] = g[i-1][j] | p[i-1][j] & g[i-1][j-1-(j%(2**i))+2**(i-1)];
        end
        else begin
          assign p[i][j] = p[i-1][j];
          assign g[i][j] = g[i-1][j];
        end
      end
    end
  endgenerate
  generate
    for (j=0; j<N**2; j++) begin
      assign s[j] = a[j] ^ b[j] ^ g[N][j];
    end
  endgenerate
endmodule

Answer 1

The code uses good indentation, and it makes good use of for loops to avoid code duplication. It's great that you used a parameter to make the design easily scalable to other bit widths.

While it is common to use brief names for the module ports for an adder (like a, b and s), because they are self-evident, you might want to give meaningful names for the internal signals, g and p. At the very least, you should add comments to the code to describe why you chose those names. If you are following a known algorithm, you should add comments describing the algorithm and provide a reference to a book, paper, website, etc.

The sum does not work for large values of the inputs. For example, when a and b are their maximum values ('1), the sum will overflow. To handle the maximum values, you need to increase the width of the s output. However, if that is not the design intent, and you assume restrictions on the input values, you should add comments to the code regarding those restrictions.

If there are any assumed restrictions on the parameter N, you should also document that with comments.

When a line has multiple operators, I prefer to use extra parentheses around each term to avoid potential operator precedence issues and to make the code easier to read.

I realize you are using this as a way to learn SystemVerilog, but you should not get too bogged down in trying to design adders with specific implementations. It is common to use synthesis tools to convert high-level Verilog code into hardware, and those tools are great at optimizing the design for speed/area/power. It is most common to design an adder with a single line of code:

assign s = a + b;

I strongly recommend testing your code with a simple self-checking testbench, such as this example.

• Miguel Ortega: Thanks, toolic. I had one more thing to ask. Is the generate block written with good practices?
• toolic: @MiguelOrtega: Yes, the generate blocks look good.
• Serge: I would recommend to add parenthesis in all expressions like j%2**i . You missed some in the condition of the 'if' statement.