Compile error assigning to unpacked from packed array

[johnjohnlin]

I tried this on the master branch (96f9f85).

module Top(
   input a [5],
   output logic b
);
   assign b = a;
endmodule

Apparently, the assignment is incorrect and shall be detected. However, the following command executes without emitting any error.

verilator Top.sv --cc -Wall

The error is then propagated to the generated C++ codes, resulting in errors like:

invalid conversion from 「CData*」 {aka 「unsigned char*」} to 「CData」 {aka 「unsigned char」}

I am willing to try to fix Verilator by myself with assistance.

[wsnyder]

Generically assignment between packed and unpacked arrays isn't currently supported. This will be a bit complicated to fix as needs to look at the data types to know when such conversion is legal. As a workaround use an unpacked array for the input (which it perhaps was intended to be anyways.)

[johnjohnlin]

Hi, I am sorry that I did not describe my problem precisely. I totally agree that such a assignment between packed and unpacked arrays is invalid, but it is better to emit error messages when verilating the code, not deferring to C++ compiling.

Based on my experience, this is a common typo, and this bug can influence the productivity. For me who is familiar with Verilator, when I see a C++ compile error like this I know I made such a typo, but it still takes a few minutes to locate the typo in Verilog. For a newbie who made such a typo, the error message doesn't help them fix the Verilog code.

My suggestion is to emit an error message like "RHS and LHS bitwidth mismatch". For example, the code

module Top(
        input [4:0] a,
        output logic b
);
        assign b = a;
endmodule

gives %Warning-WIDTH as expected.

I think this requires to add more type checking code without large code rewrite. Again, I am willing to try to fix Verilator by myself with assistance.

[wsnyder]

Completely agree it should warn or support it.

As you probably anticipated the code to detect and warn is only slightly less work than supporting the conversion (but conversion would still need to add an error when sizes don't match according to IEEE). But neither should be a huge deal as I think all of the underlying data structure improvements needed to warn or support this are probably in place.

If you'd like to work on this it would be great:

1. Add appropriate tests to test_regress. (That is both cases that should work, plus cases that should give errors).
2. In V3Width update probably iterateCheck() to do the conversion.

[yodalee]

I just checked the assignment of a [5]. In V3Width.cpp function widthCheckSized, I can see that it access the width of nodep (aka a[5]) but the width it return is 1.
So there is no error emitted. However I have no idea why the a[5] is consider as width 1. Is there any hints about it?

[wsnyder]

 input a [5],

is a 1 bit wide signal that is then part of an unpacked array of dimension 5. width shows the unpacked width, which thus is 1.

[yodalee]

Yes, I think so. So do we have way do know that it is an unpacked array, and to get the dimension?
I think this problem might not be in V3Width. We should block the assignment from array type to scalar type at type checking.

[yodalee]

I tried to debug this issue with following test case:

module t_cover_width (
  input [4:0] a1,
  input a2 [5],
  output logic b1,
  output logic b2
);
assign b1 = a1;
assign b2 = a2;
endmodule

I tracked another round and find something near V3Width.cpp:iterateCheck.
It gets the type of

1. expBasicp
2. underBasicp
   Both of them are logic. However it is weird to me that the info about the unpacked array is disappear. Do we have any way to know that at iterateCheck?
   Supplemental: I found that when we get underBasicp, we will call underp->dtypep()->basicp(). The type returned by dtypep() is an AstNodeArrayDType. So it is possible to know that rhs is a array type at iterateCheck. Maybe we can check here and emit the warning. How do you think about it?

[wsnyder]

The dtype() can be checked to see if it's a unpacked, some other code does this. I think this is probably past the point it's easily fixable, but thanks for trying, I'll need to spend some time trying to fix it.

[wsnyder]

Note #2458 which is a somewhat similar missing warning.