Skip to content
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

Closed
veripoolbot opened this issue Nov 11, 2019 · 16 comments
Closed

Fix interface reference tracing #1595

veripoolbot opened this issue Nov 11, 2019 · 16 comments
Assignees
Labels
resolution: fixed Closed; fixed

Comments

@veripoolbot
Copy link
Contributor


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:

     1:2:2:1: VARSCOPE 0x560cca5781b0 <e971> {d63} @dt=0x560cca56e410@(w1)iface  TOP->t__DOT__c1__DOT__isub -> VAR 0x560cca56c070 <e742> {d63} @dt=0x560cca56e410@(w1)iface  t__DOT__c1__DOT__isub IFACEREF

And then looks up its data type:

     3:1: IFACEREFDTYPE 0x560cca56e410 <e743> {d63} @dt=this@(w1)iface if=ifc mp=out_modport -> IFACE 0x560cca504ea0 <e253> {d8}  ifc  L5 [LIB]

Which points at the IFACE:

     1: IFACE 0x560cca504ea0 <e253> {d8}  ifc  L5 [LIB]
     1:2: VAR 0x560cca50dc60 <e565> {d9} @dt=0x560cca50f2e0@(G/sw32)  hidden_from_isub VAR
     1:2: VAR 0x560cca50e060 <e566> {d10} @dt=0x560cca50f2e0@(G/sw32)  value VAR
     1:2: MODPORT 0x560cca50e450 <e19> {d11}  out_modport
     1:2: SCOPE 0x560cca574800 <e931> {d22}  TOP.__PVT__t__DOT__itop
     1:2:1: VARSCOPE 0x560cca55e820 <e933> {d9} @dt=0x560cca50f2e0@(G/sw32)  TOP.__PVT__t__DOT__itop->hidden_from_isub -> VAR 0x560cca50dc60 <e565> {d9} @dt=0x560cca50f2e0@(G/sw32)  hidden_from_isub VAR
     1:2:1: VARSCOPE 0x560cca5748f0 <e936> {d10} @dt=0x560cca50f2e0@(G/sw32)  TOP.__PVT__t__DOT__itop->value -> VAR 0x560cca50e060 <e566> {d10} @dt=0x560cca50f2e0@(G/sw32)  value VAR
     1:2: SCOPE 0x560cca5749c0 <e938> {d23}  TOP.__PVT__t__DOT__itop2
     1:2:1: VARSCOPE 0x560cca574ab0 <e940> {d9} @dt=0x560cca50f2e0@(G/sw32)  TOP.__PVT__t__DOT__itop2->hidden_from_isub -> VAR 0x560cca50dc60 <e565> {d9} @dt=0x560cca50f2e0@(G/sw32)  hidden_from_isub VAR
     1:2:1: VARSCOPE 0x560cca574b80 <e943> {d10} @dt=0x560cca50f2e0@(G/sw32)  TOP.__PVT__t__DOT__itop2->value -> VAR 0x560cca50e060 <e566> {d10} @dt=0x560cca50f2e0@(G/sw32)  value VAR

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?

@veripoolbot
Copy link
Contributor Author


Original Redmine Comment
Author Name: Wilson Snyder (@wsnyder)
Original Date: 2019-11-11T13:34:23Z


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.

@veripoolbot
Copy link
Contributor Author


Original Redmine Comment
Author Name: Todd Strader (@toddstrader)
Original Date: 2019-11-19T12:39:46Z


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):

Breakpoint 4, TraceDeclVisitor::visit (this=0x7ffd450b6e80, nodep=0x55b6866943b0) at ../V3TraceDecl.cpp:204
204             if (m_traVscp && nodep->ifacep()) {
(rr) p nodep->dumpGdb()
This=IFACEREFDTYPE 0x55b6866943b0 back=0x55b686678d40 next=0x55b686694650 headtail=0
  Fileline = t/t_interface1_modport.v:63: 
  IFACEREFDTYPE 0x55b6866943b0 <e743> {d63} @dt=this@(w1)iface if=ifc mp=out_modport -> IFACE 0x55b68662aea0 <e253> {d8}  ifc  L5 [LIB]
$3 = void
(rr) p nodep->m_cellp 
$4 = (AstCell *) 0x0

But this visitor remembers the VARSCOPE that brought it here:

(rr) p m_traVscp->dumpGdb()
This=VARSCOPE 0x55b68669e1b0 back=0x55b68669bbe0 next=0x55b68669e280 headtail=0 iterpp=0x7ffd450b6a00*=0x55b68669e1b0
  Fileline = t/t_interface1_modport.v:63: 
  VARSCOPE 0x55b68669e1b0 <e971> {d63} @dt=0x55b6866943b0@(w1)iface  TOP->t__DOT__c1__DOT__isub -> VAR 0x55b686692010 <e742> {d63} @dt=0x55b6866943b0@(w1)iface  t__DOT__c1__DOT__isub IFACEREF

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.

@veripoolbot
Copy link
Contributor Author


Original Redmine Comment
Author Name: Wilson Snyder (@wsnyder)
Original Date: 2019-11-20T03:21:38Z


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.)

@veripoolbot
Copy link
Contributor Author


Original Redmine Comment
Author Name: Todd Strader (@toddstrader)
Original Date: 2019-11-29T17:14:07Z


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.
https://github.com/toddstrader/verilator-dev/tree/iface-trace-perf

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:
toddstrader/verilator-dev@a95639d

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:
toddstrader/verilator-dev@3832e91

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.

@veripoolbot
Copy link
Contributor Author


Original Redmine Comment
Author Name: Wilson Snyder (@wsnyder)
Original Date: 2019-11-29T19:20:28Z


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.

@veripoolbot
Copy link
Contributor Author


Original Redmine Comment
Author Name: Todd Strader (@toddstrader)
Original Date: 2019-11-29T20:13:48Z


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.

@veripoolbot
Copy link
Contributor Author


Original Redmine Comment
Author Name: Todd Strader (@toddstrader)
Original Date: 2019-12-02T21:19:47Z


Latest updates are still in this branch:
https://github.com/toddstrader/verilator-dev/tree/iface-trace-perf

I am now concatenating the trace signal name in the generated code. It looks like so:

         {
             size_t nameLen = strlen("value");
             nameLen += strlen(scope) + 1;
             char name [nameLen];
             strcpy(name, scope);
             strcat(name, " ");
             strcat(name, "value");
             vcdp->declBus(c+2,name,-1,31,0);
         }

and here is the code:
toddstrader/verilator-dev@92da07a#diff-6f223b255be8ed239e3a554646e33fc8

I also realized that this interacted with --protect-ids (and broke it for interfaces). This has been fixed here:
toddstrader/verilator-dev@a044cd4

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?

@veripoolbot
Copy link
Contributor Author


Original Redmine Comment
Author Name: Wilson Snyder (@wsnyder)
Original Date: 2019-12-03T00:37:52Z


From your post:

         size_t nameLen = strlen("value");
         nameLen += strlen(scope) + 1;
         char name [nameLen];
         strcpy(name, scope);
         strcat(name, " ");
         strcat(name, "value");
         vcdp->declBus(c+2,name,-1,31,0);

You can't use runtime array sizes on the stack as that makes MSVC++ throw up. Anyhow there's a much cleaner way:

         vcdp->declBus(c+2, Verilated::catName(scope, " value"), -1,31,0);

I also realized that this interacted with --protect-ids (and broke it for
interfaces). This has been fixed here:
toddstrader/verilator-dev@a044cd4

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?

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.

+class AstIntfRef : public AstNode {
+    // An interface reference
+private:
+    string      m_name;         // Name of the reference

Nit: For new classes (where not aligning with old stuff), please use
clang-format indentation:

     string m_name;  // Name of the reference


diff --git a/src/V3Inline.cpp b/src/V3Inline.cpp
+class InlineIntfRefVisitor : public AstNVisitor {

Does this have to be done as another whole visitor pass?

Assuming so, add this:

 virtual void visit(AstNodeMath*) {}  // Accelerate
 virtual void visit(AstNodeStmt*) {}  // Accelerate

Which will make it a lot faster/less expensive as it'll skip iterating some stuff.

diff --git a/src/V3TraceDecl.cpp b/src/V3TraceDecl.cpp
         string name = basep->name()+"__"+cvtToStr(++m_funcNum);
         AstCFunc* funcp = NULL;
-        if (basep->funcType()==AstCFuncType::TRACE_INIT) {
+        if (basep->funcType()==AstCFuncType::TRACE_INIT ||
+            basep->funcType()==AstCFuncType::TRACE_INIT_SUB) {

|| goes first on line.

+                size_t lastDot = scopeName.find_last_of('.');

Think you need to deal with lastDot being npos. Think its possibly valid (not just assert), consider adding a test with appropriate flattening.

@veripoolbot
Copy link
Contributor Author


Original Redmine Comment
Author Name: Todd Strader (@toddstrader)
Original Date: 2019-12-03T15:00:06Z


Think you need to deal with lastDot being npos. Think its possibly valid (not just assert), consider adding a test with appropriate flattening.

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.

@veripoolbot
Copy link
Contributor Author


Original Redmine Comment
Author Name: Wilson Snyder (@wsnyder)
Original Date: 2019-12-03T17:34:56Z


I wasn't sure if a no-dot was possible. If you can't get it, just add an assert please.

@veripoolbot
Copy link
Contributor Author


Original Redmine Comment
Author Name: Todd Strader (@toddstrader)
Original Date: 2019-12-04T13:11:36Z


Does this have to be done as another whole visitor pass?
I tried to add this to existing visitors before but it wasn't working out. I'll take another look now that everything is a little more stable.

Modulo that, I think I've got everything else taken care of:
https://github.com/toddstrader/verilator-dev/tree/iface-trace-perf

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.

@veripoolbot
Copy link
Contributor Author


Original Redmine Comment
Author Name: Wilson Snyder (@wsnyder)
Original Date: 2019-12-06T00:19:02Z


Please add a new optional third arg to catName defaulting to '.'. Or, if
you prefer a new function. I'm not opposed to include/ changing, but would
prefer not to change the tracing functions. (Otherwise if we have a design
with a lot of interfaces, all of that code inserted to make the name will
just slow the compiler down, not to mention look unfortunate.)

+++ b/src/V3AstNodes.h
@@ -1833,6 +1833,17 @@ public:
+    string m_name;      // Name of the reference

Two spaces before //:

 string m_name;  // Name of the reference


+++ b/src/V3Inline.cpp
@@ -613,6 +613,108 @@ public:
+    // NODE STATE
+    //   AstVar::user1p()   // AstCell which this Var points to

Add:

 AstUser2InUse       m_inuser2;

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).

+    string m_scope; // Scope name

s!; //!; //!

+                if ((cellp = VN_CAST(fromVarp->user1p(), Cell)) ||
+                    (cellp = irdtp->cellp())) {

|| on beginning of next line.

+++ b/src/V3TraceDecl.cpp
@@ -83,25 +83,32 @@ private:
-        funcp->argTypes(EmitCBaseVisitor::symClassVar()+", "+v3Global.opt.traceClassBase()+"* vcdp, uint32_t code");
+        string argTypes(EmitCBaseVisitor::symClassVar()+", "+v3Global.opt.traceClassBase()+"* vcdp, uint32_t code");
+        if (m_interface) argTypes += ", const char* scope";

s/scope/scopep/.
Break long lines to < 100 chars when edit them.

+        string args("vlSymsp, vcdp, code");
+        callp->argTypes(args);

callp->argTypes("vlSymsp, vcdp, code");

@@ -147,6 +154,43 @@ private:

+    virtual void visit(AstScope* nodep) {
+        AstCell* cellp;
+        if ((cellp = VN_CAST(nodep->aboveCellp(), Cell))
+            && VN_IS(cellp->modp(), Iface)) {

While logically fine, I think MSVCC will warn about mixing an assignment in a compound statement.

        AstCell* cellp = VN_CAST(nodep->aboveCellp(), Cell);
        if (cellp && VN_IS(cellp->modp(), Iface)) {


+                AstIntfRef* nextIrp = cellp->intfRefp();
+                while (nextIrp) {
+                    AstIntfRef* irp = nextIrp;
+                    nextIrp = VN_CAST(irp->nextp(), IntfRef);

Not sure why you need nextIrp and a while loop versus just this:

for (AstIntfRef* nextIrp = cellp->intfRefp();
     nextIrp; nextIrp = VN_CAST(irp->nextp(), IntfRef)) {

@veripoolbot
Copy link
Contributor Author


Original Redmine Comment
Author Name: Todd Strader (@toddstrader)
Original Date: 2019-12-06T21:21:39Z


I believe everything is handled here now:
https://github.com/toddstrader/verilator-dev/tree/iface-trace-perf

Does this have to be done as another whole visitor pass?
I looked again and could not figure out a way to merge this into another visitor. I was hoping to use ScopeVisitor, but the AstAssignVarScopes point to interface dtypes which don't point to cells. And after that pass, the AstAssignVarScopes go away so I can't get the necessary information anymore.

Please add a new optional third arg to catName defaulting to '.'.
I made this argument a const char* instead of a char so that we can still use strcat (instead of character twiddling) and have the option for arbitrary sized delimiters. If preferred, I can switch it to a char and copy that and '\0' into the buffer.

Not sure why you need nextIrp and a while loop versus just this:
I started with a for loop, but that stopped working when I began unlinking the IntfRefs from the Cell and adding them to the CCall. This prefetches nextp() to avoid the problem. I added a comment to the code to explain this.

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 . . .

@veripoolbot
Copy link
Contributor Author


Original Redmine Comment
Author Name: Wilson Snyder (@wsnyder)
Original Date: 2019-12-06T23:59:34Z


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.

@veripoolbot
Copy link
Contributor Author


Original Redmine Comment
Author Name: Todd Strader (@toddstrader)
Original Date: 2019-12-07T17:41:35Z


Squashed and pushed.

@veripoolbot
Copy link
Contributor Author


Original Redmine Comment
Author Name: Wilson Snyder (@wsnyder)
Original Date: 2019-12-08T13:11:12Z


In 4.024.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
resolution: fixed Closed; fixed
Projects
None yet
Development

No branches or pull requests

2 participants