Original Redmine Comment
Author Name: Jeremy Bennett (@jeremybennett)
Original Date: 2012-05-21T14:10:08Z

Investigating further, the problem is that this is a very early pass. So in fact even a plain constant parameter will also fail. It looks like the strategy must be to postpone evaluation until a later pass.

Original Redmine Comment
Author Name: Wilson Snyder (@wsnyder)
Original Date: 2012-05-21T22:45:20Z

Link at present is a textual match, and it can't resolve cross-module references statically unless they are a constant. The solution as you noted is to move that later, but it's going to be a bit of work.

Original Redmine Comment
Author Name: Guy Hutchison
Original Date: 2012-10-05T00:14:46Z

Any update on this feature? This is very useful for parameterized designs.

Original Redmine Comment
Author Name: Julius Baxter (@juliusbaxter)
Original Date: 2012-10-08T16:11:56Z

I am also wondering if there is an update on this feature.

I'm more interested in the case of addressing members in arrays of structs. Perhaps there's a way to do this, will post to the forum and ask, but it's the same error, so maybe it's to do with the same internal mechanism.

Original Redmine Comment
Author Name: Geoff Barrett
Original Date: 2014-01-23T16:01:45Z

Any updates? I am seeing this error message from the following code:

  1 module m1();
  2 logic v1;
  3 endmodule
  4 
  5 module m2();
  6 for (genvar i = 0; i < 4; i++) begin : m1_b
  7   m1 m1_inst(); 
  8 end
  9 for (genvar j = 0; j < 4; j++) begin
 10   always_comb m1_b[j].m1_inst.v1 = 1'b0;
 11 end
 12 endmodule

Is this the same error? It makes it hard to create a datapath driver for a complex IP where we want to provide an efficient cycle accurate model by keeping the control logic in RTL and replace the datapaths with C++.

Original Redmine Comment
Author Name: Wilson Snyder (@wsnyder)
Original Date: 2014-01-25T14:36:24Z

Yes, sorry this is the same issue. At present all module structures are "flat" (basically separate variables per module) so there is no easy way to index among them.

Original Redmine Comment
Author Name: Todd Strader (@toddstrader)
Original Date: 2014-09-22T10:24:21Z

As mentioned in issue822, I'm interested in kicking this problem around. Caveat: I'm not a compiler-guy, but I'm certainly interested in becoming familiar with the project.

Is it fair to say that the compiler needs to be able to postpone the linking of all not-immediately-discoverable references? Is this just dotted hierarchical references and array slices?

Also, I'm wondering if this would / could lead to something like modular Verilator compilation? If the linking is done after compilation (as in gcc), could the intermediate step be saved to something akin to an object file (maybe more like saving the AST to a file?) so that you could compile portions of a project independently?

When I can pick this up again, I was thinking I should start by commenting out the link step and seeing how things fell apart. I'm assuming the link step is the beginning bit of process() in Verilator.cpp. Is this correct, and do you think it's a reasonable first step?

Thanks in advance for any advice you can provide,
Todd

Original Redmine Comment
Author Name: Wilson Snyder (@wsnyder)
Original Date: 2015-06-04T00:08:32Z

Some notes on solutions for this:

The current approach is based on Verilog 1995, which parses a module (V3Parse), determines sub-modules (V3LinkCells), parses those, then with all modules in memory determines generate statement values (V3Param), and moves on from there.

The proper solution which would fix "all" the issues is to parse the top module, resolve generate statements, then based on what modules exist parse those, then repeat the generate/parse on sub modules. This obviously requires much greater coupling between what are now are separate stages.

As to cells with "arrays" verilator converts FOO[0] to (basically) foo__0__ and deals with it as a flat module. Properly treating it as foo[0] would require different surgery on how the symbol table is done.

It may be possible to hack in the simple cases described (generate value into a cell reference). Where verilator presently convert FOO[0] to foo__0__, instead put in a reference to e.g. a new AstNode CellArray which specifies an as-yet unknown array index number. Later the V3Param processing would determine the constant indexing. This should work as it doesn't require solving the more fundamental and complicated problems above.

If someone is interested in fixing any of these we are interested and I can provide more details, but expect any to require at least a week commitment.

Original Redmine Comment
Author Name: Luke Yang
Original Date: 2015-06-08T14:23:57Z

I am interested in fixing this issue and did some experiments. For the simple case described above, it seemed an early constant collapse would solve the problem. So I tried to add a ConstifyAll call before LinkDotResolve, which however did not work. I would like to know more details about the proper solution. Time and commitment are no problem to me, and I'd appreciate more discussions as they are helpful. Thank you so much!

Original Redmine Comment
Author Name: Luke Yang
Original Date: 2015-06-22T16:51:22Z

A trivial fix to t_gen_index (the test code in the original post) can be found in the attachment. This fix, however, doesn't attack the deeper and more fundamental problem.

Even when there is no gen loop, verilator still can't handle arrayed interfaces correctly. Please see attached test case. It is a modified version of the test case written by Todd Strader in bug 822.

For this test case, after V3Inst::dearrayAll() pass, the IFACEREFTYPE entry in the type table will contain a dangling pointer to a nonexistent AST node, which is found by the program and causes the program to quit.

More specifically, InstDeVisitor::visit(AstCell* nodep, ...) applies to such structure in the AST:

CELL 0xabac50 ... foos -> IFACE 0xa8fe10 ... foo

3: RANGE ...

3:2: CONST ...

3:3: CONST ...

(and in TYPETABLE section:)

IFACEREFDTYPE 0xabd070 ... cell=foos if=foo -> CELL 0xabac50 ... foos -> IFACE 0xa8fe10 ... foo.

It duplicates the CELL node according to the range expression, and hard-codes the cell index to each duplicated entry.
(For example, foos[2:0] would become CELL foos__BRA__2__KET__, CELL foos__BRA__2__KET__, CELL foos__BRA__1__KET__.)

It then removes the RANGE node and its children, as well as its parent, CELL at 0xabac50. All newly created CELL nodes have addresses other than 0xabac50.

However, the type table is not updated accordingly. The IFACEREFDTYPE node at 0xabd070 is still expecting a CELL node at 0xabac50, which once existed but not anymore. This link is found to be broken and caused the program to crash.

I would like to get help on fixing this issue first, as it's well-understood by me and I believe it's a good first step towards fixing the issue regarding arrayed interfaces in general. Could you please offer some suggestions on keeping the type table consistent? Thank you very much.

Original Redmine Comment
Author Name: Wilson Snyder (@wsnyder)
Original Date: 2015-07-09T01:17:13Z

I spent some time looking at how to pull together to have the functions you suggest be callable, but it's a big mess, it violates too many assumptions. I'd suggest the alternative tack of creating an AstCellArray, then much later in V3Param converting it to an AstCell with the proper reference.

Original Redmine Comment
Author Name: Wilson Snyder (@wsnyder)
Original Date: 2015-07-09T02:00:25Z

Sorry I mean a AstCellArrayRef, that is a reference to an as yet unknown cell number.

This has a huge ripple effect.

As you noted right now the full scope e.g. "a.b.c__BRA__0__KET.varname" is generated by V3LinkDot as a string and becomes part of a AstVarXRef as text. Then all later processing uses that textual scope name.

Instead V3LinkDot should form something like AstVarXRef("varname", AstCellNameRef("a", AstCellNameRef("b", AstCellArrayRef("c", expression)))), that is AstNodes that specify how to scope into the cell containing that variable. Then all code that uses AstVarXRef (which is mostly V3LinkDot) needs to understand how to traverse those trees instead of simply using a string.

Original Redmine Comment
Author Name: Todd Strader (@toddstrader)
Original Date: 2015-10-05T11:33:22Z

Back to this one again. Some more questions:

Is it anticipated that this should only impact linking that runs into things that V3Param will later constify? Or is it believed that the linker should just build everything as this chain of references and then squash them all after V3Param has run?

Will this affect just V3LinkDot or the other V3Link* passes as well?

Would it be possible instead to run V3Param on the yet-to-be-constified node from within V3LinkDot instead of the deferred linking suggested above? If I understand correctly, V3Param already does this sort of thing with V3Width.

Is it known if this is only a problem with array slicing? Or are there other should-be-resolvable things that need to be considered?

Original Redmine Comment
Author Name: Wilson Snyder (@wsnyder)
Original Date: 2015-10-06T01:07:41Z

Is it anticipated that this should only impact linking that runs into things that V3Param will later constify? Or is it believed that the linker should just build everything as this chain of references and then squash them all after V3Param has run?

The AstCellArrayRef idea is it's linked to a single one of the arrays. Until after V3LinkDot it doesn't really matter which one it points to as the arrays don't yet have unique variables (e.g. a[0].v and a[1].v both point at the same "v". so we can represent it instead as a[*].v and resolve the * later.)

Will this affect just V3LinkDot or the other V3Link* passes as well?

Not sure until we try it.

Would it be possible instead to run V3Param on the yet-to-be-constified node from within V3LinkDot instead of the deferred linking suggested above? If I understand correctly, V3Param already does this sort of thing with V3Width.

No, V3LinkDot must have the entire and stable variable list across all modules for it to work.

Is it known if this is only a problem with array slicing? Or are there other should-be-resolvable things that need to be considered?

There are other problems with the current approach, such as parsing modules that are not required. The proper approach is to parse and elaborate each module all of the way down, rather than step-by-step, but as also noted that's a lot of work.

Original Redmine Comment
Author Name: Todd Strader (@toddstrader)
Original Date: 2015-10-09T21:30:06Z

First stab, doesn't do everything the issue covers, lots of caveats, etc., etc.:
https://github.com/toddstrader/verilator-linkdot-elab

First off, I'm initially trying to fix t_interface_array, and the simpler variant t_interface_array_broken. t_interface_array_broken now passes, but t_interface_array fails with a new (to me) error which may or may not suggest a different issue. I'm not sure yet. Also, I don't seem to have broken anything else with this change.

I also know for sure that this change won't fix a.b.foos[identity(0)] cases yet. I'm relatively sure I can pick that up easily, but I'd prefer to discuss the changes first.

One major question:

AstVarXRef's parent, AstNodeVarRef, explicitly forbids children. Wilson's suggestion above seems to suggest AstVarXRef having children (may be reading wrong), but instead of breaking AstNodeVarRef's assertion, I created AstUnlinkedVarXRef and stashed the proto-AstVarXRef under it until I could complete it. Is this acceptable?

Beyond that, please take a look at the code and let me know if there is anything else I should consider for this particular case.

Again, to be clear, this is not ready to be pulled yet. I need to do a lot of cleanup and error checking. I also want to go back and support some of the other cases discussed in the issue.

Also, I think I have my head sufficiently around this. You can assign it to me.

Original Redmine Comment
Author Name: Todd Strader (@toddstrader)
Original Date: 2015-10-13T10:07:04Z

I updated the GitHub repo with the solution to the error mentioned above that t_inteface_array was throwing. Basically, the unroller was checking to see if the if a genvar was being used anywhere on the LHS and aborting if so. Since I'm hiding the genvar as the select expression under AstCellArrayRef, this check was failing there. I told the unroller not to look under AstCellArrayRef nodes which gets me past this issue.

What I'm seeing now is that V3Param is trying to flatten my AstUnlinkedVarXRef and throwing this:

%Error: t/t_interface_array_genfor.v:19: Expecting expression to be constant, but variable isn't const: i

I'm guessing this means that the genvar hasn't been expanded yet and that I need to (try to) defer this flattening until after that happens. Hopefully that works. I'll try that next.

t_interface_array_genfor is a test to help me narrow down the current issue I'm seeing. I'll collapse all these tests when I have something final.

Original Redmine Comment
Author Name: Todd Strader (@toddstrader)
Original Date: 2015-10-14T00:41:57Z

I updated the GitHub repo again. My last change was in error since the problem was not in the unroller, but that the LinkLvalue visitor didn't yet know about AstCellArrayRef nodes and that the select expression is not an lvalue. This has been addressed and all tests including t_interface_array are now passing.

Next steps will be to handle other cases discussed here and then cleanup. Comments definitely welcomed.

Original Redmine Comment
Author Name: Wilson Snyder (@wsnyder)
Original Date: 2015-10-15T02:27:09Z

Generally seems might work. Obviously need more in place to be sure.

Only one nit so far - in the V3AstNodes.h class call the names just m_name, and make the accessors "virtual name()" and the default dumpers will print it out, so you won't need the dump() methods.

Original Redmine Comment
Author Name: Todd Strader (@toddstrader)
Original Date: 2015-10-16T21:27:45Z

https://github.com/toddstrader/verilator-dev/tree/issue517

Ready to pull or for further comments.

I've done a bunch of cleanup, and squashed the commits. Jeremy Bennett's test is no longer available, but I created a test based on Geoff Barrett's code snippet. This now works as well.

Original Redmine Comment
Author Name: Wilson Snyder (@wsnyder)
Original Date: 2015-10-16T23:00:42Z

Wow thought there was going to need to be a lot more code added after your last patch, glad not much more was needed. Good stuff.

--- a/src/V3AstNodes.h
+    ASTNODE_NODE_FUNCS(UnlinkedVarXRef, UNLINKEDVARXREF)
+    // ACCESSORS
+    virtual string name()      const { return m_name; }                        // * = Var name
+    AstVarXRef* vxrp()         const { return op1p()->castVarXRef(); }         // op1 = VarXRef
+    AstNode* crp()             const { return op2p(); }                        // op1 = CellArrayRef or CellRef

Nit, please rename varxrefp and cellrefp.


--- a/src/V3LinkDot.cpp

+    virtual void visit(AstCellRef* nodep, AstNUser*) {
+       UINFO(5,"  AstCellRef: "<<nodep<<" "<<m_ds.ascii()<<endl);
+       nodep->iterateChildren(*this);
+    }
+    virtual void visit(AstCellArrayRef* nodep, AstNUser*) {
+       UINFO(5,"  AstCellArrayRef: "<<nodep<<" "<<m_ds.ascii()<<endl);
+       nodep->iterateChildren(*this);
+    }
+    virtual void visit(AstUnlinkedVarXRef* nodep, AstNUser*) {
+       UINFO(5,"  AstUnlinkedVarXRef: "<<nodep<<" "<<m_ds.ascii()<<endl);
+       nodep->iterateChildren(*this);
+    }

While useful for debug, otherwise aren't needed, please remove.


--- a/src/V3Param.cpp
+    virtual void visit(AstCellArrayRef* nodep, AstNUser*) {
+        V3Const::constifyParamsEdit(nodep->selp());
+       if (AstConst* constp = nodep->selp()->castConst()) {
+           string index = AstNode::encodeNumber(constp->toSInt());
+            m_unlinkedTxt += nodep->name() + "__BRA__"+index+"__KET__";
+        } else {
+            nodep->v3error("Could not elaborate dotted reference");

I'd like to see a better error message including the name of the cell,
ditto the other similar lines, e.g. at least

             nodep->v3error("Could not elaborate dotted reference: "<< prettyName(nodep));

also please add a _bad test to check all three of these messages (probably
all three you can hit in one test).


--- test_regress/t/t_gen_index.pl

This is Jeremey's original test.  Remove this line and you'll see it is
better, but is still unhappy, this requires a bit more than your patch so
doesn't have to get fixed to take the patch but would be nice if you could
take a look.

-$Self->{vlt} and $Self->unsupported("Verilator unsupported, #�");

Original Redmine Comment
Author Name: Todd Strader (@toddstrader)
Original Date: 2015-10-17T10:40:40Z

Great, thanks for the feedback. I'll work on the changes and take a look at t_gen_index. Couple questions in the meantime:

It turns out that the V3LinkDot AstUnlinkedVarXRef visitor is superfluous, but the other two are necessary to prevent checkNoDot() from throwing something like this:

%Error: t/t_genfor_hier.v:15: Syntax Error: Not expecting CELLREF under a DOT in dotted expression

This happens because the default visitor is checking to see if something is under a DOT which shouldn't be there. I could solve this other ways (perhaps by doing casting checks in the default visitor or checkNoDot()), but I thought this looked the cleanest. Also, I added the UINFO()'s to look more like the other V3LinkDot visitors. I'll remove the unnecessary visitor, but do you have suggestions about how to handle the other two? Should I just leave them as-is?

Also, what do you do for error reporting when the node has no name (e.g. a DOT)? Is there some clever way to get a meaningful name when there is no name()? I don't see anything like expressionPrettyName(). I'm guessing this just needs to be handled on a case-by-case basis.

Original Redmine Comment
Author Name: Wilson Snyder (@wsnyder)
Original Date: 2015-10-17T11:03:46Z

Forgot LinkDot had an unusual default visitor, so yes you need those. As you suspected for errors you'll need a special custom message. Perhaps m_ds.m_dotText is valid at this point? If there's no obvious additional information that's ok, just make sure the message is somewhat obvious as to what the user did. (As opposed to internal only errors where I'm not very descriptive - as you've seen reading the code!)

Original Redmine Comment
Author Name: Todd Strader (@toddstrader)
Original Date: 2015-10-21T10:06:09Z

OK, ready to pull or for more discussion:

https://github.com/toddstrader/verilator-dev/tree/issue517.2

If you'd rather see the un-squashed changes since last time, look here:

https://github.com/toddstrader/verilator-dev/tree/issue517

I made the name changes requested, and tried to be more user friendly with the error messages (including node names now). t_gen_index now passes with everything else.

I added t_interface_array_bad to check the error messages, but I can only hit 1/3 of them. The two elusive checks are both in the AstCellRef visitor in V3Param. They both verify that the AstCellRef's children are AstCellRef, AstCellArrayRef or AstParseRef. I can't seem to produce this scenario as everything I try gets handled at some earlier parser or syntax checking phase. I left a bunch of things that I tried as comments in the test. If anyone has suggestions on how to produce this, I'm definitely interested.

Regarding the code changes, there were two major assumptions that I had previously which were broken. t_gen_index showed me that I wasn't deferring all the things that could be constified later by V3Param (arithmetic expressions in this case). To solve this, I changed V3LinkDot's AstSelBit visitor to defer all select expressions.

This fixed t_gen_index, but broke some other tests, most notably t_var_xref_gen. It showed me that I assumed that the AstUnlikedVarXRef would always percolate up to the top of the DOT tree. In the case of unpacked arrays, this is not true. To handle this, I now track AstCellRef / AstCellArrayRef nodes looking for an AstVarXRef in the m_ds state so that an AstUnlinkedVarXRef can be built at an arbitrary location in the DOT tree.

Original Redmine Comment
Author Name: Wilson Snyder (@wsnyder)
Original Date: 2015-10-21T10:31:48Z

+    virtual void visit(AstCellArrayRef* nodep, AstNUser*) {
+       UINFO(5,"  AstCellArrayRef: "<<nodep<<" "<<m_ds.ascii()<<endl);
+        // All sorts of possible stuff down here, we'll just save for V3Param
+    }

I'm doubtful that skipping the default visitor is the right thing to do. If for example the CellArrayRef's index value is a varxref (dotted variable from another module), I think the pointer will dangle and it will crash. Please try that in your tests.

Also a nit, please put tabs to indent the front of your new code. You seem to have tabs in some cases, spaces in others.

Otherwise looking good.

Original Redmine Comment
Author Name: Todd Strader (@toddstrader)
Original Date: 2015-10-21T22:20:03Z

Yeah, it doesn't crash, but it can't constify the VarXRef:

%Error: t/t_crazy_sel.v:25: Expecting expression to be constant, but can't convert a VARXREF 'THE_LP' to constant.
%Error: t/t_crazy_sel.v:25: Could not expand constant selection inside dotted reference: THE_LP
%Error: t/t_crazy_sel.v:25: Can't find definition of '' in dotted signal: .foo_inst.y
%Error:      Known scopes under 'y': bar_inst
%Error: Exiting due to 3 error(s)

You can find the test here:

https://github.com/toddstrader/verilator-dev/tree/issue517.crazy_sel

We get through linking fine, but when we try to constify the CellArrayRef with a VarXRef for a select expression, we get the first error message from V3Const because we hit the default visitor. Here's exactly what we're trying to constify:

- V3Param.cpp:265:    CARP selp: VARXREF 0x286d710 <e159> {e25} @dt=0@  THE_LP [RV] <- bar_inst. - VAR 0x2897c70 <e126> {e33} @dt=0@  THE_LP LPARAM

So my question is, is there any simple way to get the constant value that this guy is pointing to? V3Const's error message would suggest there isn't. I'm guessing I can follow varp(), but I'm starting to get out of my depth pretty quickly here.

Original Redmine Comment
Author Name: Todd Strader (@toddstrader)
Original Date: 2015-10-22T00:28:27Z

Looking at the varp I get from the VarXRef, I can tell it is a parameter. I could special case the scenario where a VarXRef is pointing at a parameter and return that constant. Is this getting to special-casey?

Original Redmine Comment
Author Name: Todd Strader (@toddstrader)
Original Date: 2015-10-22T02:08:51Z

I changed the AstVarRef visitor to be an AstNodeVarRef visitor and that seems to have fixed this case:

toddstrader/verilator-dev@a6d1046

I still need to clean up the whitespace, but does this look like it covers the hierarchical dotting case in question?

Original Redmine Comment
Author Name: Todd Strader (@toddstrader)
Original Date: 2015-10-22T21:25:10Z

Cleaned up (including whitespace), squashed and rebased for consideration:

https://github.com/toddstrader/verilator-dev/commits/issue517.3

Original Redmine Comment
Author Name: Wilson Snyder (@wsnyder)
Original Date: 2015-10-23T00:16:16Z

Great. I just reindented the .v files, and cleaned up some code you inherited of mine.

Pushed to git towards 3.878.

Original Redmine Comment
Author Name: Wilson Snyder (@wsnyder)
Original Date: 2015-11-01T13:21:54Z

In 3.878.