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 #1469

VPI module

Added by Stefan Wallentowitz 5 months ago. Updated about 1 month ago.

Status:
Closed
Priority:
Low
Category:
Unsupported
% Done:

0%


Description

Hi,

with the ongoing integration of Verilator with cocotb, one of the first steps is to get a minimal working VPI Module implementation.

As 1. I am not sure I fully grasp the complexity of the VPI Module yet, and 2. cocotb only uses toplevel iteration, I think the following should work:

  1. Don't add a new runtime type VerilatedModule or so, instead extend the VerilatedScope (as representation of vpiInternalScope in the LRM, so kind of navigate in the other direction)
  2. Add the type of the scope to VerilatedScope (module, generate block, etc.)
  3. Add a vpiModule iterator to VPI that traverses the list of scopes and filters by modules

What would be left is the question of how to represent hierarchical relations. I think it could be done in two ways:

  • Adding hierarchical relations to the VerilatedScope, or
  • Implementing the traversal of iterations by parsing the full names at runtime

The first seems more natural, the latter more complex at runtime but "easier" to implement.

Overall, the user of course must be aware of the fact that the hierarchy is generally sparse as only those scopes that are public are visible. So, they could not expect to see anything in iteration that is not made public.

What do you think? Any suggestions/thoughts?

Best, Stefan

vpimodule.patch View (30.6 KB) Stefan Wallentowitz, 09/23/2019 09:55 PM

vpimodule.patch View (33.6 KB) Stefan Wallentowitz, 09/27/2019 04:45 PM

vpimodule.patch View (34.7 KB) Stefan Wallentowitz, 09/28/2019 03:17 PM

bug1469.patch View (35.3 KB) Wilson Snyder, 09/29/2019 05:59 PM

History

#1 Updated by Todd Strader 5 months ago

But we don't want to limit the iteration to just modules, correct? I'm sure I still don't understand enough about how cocotb works, but I would expect it to give me a handle to a signal as long as I give it a canonical scope name. I think that should be true regardless of whether the name includes modules, interfaces or generate blocks.

#2 Updated by Wilson Snyder 5 months ago

  • Status changed from New to Feature

My first reading of this was you needed module scopes, but that is already there, so I think you're saying you want scopes of other sorts of things?

Having a type in VerilatedScope seems reasonable.

I think you need to go with the relations being expressed in the data structures as vpiNext etc require traversal information that can't be done (reasonably) with string operations. Perhaps cocotb doesn't use some of these vpi ops, but we should do something to allow that in the future.

#3 Updated by Stefan Wallentowitz 5 months ago

Essentially, cocotb accesses the signals only by their name. It only uses the vpiModule iterator to find toplevel modules.

So, after discussion with Todd on our discussion channel (https://gitter.im/librecores/cocotb-verilator) I should probably add more information on how I came to the proposal:

The background is that cocotb only calls vpi_iterate(vpiModule, NULL) and then doesn't do anything with the vpiModule. While at some point we want vpiModule to be complete in Verilator, for now that is all we need for this particular task. I am wondering now if it is sufficient to use the method I described above. What I have done before in my mockup implementation is to emit the hierarchy of VerilatedModule to the Verilator runtime. I then created the vpiModule iterator class to point to one hierarchy level (nested vectors..). So, calling vpi_iterate(vpiModule, NULL) then returned a handle that gets casted to this iterator.

I came to the proposal above by thinking that it is not needed to add another infrastructure to generate except VerilatedScope. The drawback is that it requires us to extend VerilatedScope without knowing if this will work in the future.

After discussion with Todd, there is simple workaround for this issue in the current situation, that would work well, but moves the discussion into the future: Only allow ref=NULL in vpi_iterate and return the toplevel scope (which needs to be added to VerilatedModule, and even more importantly made public automatically).

As you say, Wilson, the big question to me now is if we can anticipate the reasonable way forward without playing the whole thing through. Unfortunately, I am not experienced enough with using VPI to make a full judgement. My impression is that we can use the VerilatedScope to build vpiModule on top of it.

So, probably it makes sense to start with adding the type to VerilatedScope. It seems useful overall. Getting from there, we could start with the workaround for the near future and then look into a more usefull vpiModule support later.

#4 Updated by Stefan Wallentowitz 5 months ago

Oh, I forgot to add: Major reason for this proposal was that nothing outside a VerilatedScope can be accessed anyways, right?

#5 Updated by Wilson Snyder 5 months ago

Thanks for the background. I agree adding the type to VpiScope seems most preferred.

#6 Updated by Stefan Wallentowitz about 2 months ago

This has been more complicated than I hoped. Attached I am sending you my proposal for review. It is probably not the best final solution once we need to add support for more operations for vpiModule or other vpiScopes (which is mapped onto VerilatedScope in this case).

The patch does not add any significant overhead on verilation, compilation and runtime for non-VPI design. The major impact on the verilation is that CellInline is alive until the end if --vpi is set. Also, the scope of the CellInline is added so that the full hierarchical name of the cell is available.

For the runtime it then emits extra scopes for all inlined cells, but only for those that are on the hierarchical path to public variables. Those scopes are empty and "only" consume space. Along with those a new runtime type "VerilatedHierarchy" is added, again only for VPI designs. The runtime construction of hierarchy of scopes is emitted too, so that the "visible hierarchy" can be traversed.

Finally, added VPI support for it.

I need to finish the test and add a few comments here and there, but want to put it to review already. Looking forward to your feedback!

Best, Stefan

#7 Updated by Wilson Snyder about 2 months ago

Thanks for working on this. I think your general approach is fine.

I suggest you add a test where there's some backslash-escaped dots in the name of signals and scopes, as there's a lot of code breaking up on ".", and could easily be some bugs messing up a use of name() versus pettyName().

Below are nits.

diff --git a/include/verilated.cpp b/include/verilated.cpp
+void VerilatedHierarchy::add(VerilatedScope *from, VerilatedScope *to) {
+    VerilatedImp::hierarchyAdd(from, to);

Our convention is * is part of type, and spacing and "p" suffix on pointers, so

void VerilatedHierarchy::add(VerilatedScope* fromp, VerilatedScope* top) {

other places too.

diff --git a/include/verilated.h b/include/verilated.h
-    const char* m_namep;  ///< Module name
+    const char*     m_namep;  ///< Module name

Can revert this.

diff --git a/include/verilated_syms.h b/include/verilated_syms.h
+class VerilatedHierarchyMap
+: public std::map&lt;const VerilatedScope*, std::vector&lt;const VerilatedScope*&gt; > {

Perhaps a typedef to cleanup that long std::map line.

diff --git a/include/verilated_vpi.cpp b/include/verilated_vpi.cpp
+#define VERILATED_CPP

Instead please define VERILATED_VPI_CPP (e.g. this filename) then edit the assertion this protects to include this define.

+    explicit VerilatedVpioModule(const VerilatedScope* modulep)
+            : m_modulep(modulep) {
+        m_name = m_modulep->name();
+        if (strncmp(m_name,"TOP.",4) == 0) m_name+=4;

Spaces after commas. Spaces around operator +=. 'if (strncmp(m_name, "TOP.", 4) == 0) m_name += 4;'

diff --git a/src/V3AstNodes.h b/src/V3AstNodes.h
     string      m_name;         // Cell name, possibly {a}__DOT__{b}...
     string      m_origModName;  // Original name of the module, ignoring name() changes, for dot lookup
+    AstScope*   m_scope;

Comment all members (multiple places). Should be m_scopep;

diff --git a/src/V3EmitCSyms.cpp b/src/V3EmitCSyms.cpp
+    typedef enum { SCOPE_MODULE, SCOPE_OTHER } ScopeType;
+    struct ScopeData { string m_symName; string m_prettyName; ScopeType m_type;
+        ScopeData(const string& symName, const string& prettyName, const ScopeType type)
+            : m_symName(symName), m_prettyName(prettyName), m_type(type) {}
+        string getVLDefine() {
+            switch (m_type) {
+                case ScopeType::SCOPE_MODULE: return "VL_SCOPE_MODULE";
+                case ScopeType::SCOPE_OTHER: return "VL_SCOPE_OTHER";
+                default: return "UNKNOWN";

Not sure there's an advantage of making ScopeType and these case statements. Maybe just when adding add the appropriate string directly (e.g. "VL_SCOPE_MODULE")?

+    string  scopeSymString(string scpName) {

Remove extra space ater string. Should be const char& argument.

+    void varHierarchyScopes(string scp) {
+        while (scp.length() > 0) {

!scp.empty() (multiple places - check all size() and length() for this)

+            if ((scpit != m_vpiScopeCandidates.end()) &&
+                    (m_scopeNames.find(scp)  m_scopeNames.end())) {

Move && on beginning of next line.

if (m_scopeNames.find(scpSym)  m_scopeNames.end()) {
-                        m_scopeNames.insert(make_pair(scpSym, ScopeNameData(scpSym, scpPretty)));
+                        cout << scpSym << " -- " << scpPretty << endl;

What's the " -- " here? (Guessing you're not done yet.)

+            puts(",VerilatedScope::Type::"+it->second.getVLDefine()+");\n");

Space after comma (except when making typedef types). (multiple places)

General - add newlines to avoid > 100 char lines.

#8 Updated by Stefan Wallentowitz about 2 months ago

Hi,

thanks for your input. I have updated the patch with your input and also test the escaped identifier.

Best, Stefan

#9 Updated by Wilson Snyder about 2 months ago

Thanks, very close.

+++ b/test_regress/t/t_vpi_module.cpp
+    CHECK_RESULT_CSTR(name, "t.mod_a.mod_c__02E")

Is this correct - what do other simulators do with escaped names in vpi module names?

+++ b/include/verilated_imp.h
+    VerilatedMutex      m_hierMutex;
+    VerilatedHierarchyMap m_hierMap VL_GUARDED_BY(m_hierMutex);

Please add comments to all member variables (other places too). Thanks.

+++ b/src/V3AstNodes.h
+    AstScope* scopep() { return m_scopep; }

Should be "scopep() const".

#10 Updated by Stefan Wallentowitz about 2 months ago

Hi,

thanks a lot for pointing it out. I discovered that icarus (and other simulators I assume) actually return the module name and not the full hierarchy name, which obviously makes a lot of sense and allows to unmangle the names and make "." in escaped names properly visible. I have fixed this and wrote the test to work with other simulators too. It decodes the names at runtime configuration now, which seems the lowest impact with low code impact.

Best, Stefan

#11 Updated by Wilson Snyder about 2 months ago

Ok, only remaining problem I think is the new decodeName function by doing a strdup will leak more on each call. One choice is to have m_name a string instead. Another is to remove escapes when Verilator makes the name (precompile), that might be cleanest but not sure if there are other implications.

Also nit, remember to use "const std::string&" on most all function inputs of strings, is much faster (avoids a copy).

#12 Updated by Stefan Wallentowitz about 2 months ago

Hi,

thanks, I was thinking the same, but not sure about the impact. As people probably rely on name() returning the full hierarchy name somewhere, I now added another variable to VerilatorScope, which is the identifier name. This is the actual name of the thing. This is the decoded name then. In my humble opinion the extra overhead of the strings shouldn't be an issue, but that's the only drawback I see. On the pro side it is much cleaner.

Also fixed nits.

I read the patch format is link to the commit on Github now, so here you are: https://github.com/wallento/verilator/commit/4cdc848a33338e27c050c0304f222ffe8f792ac7

Thanks a lot for your patience.

Best, Stefan

#13 Updated by Wilson Snyder about 2 months ago

If you want to post github links the easiest thing to do is make a personal branch (e.g. called bug1469) and point to that, then I can just merge.

Please see attached for I think what are your changes with some trivial style stuff cleaned up.

1. A "make test" turns up many problems.

2. I reviewed all the changes and sorry missed earlier, but you added this:

class AstCellInline : public AstNode {
+    AstScope*   m_scopep;       // The scope that the cell is inlined into

Note when you add a member pointer you also need the below. This makes sure the pointer is never dangling.

virtual const char* broken() const { BROKEN_RTN(m_scopep && !m_scopep->brokeExists()); return NULL; }

Once I do that a lot of tests fail, so something isn't tracked properly with adding this.

#14 Updated by Stefan Wallentowitz about 2 months ago

Thanks, Wilson. Just to confirm: The tests are broken with the dangling pointer check in and not before, right?

#15 Updated by Wilson Snyder about 2 months ago

Before, and worse after. I thought my small changes might have done it, but seemed to get the same with the patch you mentioned. Maybe I missed something though.

#16 Updated by Stefan Wallentowitz about 2 months ago

Wilson Snyder wrote:

Before, and worse after. I thought my small changes might have done it, but seemed to get the same with the patch you mentioned. Maybe I missed something though.

Okay, that's strange, because I ran "make test" just earlier and it passes, I will try the next days on a clean checkout. Thanks for your patience!

#17 Updated by Stefan Wallentowitz about 2 months ago

Hi again,

sorry, I shouldn't develop, test and prepare patches while running ORConf, branches got mixed up. Everything works now, the broken was because I missed to default initialize the member variable which did not result in functional errors, but is of course silly..

You can find the patch with your minor changes applied and successfully tested on a clean checkout here: https://github.com/wallento/verilator/tree/bug1469 It is also a fork from your github repo now, so that you probably can git cherry-pick fdf8b55c9ff0a5f739da8f0342d6efaa46bf54e3 too.

Best, Stefan

#18 Updated by Wilson Snyder about 2 months ago

  • Status changed from Feature to Resolved

Great, looks good now thanks for taking all the feedback.

Pushed to git towards eventual 4.020 release.

#19 Updated by Wilson Snyder about 2 months ago

I suspect you forgot to push bug1469-1.

#20 Updated by Stefan Wallentowitz about 2 months ago

Hi,

sorry, even worse, I deleted the branch and the comment because I realized it could be done more elegantly: I had a differentiation in code between scope and module and found this is less code and cleaner when just overloading the scope object with the module object. The scope object is a bit casually used as vpiScope, but similar to vpiPort before it doesn't actually support the operations. At least for vpiModule it should therefore just return that and not an intermediate scope etc. Sorry, forgot you receive the mail. It is solved nicer now and

You can find the patch in https://github.com/wallento/verilator/tree/bug1469-1, commit 65770ca1010dcff4bce58c6623bb7a2687bfb2d4.

Best, Stefan

#21 Updated by Wilson Snyder about 2 months ago

bug1469-1 pushed.

#22 Updated by Wilson Snyder about 1 month ago

  • Status changed from Resolved to Closed

In 4.020. Thanks for reporting this; if there are additional related problems, please open a new issue.

Also available in: Atom