Explicit array scope support

[gezalore]

As far as I can tell, there is currently no way to emit an array scope type into the FST.

Here is an example that demonstrates the problem this causes:

 module t;

     typedef struct packed {
       logic a;
       logic b;
       logic [1:0][0:0] arr;
     } struct_t;

     struct_t [2:0][1:0] thing = '0;

     initial forever begin
       $dumpfile("dump.fst");
       $dumpvars;
       while (!&thing) begin
         #1;
         thing = thing + $bits(thing)'(1);
       end
       #1;
       $finish;
     end

  endmodule

Running this with Verilator:

verilator --binary --trace-fst --trace-structs thing.sv && ./obj_dir/Vthing

The dump contains the unrolled struct scopes, and within the structs the unrolled member arrays), and the viewers guess some of the arrayness (based on variable naming I presume). In both GTKWave and Surfer, the signal is presented similarly:

The real world problem is that it's pretty common to have many small arrays of structures in modern SV. I have examples where there are hundreds of these unrolled arrays of structs entries under each instance of a module in the hierarchy, which makes navigating the design very hard.

Circling back, the underlying issue is that there is no way to emit in the FST that a set of signals/struct scopes are part of an array. The scope tree looks like this:

pushScope "t" FST_ST_VCD_MODULE
pushScope "thing[0][0]" FST_ST_VCD_STRUCT
var "a"
var "b"
var "arr[1][0]"
var "arr[0][0]"
popScope // "thing[0][0]" FST_ST_VCD_STRUCT

...

popScope // "t" FST_ST_VCD_MODULE

What I would like is:

pushScope "t" FST_ST_VCD_MODULE
pushScope "thing[2:0]" ARRAY_PACKED
pushScope "thing[0][1:0]" ARRAY_PACKED
pushScope "thing[0][0]" FST_ST_VCD_STRUCT
var "a"
var "b"
pushScope "arr[1:0]" ARRAY_PACKED
pushScope "arr[1][0:0]" ARRAY_PACKED
var "arr[1][0]"
popScope // "arr[1][0:0]" ARRAY_PACKED
pushScope "arr[0][0:0]" ARRAY_PACKED
var "arr[0][0]"
popScope // "arr[0][0:0]" ARRAY_PACKED
popScope // "arr[1:0]" ARRAY_PACKED
popScope // "thing[0][0]" FST_ST_VCD_STRUCT

...

popScope // "thing[0][1:0]" ARRAY_PACKED

...

popScope // "thing[2:0]" ARRAY_PACKED

...

popScope // "t" FST_ST_VCD_MODULE

@tbybell @rfuest, is there a current way to communicate this layout in FST at the moment so the viewers don't have to guess? I failed to find one.

If not, do we have a strategy for extending the FST format in a forward compatible way so adding this does not lead to crashes in viewers that don't understand it?

cc @oscargus, @ekiwi, @TheZoq2, @Lramseyer, @johnjohnlin

[tbybell]

I'd say in fstapi.h:
FST_ST_MAX = 21,

becomes

    FST_ST_ARRAY_DIM = 21,
    FST_ST_MAX = 22,

In fstapi.c, static const char *modtypes[] add an "array_dim" element at the very end past "vhdl_package".

In fst.c if the default switch(h->u.scope.typ) statement ttype of TREE_UNKNOWN causes issues, add for now before the default case just to get something reasonable iconwise up in the GUI as there is no array dimension range icon currently:

case FST_ST_ARRAY_DIM : ttype = TREE_VCD_ST_BEGIN; break;

Then in the FST emission code in Verilator wrap the array emission iterator code with scope/upscope calls for the new scope type as appropriate.

-Tony

[ekiwi]

The way I would like to see this emitted for Surfer would be something like this, which would require us to add a Array type to FST:

- thing (Array)
  - [0] (Array)
    - [0] (VhdlRecord or Struct)
      - a
      - b
      - arr  (Array)
        - [0]  (Array)
          - [0]
        - [1]  (Array)
          - [0]
    - [1] (VhdlRecord or Struct)
      - a
      - b
      - arr  (Array)
        - [0]  (Array)
          - [0]
        - [1]  (Array)
          - [0]
  - [1] (Array)
    - [0] (VhdlRecord or Struct)
      - a
      - b
      - arr  (Array)
        - [0]  (Array)
          - [0]
        - [1]  (Array)
          - [0]
    - [1] (VhdlRecord or Struct)
      - a
      - b
      - arr  (Array)
        - [0]  (Array)
          - [0]
        - [1]  (Array)
          - [0]

This is close to what the Surfer backend generates from GHW files which contain much richer type information.

[johnjohnlin]

I perfer Tony's proposal. Adding FST_ST_ARRAY_DIM = 21, is the cleanest way, but as Tony mentioned old codes will crash.

If the goal is to maximize backward compatibiltiy, I remembered that commercial tools (like vcs) used "$scope fork" for struct, since VCD only supports:

VCD scope_type ::= begin | fork | function | module | task

Grouping the array signals in a module is quite ugly IMO, but at least it helps design navigation.

[tbybell]

IIRC, fsdb2vcd writes out "$scope fork" for struct as well. Over the years I haven't received any complaints that I can remember about module/signal types added in the whatever2vcd tools. It's easy to add scope and signal types to the writer/reader.

BTW, the main thing "wrong" with grouping signals into artificial hierarchies is the flattened dot form no longer matches what is in the RTL, which can be a completely different source of aggravation. I know on large multidimensional arrays it can be cumbersome dealing with the unrolled signals, so anything like this should be an option for the writer. On commercial designs, things get ugly with hundreds if not thousands of dflop assignments inside nested generate loops which is why --sstexclude exists.

I'm going to let you as a group debate among yourselves what format is best for array visualization because others would want a window to pop up with a datatype browser, etc. @wsnyder (Wilson Snyder) probably will have the best judgment call on what is sane.

[wsnyder]

FWIW Verilator's tests on fst2vcd output presently get from it $scope struct not $scope fork.

I agree IEEE only has begin | fork | function | module | task, but viewers I would hope would allow anything there.

I'd suggest FST_ST_UNPACKED_DIM and fst2vcd output as $scope unpacked. (Naming it that way to leave future room for other array types). A viewer could choose to see $scope unpacked and not show it as hierarchy or match it to source dotted scopes if it desires.

Worse case if a viewer breaks and can't change, which I suspect is unlikely, Verilator/*2fst can add a pedantic-vcd option, converting it to e.g. $scope begin or dropping.

[tbybell]

@wsnyder,
b31f2fc..8cd739e lts -> lts

I'll do updates in LTS at the moment to avoid churn in libfst if there are issues, but this should add that feature to LTS. Note that old viewers can deal with it but as a datatype doesn't exist, it will generate a TREE_UNKNOWN (empty) icon.

[gezalore]

Thanks for the quick work Tony. All the arrays involved in the original example I posted are in fact packed, so it's a bit unfortunate we ended up with UNPACKED_DIM.

Note the actual total set of SystemVerilog types that exists and warrants different display drawing in various viewers (not the open source ones), or different processing in other downstream tools that consume waveforms is:

• ARRAY_PACKED
• ARRAY_UNPACKED
• STRUCT_PACKED
• STRUCT_UNPACKED
• UNION_PACKED
• UNION_UNPACKED

(There are also UNION_TAGGED_PACKED, and UNION_TAGGED_UNPACKED, but I don't so much worry about those as they are little used.)

There is already a fstPackType subtype of fstAttrType attributes which could be used to differentiate packed vs unpacked. (There is also fstArrayType). The actual problem I'm trying to solve is the lack of an array scope.

We also already have FST_ST_VCD_STRUCT and FST_ST_VCD_UNION. Would you mind changing FST_ST_UNPACKED_DIM to FST_ST_ARRAY? That way we can represent all crosses of {ARRAY,STRUCT,UNION}x{PACKED,UNPACKED} using scope type + pack type attribute.

[tbybell]

The FST_ST_UNPACKED_DIM hierarchy type Wilson proposed isn't a datatype attribute. It's a fictitious hierarchy in the scope tree that indicates that the following dimension(s) are artificial yellow sticky note grouping indices (the "DIM" part of the define) for some underlying chunk of something so you don't get lost in scrolling in navigating what the simulator has pre-unrolled (if packed) into the FST file. Simulators can do this unrolling for efficiency reasons. If you had a 256kB SRAM stored in a dumpfile addressed as a true packed array, many simulators would not emit it as a single, wide 1-D bitvector every cycle. They'd either throw up their hands and not emit the signal or they would emit individual elements instead.

Someone could sanely argue that the burden should be on the waveform viewer GUI code to aggregate arrays visually back into a programmer-friendly form with nothing else new in the dumpfile, and despite the pain of implementing it, I'd have to say he or she would have a valid point. It gets tricky though because with any form of grouping, you'd then only see array elements in the bottom part of the SST not mixed with other signals, and some users might complain the extra navigation is burdensome (e.g., with smallish arrays).

Note that in dumpfiles on commercial tools for years, indexed elements have largely been unrolled. Only within the last two years has VCS/FSDB for the sim data started truly storing packing arrays (much to my consternation at my day job) as they have private reader API I haven't discovered yet that conveys the dimensions of the packed data. This is real fun when you're working on multiple variant designs at once (e.g., a "P" vs "E" core) that have different queue sizes and widths. RetDec crashes on the reader library, otherwise I'd be all over it. :-)

This gets to be an annoying problem when you peel the onion: there is more because generate is affected as well and we haven't addressed that monster yet. Is someone going to be annoyed that his hierarchy scope is flooded with whatever[0][0] ... whatever[7][255] and want this fix there, too?

If the Surfer guys have opinions, someone please invite them. I suspect this is going to be a lively discussion. :-)

[oscargus]

@ekiwi is the Surfer guy is this regard as he writes the wellen library that Surfer relies on.

Not sure that I really follow the details of the discussion, but my long term goal for Surfer is to move arrays and records from scopes to (expandable) variables, so anything which keeps that information one way or the other (and doesn't rely too much on guessing hierarchy based on the name) I'm happy. Hence, my guess is that the suggestion by @ekiwi is somehow what I'd imagine as that can be generalized to arrays of arrays of records that contains array of records that contains... However, maybe the other ones can do that as well? This said without knowing how FST actually stores the data.

[oscargus]

And while the VCD standard only defines five scope types, this is the complete list of the 19 currently supported in wellen, all found in the wild:

https://github.com/ekiwi/wellen/blob/35c7fd9406d3ad973e773cc39e80709682d75f05/wellen/src/vcd.rs#L668-L694

The point of having detailed types is that the waveform viewer can do something more clever than just displaying it as just another hierarchical scope. Although, admittedly, we do use that to a very limited degree so far. Currently, we only use it to tweak the unique variable name generation so that groups that corresponds to scopes that are variables only displays the local variable name.

Adding another scope type is simple and will help with the name generation. Currently there is a VhdlArray scope type which is generated from GHW files.

[wsnyder]

I think @gezalore's comment may have been misread, plus I then mistranslated his request.

I think all we need now is for you to please rename FST_ST_UNPACKED_DIM to FST_ST_SV_ARRAY, and done. Then we'll use the existing fstPackType to indicate packed versus (no attribute unpacked arrays), similar to how we can use FST_ST_VCD_STRUCT with packed attribute (or not).

As to @oscargus https://github.com/oscargus 's comment, we don't have yet plans for it, but if you want to add these it will be good. Verilator supports these constructs but doesn't specially indicate them in the FST, so easy improvement for the future.

FST_ST_SV_CHECKER
FST_ST_SV_CLOCKING
FST_ST_SV_COVERGROUP
FST_ST_SV_PROPERTY
FST_ST_SV_SEQUENCE

[tbybell]

8cd739e..990b978 lts -> lts
has the change from FST_ST_UNPACKED_DIM to FST_ST_SV_ARRAY. I didn't yet add the other 5 from CHECKER..SEQUENCE.

[wsnyder]

Thanks, note Verilator will need the define/writer support in gtkwave/libfst too.