Assertion firing when accessing structure array elements through foreach

[wmlye]

I'm running under WSL2, using the most-recent head of the tree (I did the 'git pull' about 5 minutes ago so this looks like the first issue filed on 13.0). I'm also taking a crack at learning SystemVerilog over a 2-week Christmas break, so it's entirely possible that I'm doing something that isn't legal SystemVerilog. I've stripped my testcase down to this:

module array_struct_assert_test;

typedef struct packed
{
  logic b;
} s;

s [1:0] a;

always @(*)
  foreach(a[i])
    a[i].b = 1'b0;

endmodule

I realize this particular snippet doesn't make a lot of sense; the real code has a lot more going on that may conditionally set a[i].b to 1 but I'm assigning the default at the beginning to avoid latch inference. I'm aware that always_comb is supposed to avoid this requirement -- Verilog habits die hard, and if I were to use always_comb instead of always I see the same issue. Either way, when I try to compile this I get the following

wlye@AMR-BU-L02782:~/foton$ iverilog -g2012 array_struct_test.sv
array_struct_test.sv:12: error: A reference to a wire or reg (`i') is not allowed in a constant expression.
array_struct_test.sv:12: error: Array index expressions must be constant here.
ivl: netmisc.cc:1612: NetExpr* collapse_array_indices(Design*, NetScope*, NetNet*, const std::__cxx11::list<index_component_t>&): Assertion `rc' failed.
Aborted
wlye@AMR-BU-L02782:~/foton$

I'm not sure whether or not the error is correct; if it is correct can I get some suggestions about how to write this correctly? If it is incorrect, then this may point to an issue in the compiler.

However, independent of whether or not the error message is correct, we shouldn't be seeing an assertion firing.

[larsclausen]

Your code is correct. This is a restriction in the current Icarus implementation. This is a duplicate of #521.

a[i].b is the same as a[i][0]. In Verilog a dynamic index was only allowed for the most outer dimension, in SystemVerilog this restriction has been removed, but iverilog hasn't be updated to handle this properly.

But you are right, this should not trigger an assert, it should print an message that this is not yet supported.

[wmlye]

Thanks, Lars. I had looked through the issues list looking for "assert" because I suspected that would be the higher-priority issue, but hadn't looked for the text of the error message otherwise I probably would have found that issue. Although, to be honest, if I had I probably wouldn't have realized it was a duplicate. Now that I think about it a bit more, it makes sense.

On a scale of 1 to 10, 10 being hardest, how would you guess the complexity of addressing at the source (i.e. allowing a dynamic index for all dimensions) would be? I'm wondering if this is something I might be able to help with or maybe take on; inn addition to learning SystemVerilog I'm also trying to get back into programming in C/C++. I was trained as a software engineer but ended up doing CMOS analog design for ~30 years and I've got >20 years of experience writing SKILL to automate aspects of my day job.

[larsclausen]

It's certainly not trivial, but also not impossible if you are up for a good challenge. The main issue is that there is a fair amount of code that assumes that the index is known during synthesis and can be collapsed into a single integer (grep for list<long>). Internally in the simulator arrays as well as indices are represented in so called canonical form.

In canonical form the array has only one dimension and the indices always start at 0. E.g.

reg [2:0][4:1] r;
r[i][j] = 10;

is the following in canonical

reg [11:0] r;
r[i*4+(j-1)] = 10;

You need to build up a NetExpr from the dynamic elements that represents the canonical index. There is a function called normalize_variable_unpacked() which is used for unpacked arrays. But the same applies for packed arrays. The other thing you have to do is replace evaluate_index_prefix() with indices_to_expressions(). And then pass the result from indices_to_expression() to to normalize_variable_unpacked() instead of prefix_to_slice().

There are a few places in the code base where this needs to be done, but your particular case with the struct is in this function https://github.com/steveicarus/iverilog/blob/master/elab_lval.cc#L1188.

The canonical offset that gets calculated here is constant. Your first step should be to turn unsigned long off into NetExpr *off. Even without handling the case where the index is dynamic. Just build up a dynamic expression from static indices and field offsets. There are some helper functions in net_misc.c like make_add_expr() and make_mul_expr() that will help you with this. Once that is working you can add support for the dynamic index.

This is not the easiest if you do not know the code base yet. Don't give up if you don't understand everything immediately, it will take some time. Run the code with debug_elaborate and also add additional debug printfs in there to help you better understand what is going on. Feel free to post early versions of your changes if you want some feedback or if there is a particular problem you don't know yet how to solve.

[wmlye]

Yeah, I've had a lot of "fun" passing MxN 2D arrays around in Verilog as canonical MN 1D arrays that I then have to pull apart as `a[Ni+:N]` in generate loops, so I'm at least familiar with the necessary concepts to make this work.

Am I correct in assuming that this is a three-pass compiler, first a parsing pass converting input into an syntax tree, second elaboration pass converting the syntax tree into an expression tree, followed by a formatting pass that converts the expression tree into the final target code, and that the portion I would be focusing on is in the second pass?

[larsclausen]

Am I correct in assuming that this is a three-pass compiler, first a parsing pass converting input into an syntax tree, second elaboration pass converting the syntax tree into an expression tree, followed by a formatting pass that converts the expression tree into the final target code, and that the portion I would be focusing on is in the second pass?

I'd say that's roughly right. Your input is PExpr objects (The P stands for parser, I believe) and the output is NetExpr objects (Net as in netlist). The NetExprs get consumed by the code code generator.

[wmlye]

Oh, interesting. Just out of curiosity I checked to see if there was a difference whether the array access was on the LHS or on the RHS of the assignment, and yes indeed there is. Here's when a[i].b is on the LHS, and it errors out and causes the assertion failure.

wlye@AMR-BU-L02782:~/foton$ cat array_struct_assert_test1.sv
typedef struct packed
{
  logic b;
} s;

module array_struct_assert_test1
(
  input  logic [1:0] c
 ,output s     [1:0] a
);

always_comb
  foreach(c[i])
    a[i].b=c[i];

endmodule
wlye@AMR-BU-L02782:~/foton$ iverilog -g2012 array_struct_assert_test1.sv
array_struct_assert_test1.sv:14: error: A reference to a wire or reg (`i') is not allowed in a constant expression.
array_struct_assert_test1.sv:14: error: Array index expressions must be constant here.
ivl: netmisc.cc:1612: NetExpr* collapse_array_indices(Design*, NetScope*, NetNet*, const std::__cxx11::list<index_component_t>&): Assertion `rc' failed.
Aborted
wlye@AMR-BU-L02782:~/foton$

However when it's on the RHS it goes through cleanly:

wlye@AMR-BU-L02782:~/foton$ cat array_struct_assert_test2.sv
typedef struct packed
{
  logic b;
} s;

module array_struct_assert_test2
(
  input  s     [1:0] a
 ,output logic [1:0] c
);

always_comb
  foreach(c[i])
    c[i]=a[i].b;

endmodule
wlye@AMR-BU-L02782:~/foton$ iverilog -g2012 array_struct_assert_test2.sv
wlye@AMR-BU-L02782:~/foton$

Looking a bit deeper at #521 I see something a bit different. The equivalent of a[i].b would be a[i][0], and if it's on the LHS I get the following:

wlye@AMR-BU-L02782:~/foton$ cat array_struct_assert_test3.sv
module array_struct_assert_test3
(
  input  logic [1:0]       c
 ,output logic [1:0][1:0]  a
);

always_comb
  foreach(c[i])
    a[i][0]=c[i];

endmodule
wlye@AMR-BU-L02782:~/foton$ iverilog -g2012 array_struct_assert_test3.sv
array_struct_assert_test3.sv:9: error: A reference to a wire or reg (`i') is not allowed in a constant expression.
array_struct_assert_test3.sv:9: error: Array index expressions must be constant here.
2 error(s) during elaboration.
wlye@AMR-BU-L02782:~/foton$

I.e. error without assertion firing. When I put it on the RHS I get the following:

wlye@AMR-BU-L02782:~/foton$ cat array_struct_assert_test4.sv
module array_struct_assert_test4
(
  input  logic [1:0][1:0]  a
 ,output logic [1:0]       c
);

always_comb
  foreach(c[i])
    c[i]=a[i][0];

endmodule
wlye@AMR-BU-L02782:~/foton$ iverilog -g2012 array_struct_assert_test4.sv
array_struct_assert_test4.sv:9: error: A reference to a wire or reg (`i') is not allowed in a constant expression.
array_struct_assert_test4.sv:9: error: Array index expressions must be constant here.
array_struct_assert_test4.sv:7: warning: always_comb process has no sensitivities.
2 error(s) during elaboration.
wlye@AMR-BU-L02782:~/foton$

In other words, while these two issues are definitely related and probably should/could be fixed at the same time, they aren't quite the same. There's also an asymmetry in LHS/RHS handling during elaboration.

[martinwhitaker]

A couple of other useful aids to debugging the compiler are the -N option accepted by the iverilog driver, which writes the output of the second (elaboration) pass to a file in a human-readable form, and the -P option accepted when running the compiler (ivl) directly, which does the same for the output of the first (parser) pass.