Preliminary unpacked support for localparam

[phhorrein]

As we definitely need this in our projects, I've started working on addition of unpacked array support initialization.

This is a first try at doing this for localparam only, to see how it goes. I've been focusing on "getting it to work", and thus a lot of things need to be added. At least, the vpi_draw and t-dll file must be updated to take the new uarray into accounts.

However, I would really like to contribute this to the project if you're interested, and thus I want to discuss how I did this before going further. So here is a draft pull request with a skeleton of how I have done it.

As this is my first meaningful modification of Icarus source code, I would really appreciate any feedback you can give, on the design choices or on the coding style, or on whole parts of the code I missed. I will do my best to answer them and to modify the code.

I have small tests I added manually to ivtest, I can push them on a separate PR if you want.

[martinwhitaker]

Sorry for the slow response. I was hoping Steve would have time to look at it, as he's the authority on architectural matters.

Could you explain why you have chosen to add IVL_VT_UARRAY. Currently unpacked arrays are handled as special cases of the base types, so this is a significant architectural change.

[phhorrein]

Hello, thanks for the comment. No problem for the time, it's not an urgent issue at all.

To be honest, I started implementing this at the same time as I learnt how iverilog was built internally. I remember having issue at first when trying to do it without the new UARRAY type, but it might as well be because of my low level of understanding of the code base. I can't rembember what it was.

I will try again without the new type, now that I understand things better, at least to be able to explain why the UARRAY if I still need it, or to provide a new way without it (which would definitely be better, I agree).

[martinwhitaker]

I don't want to discourage you from proposing architectural changes if they make it easier to add new features. Design decisions made before SystemVerilog existed may not be the best now.

[phhorrein]

I removed the new type. It turns out it was useless, and I find it much better this way. It implies a lot less modifications. I'm writing an associated test for that (I already have one, I'm only trying to add cases and check that it's really working).

[jlwehle]

Unfortunately this crashes on a design accepted by Vivado.
The platform is FreeBSD 12.2 x64 ... iverlog 11.0 works fine on the
machine, though without supporting localparam arrays.
Reduced testcase attached:

iverilog -g2009 -o lpa_tc lpa_tc.v
lpa_tc.v:15: assert: eval_tree.cc:783: failed assertion lval.len() == wid
Abort trap (core dumped)

lpa_tc.v.txt

[jlwehle]

What I see while debugging is:

localparam integer NCOEFF [0:3] = '{ NTAPS[0] / 2, ...

being processed as:

  ddc_cfir.v:64: elaborate_expr_param_: Parameter: NTAPS['sd0]
  ddc_cfir.v:64: elaborate_expr_param_: par_type: 11netuarray_t
  ddc_cfir.v:64: PENumber::test_width: Value='sd0, width=32, output mode=unsized
  ddc_cfir.v:64: elab_and_eval: test_width of 'sd0
  ddc_cfir.v:64:              : returns type=logic, context_width=-1, signed=1, force_expand=0,  expr_width=32, mode=unsized
  ddc_cfir.v:64:              : cast_type=<no_type>
  ddc_cfir.v:64: elab_and_eval: Calculated width is 32.
  PENumber val 'sd0  expr_wid 32
  ddc_cfir.v:64: debug: Calculate bit select [32'sd0] from range [0:3].
  PENumber val 'sd2  expr_wid 1
  ddc_cfir.v:64: debug: elaborate expression (NTAPS['sd0])/('sd2) expr_width=1

Notice that expr_width is 1. Tracing the code a bit I see:

  ...
  PEAssignPattern::elaborate_expr ...
    ...
    for (size_t idx = 0 ; idx < parms_.size() ; idx += 1) {
      NetExpr*tmp = parms_[idx]->elaborate_expr(des, scope, elem_type, flags);
      ...

ending up in:

  ...
  PExpr::elaborate_expr ...

where the width is explicitly set to 1. The attached patch passes a quick
smoke test, however I have no idea if it's correct or the best solution.

PatchJLW01.txt

[jlwehle]

Here's another test case which produces a slightly different crash:

module tc_lpva (
  output reg signed [15:0] y,
  input [1:0]              f
  );

  localparam integer NTAPS [0:3] = '{ 800, 688, 360, 180 };


  always @*
    begin
      y = NTAPS[f];
    end

endmodule

debug shows:

tc_lpva.v:11: elaborate_expr_param_: Parameter: NTAPS[f]
tc_lpva.v:11: elaborate_expr_param_: par_type: 11netuarray_t
tc_lpva.v:11: symbol_search: scope: tc_lpva
tc_lpva.v:11: symbol_search: path: f
tc_lpva.v:11: symbol_search: Looking for f in scope tc_lpva prefix_scope=0
tc_lpva.v:11: PEIdent::test_width: f is a net, type=logic, width=2, signed_=false, use_depth=0, packed_dimensions=1, unpacked_dimensions=0
tc_lpva.v:11: elab_and_eval: test_width of f
tc_lpva.v:11:              : returns type=logic, context_width=-1, signed=0, force_expand=0, expr_width=2, mode=sized
tc_lpva.v:11:              : cast_type=<no_type>
tc_lpva.v:11: elab_and_eval: Calculated width is 2.
tc_lpva.v:11: PEIdent::elaborate_expr: path_=f
tc_lpva.v:11: symbol_search: scope: tc_lpva
tc_lpva.v:11: symbol_search: path: f
tc_lpva.v:11: symbol_search: Looking for f in scope tc_lpva prefix_scope=0
tc_lpva.v:11: PEIdent::elaborate_expr: Symbol search found base_path=f, member_path=<nil>, par=0, net=0x41116140, eve=0
tc_lpva.v:11: PEIdent::elaborate_expr_net: net=f, net->unpacked_dimensions()=0, net->get_scalar()=false, net->net_type()=netvector_t:logic unsigned[1:0]
tc_lpva.v:11: PEIdent::elaborate_expr: Expression as net. expr_wid=2, tmp->expr_width()=2, tmp=f[1:0]
tc_lpva.v:11: debug: Calculate bit select [f[1:0]] from range [0:3].
Segmentation fault (core dumped)

[caryr]

Give me a week or so and I'll try to help. I have been swamped at work for most of the last year, but it looks like things may slow down a bit soon.

[jlwehle]

That's fine.
In the meantime here's another test case with a different failure ... I'm attaching these in part so I don't lose them.

module tc_lpa_cs
  (
  input              clk
  );

  localparam integer NTAPS [0:1] = '{ 32, 688 };

  localparam integer NCOEFF [0:1] = '{ NTAPS[0] / 2,
                                       NTAPS[1] / 2 };

  localparam integer BADDRS [0:1] = '{ 0,
                                       NCOEFF[0] };

  reg [8:0]          baddr;

  always @(posedge clk)
    begin
      baddr <= BADDRS[1];
    end

endmodule

The failure shows up as:

iverilog -g2009 -o tc_lpa_cs tc_lpa_cs.v
...
<toplevel>: debug:  finishing with 1 root scopes 
tc_lpa_cs.v:18: Sorry: cannot evaluate VEC4 expression (26)
error: Code generation had 1 error(s).

[jlwehle]

What I see while debugging is:

  localparam integer NTAPS [0:1] = '{ 32, 688 };
  localparam integer NCOEFF [0:1] = '{ NTAPS[0] / 2, NTAPS[1] / 2 };
  localparam integer BADDRS [0:1] = '{ 0, NCOEFF[0] };
  ...
  baddr <= BADDRS[1];

being processed as:

  tc_lpa_cs.v:11: evaluating parameter BADDRS, param_type: 0x40900cc0(base type: logic), use_type: logic.
  tc_lpa_cs.v:11: elab_and_eval: pe='{'sd0, NCOEFF['sd0]}, lv_net_type=11netuarray_t
  ...
  tc_lpa_cs.v:12: symbol_search: scope: tc_lpa_cs
  tc_lpa_cs.v:12: symbol_search: path: NCOEFF['sd0]
  tc_lpa_cs.v:12: symbol_search: Looking for NCOEFF['sd0] in scope tc_lpa_cs prefix_scope=0
  ...
  tc_lpa_cs.v:8: evaluate_parameter_logic_: Parameter value: '{(NTAPS['sd0])/('sd2), (NTAPS['sd1])/('sd2)}
  tc_lpa_cs.v:8: evaluate_parameter_logic_: Elaborated value: '{(32'b00000000000000000000000000100000)/(32'b00000000000000000000000000000010), (32'b00000000000000000000001010110000)/(32'b00000000000000000000000000000010)}
  tc_lpa_cs.v:11: evaluate_parameter_logic_: Parameter type: 11netuarray_t
  tc_lpa_cs.v:11: evaluate_parameter_logic_: Parameter value: '{'sd0, NCOEFF['sd0]}
  tc_lpa_cs.v:11: evaluate_parameter_logic_: Elaborated value: '{32'sd0, '{(32'b00000000000000000000000000100000)/(32'b00000000000000000000000000000010), (32'b00000000000000000000001010110000)/(32'b00000000000000000000000000000010)}}

Notice that NCOEFF['sd0] has been replaced by the entire contents of NCOEFF, rather
than the value of the first entry. Tracing the code a bit I see:

  ...
  PEIdent::elaborate_expr(Design*des, NetScope*scope, ivl_type_t ntype, ...
    ...
    symbol_search(this, des, use_scope, path_, net, par, eve, par_type, cls_val);

    if (par != 0) {
      return (NetExpr *)par;
    }
    ...

when given NCOEFF['sd0] returns the array rather than the requested entry.

The attached patch is a simplified version of what's present in:

PEIdent::elaborate_expr(Design*des, NetScope*scope, unsigned expr_wid, ...

and passes a quick smoke test, however I have no idea if it's correct or the best solution.

PatchJLW02.txt

[jlwehle]

Is this pull request still active?

I used this in some projects so I am willing to put some time towards getting this feature into shape for inclusion into a future release if someone can guide me on what needs to happen.

[mole99]

Hi, given the time that has passed since this PR was opened, I believe the original author will not complete it.

Pinging @martinwhitaker and @caryr since both commented on this PR, maybe one of you has some time to guide jlwehle.