Project

General

Profile

[logo] 
 
Home
News
Activity
About/Contact
Major Tools
  Dinotrace
  Verilator
  Verilog-mode
  Verilog-Perl
Other Tools
  IPC::Locker
  Parallel::Forker
  Voneline
General Info
  Papers

Issue #1595

Fix interface reference tracing

Added by Todd Strader 29 days ago. Updated 2 days ago.

Status:
Closed
Priority:
Normal
Assignee:
Category:
-
% Done:

0%


Description

I discovered that my commit for bug1594 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/IFACEREFDTYPE points to? If not, is there a latter stage I can defer this bit of TraceDecl to that does have this information?


Related issues

Related to Issue #1594: Add interface port visibility in traces Closed

History

#1 Updated by Todd Strader 29 days ago

  • Related to Issue #1594: Add interface port visibility in traces added

#2 Updated by Wilson Snyder 29 days ago

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.

#3 Updated by Wilson Snyder 26 days ago

  • Status changed from New to Assigned

#4 Updated by Todd Strader 21 days ago

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.

#5 Updated by Wilson Snyder 21 days ago

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

#6 Updated by Todd Strader 11 days ago

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: https://github.com/toddstrader/verilator-dev/commit/a95639d4b608f5ea339e0eff4ef43680ab1a4a60

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: https://github.com/toddstrader/verilator-dev/commit/3832e91ad15347d6296ec56b63f4c6b44070d241

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.

#7 Updated by Wilson Snyder 11 days ago

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.

#8 Updated by Todd Strader 11 days ago

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.

#9 Updated by Todd Strader 8 days ago

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: https://github.com/toddstrader/verilator-dev/commit/92da07a7e61b41af359c51f09d56425b572ec87a#diff-6f223b255be8ed239e3a554646e33fc8

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

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?

#10 Updated by Wilson Snyder 8 days ago

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: https://github.com/toddstrader/verilator-dev/commit/a044cd47fba33e5b0e67830458e73ce756d1623d

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.

#11 Updated by Todd Strader 7 days ago

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.

#12 Updated by Wilson Snyder 7 days ago

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

#13 Updated by Todd Strader 6 days ago

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.

#14 Updated by Wilson Snyder 5 days ago

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

#15 Updated by Todd Strader 4 days ago

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

#16 Updated by Wilson Snyder 4 days ago

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.

#17 Updated by Todd Strader 3 days ago

  • Status changed from Assigned to Resolved

Squashed and pushed.

#18 Updated by Wilson Snyder 2 days ago

  • Status changed from Resolved to Closed

In 4.024.

Also available in: Atom