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

Inconsistent LITENDIAN warnings on arrays #1382

Open
veripoolbot opened this issue Dec 31, 2018 · 2 comments
Open

Inconsistent LITENDIAN warnings on arrays #1382

veripoolbot opened this issue Dec 31, 2018 · 2 comments

Comments

@veripoolbot
Copy link

@veripoolbot veripoolbot commented Dec 31, 2018


Author Name: Al Grant
Original Redmine Issue: 1382 from https://www.veripool.org


This is something that has come up before in #614 and #1202 but still doesn't seem quite right as of 4.008. In this code:

module endian;
  logic e_big[3:0];
  logic e_lit[0:3];
  logic eu_big_big[3:0][3:0];
  logic eu_lit_big[0:3][3:0];
  logic eu_big_lit[3:0][0:3];
  logic eu_lit_lit[0:3][0:3];
  logic [3:0] epu_big_big[3:0];
  logic [0:3] epu_lit_big[3:0];    // warn
  logic [3:0] epu_big_lit[0:3];
  logic [0:3] epu_lit_lit[0:3];    // warn
  logic [3:0][3:0] ep_big_big;
  logic [0:3][3:0] ep_lit_big;     // warn
  logic [3:0][0:3] ep_big_lit;     // warn
  logic [0:3][0:3] ep_lit_lit;     // warn
endmodule

Verilator warns as shown. This is in accordance with the resolution to #614 which was to warn for ascending dimensions when packed but not when unpacked. But what's really the issue here is whether the dimension is of a bit vector or not, i.e. would define the bit indexes into a contiguous bit vector that ends up as a single C object. And that to me looks independent of whether it's packed or unpacked - it has much more to do with whether the dimension is the first dimension or not. It just happens that often in cases like

logic [63:0] arr[0:3];

the first dimension is a bit index and the second one isn't - so #644's rule that says to warn about packed but not unpacked turns out to do what we want. In cases like

logic [3:0] arr[0:3];

it's less clear - perhaps Verilator would treat this as a 4x4 bit matrix and pack it into one word. But either way, whether it's packed or unpacked makes no difference. It's the order of the dimensions, and the overall size of the object as each dimension is applied, which determines whether objects might be packed into a bit vector.

I'd suggest a different rule:

  • process dimensions left to right (ignoring any distinction between packed/unpacked i.e. before/after the name)
  • keep track of the accumulated size of the object as we multiply in the latest dimension
  • while the object is still packable into a bit vector (size < 64 maybe?) warn if the latest dimension is ascending

This also does the right thing for arrays of structures. It makes the warning much more dependent on how objects are or aren't represented as data, and much less on how they are syntactically expressed in the source.

@veripoolbot

This comment has been minimized.

Copy link
Author

@veripoolbot veripoolbot commented Jan 2, 2019


Original Redmine Comment
Author Name: Wilson Snyder (@wsnyder)
Original Date: 2019-01-02T23:15:22Z


The warning is intended to find common Verilog bugs, not warn of constructs that may have confusing mappings to C++. Perhaps add a new warning that strictly warns about any big endianness?

As to warning about anything more than 64 bits this seems strange. Presumably the user knows the size of the operations they are using, how is that a mistake?

@veripoolbot

This comment has been minimized.

Copy link
Author

@veripoolbot veripoolbot commented Jan 3, 2019


Original Redmine Comment
Author Name: Al Grant
Original Date: 2019-01-03T09:40:13Z


Well the 64-bit cutoff was just an idea, to achieve the same effect as the current packed/unpacked heuristic but in a more principled way. The idea would be to stop warning about ascending dimensions once the total object gets larger than 64 bits (or some other size that stops it being represented in one word).

So you would get a warning for either of

local a[0:3];
local [0:3] a;

but neither of

somebigobject a[0:3];
somebigobject [0:3] a;

The principle being that the warning depends on the dimensions and the size of the object being dimensioned, not on whether the dimension happens to occur before or after the name.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
1 participant
You can’t perform that action at this time.