New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix interface reference tracing #1595
Comments
Original Redmine Comment The scopes are known once V3Scope completes, so yes they are known. I'm not sure what the easiest way to know from the instantiating module which interface scope applies. The ifaceref has a cell pointer, and that cell has a name which could determine the scope. Maybe IfaceRefDtype should be enhanced to record the iface's scope? Need to do some research to see if there's a nicer way already. |
Original Redmine Comment I'm now thinking that the cleaner way to do this is to TraceDecl the entire interface. That way we can emit a TraceDecl method for the actual interface and just call it with the name and scope for every interface reference we have. Unfortunately, it seems that an IFACEREFDTYPE has only a cell or iface pointer (at least that's what I'm seeing):
But this visitor remembers the VARSCOPE that brought it here:
So now I have the VARSCOPE, and by extension the VAR. The IFACE SCOPEs are created in ScopeVisitor, which traverses CELLs to create SCOPEs. This has nothing to do with VARs so I think ultimately I'll need to do: IFACEREFDTYPE -> VARSCOPE or (VARSCOPE -> VAR) -> CELL -> SCOPE SCOPEs already have a CELL pointer, but not the other way around. However, that was easy to add. So now all I'm missing is a VARSCOPE -> CELL or VAR -> CELL pointer. Is there any place where a VAR or VARSCOPE of an interface signal can be associated with the interface's CELL? I'm looking around but haven't found it yet. |
Original Redmine Comment You can't have cells point to varscopes as it's a many-to-one arrangement, which is why scopes exist in the first place. See e.g. the debug tree file for t_inst_dtree_inla.v. VARs, and I think IFACEREFDTYPEs likewise (I believe) can be used in multiple SCOPEs. It's probably a good idea to make a test like t_inst_dtree* to see how they work. Maybe we should have V3Scope create a new IFACECELLSCOPE, then use that to create the trace (or create a new TRACEDECLSCOPE.) |
Original Redmine Comment What I've ended up on is creating a new AstIntfRef which is a child of AstCell nodes which are interfaces. These always have the full scope name so that when TraceDecl runs it can create a new method for all the decl* needed for the interface and then call this new method once for each interface reference with the correct scope. The AstIntfRef nodes are created at the end of inlining and learns about interface references either by looking at AssignVarScope nodes or by traversing Cell -> Module links for modules which haven't been inlined. Since the trace signal's name now has two components, I passed this change along through to the VCD/FST runtime. However, if it's better to preserve that API, I can concatenate the two C strings in the emitted code. One thing I noticed along the way was that this m_initSubStmts term in V3TraceDecl.cpp didn't seem to be needed unless you set --output-split-ctrace to a negative value: The documentation doesn't talk about what a negative value there would mean, so I think this term isn't needed. One problem I've found with my solution is that if you have a cell which is in a multiply instantiated non-inlined module, the interface references to the different interface instances all get placed under the same AstCell which messed up the trace file in the end. I solved this by comparing scopes during TraceDecl: But this is obviously not ideal. I'm still looking into a better way to do this, but any suggestions on how to solve this more elegantly or if I should be taking a different path altogether are welcomed. |
Original Redmine Comment I don't understand the need to change the include/ headers. I would think calling these decl functions with the proper name argument (perhaps edited in generated code) would be sufficient? Commented on 2 small patches in github. |
Original Redmine Comment Yeah, I'll just need some C str functions in the generated code to concatenate the two parts of the name. I'll do that and leave the runtime alone. |
Original Redmine Comment Latest updates are still in this branch: I am now concatenating the trace signal name in the generated code. It looks like so:
and here is the code: I also realized that this interacted with --protect-ids (and broke it for interfaces). This has been fixed here: However, I'm wondering if this is OK because before my change protectWordsIf() was contained to V3File and V3EmitC*. Is it acceptable to add it to TraceDecl (since I'm pre-emitting here)? Or should I defer this to the EmitC step? |
Original Redmine Comment From your post:
You can't use runtime array sizes on the stack as that makes MSVC++ throw up. Anyhow there's a much cleaner way:
Please defer it. The theory/intent is the Ast's (and .tree output) always have the unprotected names so that debug and error messages can always easily see what's going on.
Nit: For new classes (where not aligning with old stuff), please use
Does this have to be done as another whole visitor pass? Assuming so, add this:
Which will make it a lot faster/less expensive as it'll skip iterating some stuff.
|| goes first on line.
Think you need to deal with lastDot being npos. Think its possibly valid (not just assert), consider adding a test with appropriate flattening. |
Original Redmine Comment
Sorry, you mentioned this before on GitHub and I lost track of it. Are you talking about /verilator public_flat/? I'm trying to create an AstScope for an interface with no dots in the name, but am unable to do so. If I apply public_flat to the signals in the interface that does not seem to affect the scope's name. And I can't apply public_flat to the interface or the instantiation of the interface. |
Original Redmine Comment I wasn't sure if a no-dot was possible. If you can't get it, just add an assert please. |
Original Redmine Comment
Modulo that, I think I've got everything else taken care of: Verilated::catName() didn't work out because it concatenates with a '.' not a space, which is what the trace signal names need. I am now allocating and freeing the memory to avoid the runtime array size. But if you'd prefer I can add an argument to Verilated::catName() or make a Verilated::vcdCatName(). I think the logic actually handles npos correctly now, but since I can't make it happen, I'm asserting. If we find a case where it does happen, I'll take a look. But we may just have to remove the assert (and add a test) at that point. |
Original Redmine Comment Please add a new optional third arg to catName defaulting to '.'. Or, if
Two spaces before //:
Add:
This checks for conflicts (before had this often had two different visitors collide in their user use). Then remove the user1ClearTree at the constructor (is redundant).
s!; //!; //!
|| on beginning of next line.
s/scope/scopep/.
callp->argTypes("vlSymsp, vcdp, code");
While logically fine, I think MSVCC will warn about mixing an assignment in a compound statement.
Not sure why you need nextIrp and a while loop versus just this:
|
Original Redmine Comment I believe everything is handled here now:
Re: clang formatting, I tried to follow your suggestions, but some I'm not entirely sure about. I tried clang-format-diff before I noticed the commit log message saying not to blindly use it. One day . . . |
Original Redmine Comment Looks good, squash and push at will. Thanks for the cleanups; note clang-format is messed up with #ifdefs and a few other things, so certainly don't trust it blindly. If I had copious free time I'd work on improving it.... Also haven't cleaned up a lot of the legacy code to be compliant. |
Original Redmine Comment Squashed and pushed. |
Original Redmine Comment In 4.024. |
Author Name: Todd Strader (@toddstrader)
Original Redmine Issue: 1595 from https://www.veripool.org
Original Assignee: Todd Strader (@toddstrader)
I discovered that my commit for #� is not yet working (and unfortunately escaped into 4.022). The problem is that interface references may not get the right interface instance's data in the trace if there are multiple instances of the same interface. See:
https://github.com/toddstrader/verilator-dev/tree/iface-trace-perf
The problem I'm hitting is that the TraceDecl code finds a VARSCOPE:
And then looks up its data type:
Which points at the IFACE:
Which has all the SCOPEs/VARSCOPEs for all the interface instances. Is it known at this stage (TraceDeclVisitor) which interface instance the VARSCOPE(e.g. TOP->t__DOT__c1__DOT__isub)/IFACEREFDTYPE points to? If not, is there a latter stage I can defer this bit of TraceDecl to that does have this information?
The text was updated successfully, but these errors were encountered: