Add vpiDefName 2 #3938
Add vpiDefName 2 #3938
Conversation
|
So I fixed the tests but I still feel like it's a WIP. It won't break anything but it doesn't really implement the feature correctly bc of the ASTscopename visitor labelling things as modules, and not always getting the defname right. But from the failing tests it looks like something relied on those being "scope_other" even though they are modules, so getting it perfect may be tricky. I'm getting the incorrect names on a larger design so it could be hard to reproduce in a test. |
AndrewNolte
force-pushed
the
an/defname-updated
branch
3 times, most recently
from
236b866
to
5b9aa95
Compare
|
So I think I figured out why it gets those wrong, but not sure how to fix it. If a module doesn't have any public signals inside that direct layer, it doesn't get processed and put into m_vpiScopeCandidates via the AstCellInline or the AstScope visitors. And so it doesn't get the module name overwritten correctly in varHierarchyScopes. Do you think there's a low perf cost way to get these leaf modules to get "SCOPE_MODULE" and the defname correctly? |
|
You mean to have all modules be included in vpiScopeCandidates? I think you should be able to add to that variable on each AstNodeModule visit. BTW make sure you are up to date with master as I fixed a bug in that code a few days ago. |
AndrewNolte
force-pushed
the
an/defname-updated
branch
from
0aa7fc4
to
1e9f8e6
Compare
| m_scopeNames.insert(std::make_pair( | ||
| scpSym, ScopeData{scpSym, scpPretty, 0, "SCOPE_OTHER"})); | ||
|
|
||
| if (varp->isSigUserRdPublic()) { |
There was a problem hiding this comment.
So what was happening was it was only calling varHierarchyScopes on modules that had public signals in them, and so only copying over those orig names and SCOPE_MODULE types. Now this will call it on those, but not add any new scopes if they don't exist. I'm worried this may impact build times though by iterating through more signals, so I'd recommend benchmarking before merging. @wsnyder
There was a problem hiding this comment.
Actually noticing quite a bit of build time impact, I'll change this so it just adds the modules and not a bunch of signals
|
@AndrewNolte might you have time to finish this off? |
|
Closing due to age - @AndrewNolte if you get back to this please open a new pull request. |
Reopen of #3931