Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix WIDTH false positive with multidimesional packed arrays #1613

Open
veripoolbot opened this issue Nov 22, 2019 · 7 comments
Open

Fix WIDTH false positive with multidimesional packed arrays #1613

veripoolbot opened this issue Nov 22, 2019 · 7 comments
Labels
area: data-types Issue involves data-types area: lint Issue involves SystemVerilog lint checking effort: days Expect this issue to require roughly days of invested effort to resolve

Comments

@veripoolbot
Copy link
Contributor


Author Name: Tim Allen
Original Redmine Issue: 1613 from https://www.veripool.org


excerpt of file.sv...

logic   [31:0]          distance;
logic   [31:0]          bfield;
logic   [3:0]           cur_bit_len;
typedef struct packed {         // 10 bits total
     bit [8:0]          max_tbl_size;
     bit [15:0][31:0]   dec_len;
     bit [15:0][31:0]   dec_pos;
     bit [31:0]         q_bits;
} decode_table_t;

distance <= bfield - o_dec_table.dec_len[cur_bit_len - 4'd1];
    ^          ^                     ^         ^         ^
//32bits    32bits                32bits    4bits     4bits

yields the following false positive warnings
%Warning-WIDTH: file.sv:123: Operator SUB expects 32 or 6 bits on the LHS, but LHS's VARREF 'cur_bit_len' generates 4 bits.
%Warning-WIDTH: file.sv:123: Operator SUB expects 32 or 6 bits on the RHS, but RHS's CONST '4'h1' generates 4 bits.

I hope this is clear enough. AFAIK 

@veripoolbot
Copy link
Contributor Author


Original Redmine Comment
Author Name: Wilson Snyder (@wsnyder)
Original Date: 2019-11-22T17:21:39Z


The rightmost index of dec_len is 31:0, meaning it needs 6 bits to index into. Your cur_bit_len as you indicate is only 4 bits. Perhaps I'm confused, but seems the warning is correct.

@veripoolbot
Copy link
Contributor Author


Original Redmine Comment
Author Name: Tim Allen
Original Date: 2019-11-22T19:44:46Z


does bit [15:0][31:0] dec_len; not mean 16, 32 bit dec_lens for a total of 512 bits? So if I say something like ```dec_len[3]

@veripoolbot
Copy link
Contributor Author


Original Redmine Comment
Author Name: Tim Allen
Original Date: 2019-11-22T20:35:12Z


I think what I said above is consistent with IEEE 1800-2017 7.4.5 which says (among other things), "A subarray is an array that is an element of another array. As in the C language, subarrays are referenced by omitting indices for one or more array dimensions, always omitting the ones that vary most rapidly..." I find that a bit obtuse but in the preceding paragraph it describes "rapidity" of various dimensions which, I think, went something like

logic [2nd][1st] var [4th][3rd]

@veripoolbot
Copy link
Contributor Author


Original Redmine Comment
Author Name: Wilson Snyder (@wsnyder)
Original Date: 2019-11-22T23:13:10Z


You're right, an easy cross check:

  $display("%d", $size(o_dec_table.dec_len));

  16

What looks to be going on is for packed structs Verilator runs these checks twice, once on the code as written then once after the unpacking, the warning is on the second one which makes no sense for the user. Will look into fixing.

@veripoolbot veripoolbot added area: data-types Issue involves data-types area: lint Issue involves SystemVerilog lint checking effort: days Expect this issue to require roughly days of invested effort to resolve labels Dec 22, 2019
@rswarbrick
Copy link
Contributor

I've just run into this in our codebase in OpenTitan. A simpler example that triggers the same bug:

module arrays(input logic [4:0]        cnt_i,
              input logic [17:0][63:0] data_i,
              output logic [63:0]      out_o);

  assign out_o = data_i[5'd17 - cnt_i];

endmodule

The behaviour hasn't changed recently but running with a recent build from master gives:

> verilator --lint-only arrays.sv
%Warning-WIDTH: arrays.sv:5:31: Operator SUB expects 32 or 7 bits on the LHS, but LHS's CONST '5'h11' generates 5 bits.
                              : ... In instance arrays
    5 |   assign out_o = data_i[5'd17 - cnt_i];
      |                               ^
                ... Use "/* verilator lint_off WIDTH */" and lint_on around source to disable this message.
%Warning-WIDTH: arrays.sv:5:31: Operator SUB expects 32 or 7 bits on the RHS, but RHS's VARREF 'cnt_i' generates 5 bits.
                              : ... In instance arrays
    5 |   assign out_o = data_i[5'd17 - cnt_i];
      |                               ^
%Error: Exiting due to 2 warning(s)
rupert@halibut /s/l/opentitan (keymgr-lint) [1]> verilator --version
Verilator 4.201 devel rev v4.200-60-g6d3ec160

wsnyder added a commit that referenced this issue Apr 1, 2021
@wsnyder
Copy link
Member

wsnyder commented Apr 1, 2021

Fortunately disabling WIDTH still gives the right answer, will need to take a further look, meantime adding a test.

@stevehoover
Copy link

It seems I've hit this one, too, or at least something similar. Here's the minimal reproducer I came up with, parameterized to show different cases:

module top(...);
   // Instantiate register-file-ish thing.
   rf rf(.rd_index('0), .rd_value());
endmodule

// A read-only packed register file containing zero values (minimal reproducer).
module rf #(parameter VAL_MAX = 31,
            parameter MIN_REG = 9)
     (input logic [3:0] rd_index,
      output logic [VAL_MAX:0] rd_value);
   
   // Register values, packed.
   logic [15:MIN_REG][VAL_MAX:0] value;
   
   // Assign zero values.
   always_comb
      for (int i = MIN_REG; i <= 15; i++)
         value[i] = '0;
   // Read.
   assign rd_value = value[rd_index];
endmodule

It seems the WIDTH check for SUB of a packed 2D index is checking the range of the wrong packed index.

For VAL_MAX >= 31, MIN_REG >= 1:

%Warning-WIDTH: top.sv:23: Operator SUB expects 32 or 6 bits on the LHS, but LHS's VARREF 'rd_index' generates 4 bits.
: ... In instance makerchip.top.rf
assign rd_value = value[rd_index];
^~~~~~~~

There is no warning for VAL_MAX < 31 or MIN_REG == 0.

The number of expected bits seems to depend upon VAL_MAX even though this is indexing into the range [15:MIN_REG], not [VAL_MAX:0].
Expected bits are 32 or:
VAL_MAX: 0 - 30: No error
VAL_MAX: 31 - 62: 6
VAL_MAX: 63 - 126: 7
...
which is not exactly the power-of-two boundary one might expect for the [VAL_MAX:0] range (which, again, is the wrong range in the first place).

I'm using Verilator 4.028 2020-02-06 rev v4.026-92-g890cecc1.

seldridge referenced this issue in llvm/circt Nov 15, 2021
These have the same constraints as array extract.

This was noticed by inspection.  These operations aren't frequently
used, so this probably just hasn't been noticed yet.
@wsnyder wsnyder changed the title verilator %Warning-WIDTH false positive Fix WIDTH false positive with multidimesional packed arrays Oct 16, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: data-types Issue involves data-types area: lint Issue involves SystemVerilog lint checking effort: days Expect this issue to require roughly days of invested effort to resolve
Projects
None yet
Development

No branches or pull requests

4 participants