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

VPI module #1469

Closed
veripoolbot opened this issue Jun 20, 2019 · 22 comments
Closed

VPI module #1469

veripoolbot opened this issue Jun 20, 2019 · 22 comments
Labels
resolution: fixed Closed; fixed type: feature-IEEE Request to add new feature, described in IEEE 1800

Comments

@veripoolbot
Copy link
Contributor


Author Name: Stefan Wallentowitz (@wallento)
Original Redmine Issue: 1469 from https://www.veripool.org

Original Assignee: Stefan Wallentowitz (@wallento)


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

@veripoolbot
Copy link
Contributor Author


Original Redmine Comment
Author Name: Todd Strader (@toddstrader)
Original Date: 2019-06-20T12:58:28Z


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.

@veripoolbot
Copy link
Contributor Author


Original Redmine Comment
Author Name: Wilson Snyder (@wsnyder)
Original Date: 2019-06-20T13:26:52Z


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.

@veripoolbot
Copy link
Contributor Author


Original Redmine Comment
Author Name: Stefan Wallentowitz (@wallento)
Original Date: 2019-06-20T13:34:58Z


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.

@veripoolbot
Copy link
Contributor Author


Original Redmine Comment
Author Name: Stefan Wallentowitz (@wallento)
Original Date: 2019-06-20T13:38:23Z


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

@veripoolbot
Copy link
Contributor Author


Original Redmine Comment
Author Name: Wilson Snyder (@wsnyder)
Original Date: 2019-06-20T13:48:59Z


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

@veripoolbot
Copy link
Contributor Author


Original Redmine Comment
Author Name: Stefan Wallentowitz (@wallento)
Original Date: 2019-09-23T21:55:20Z


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

@veripoolbot
Copy link
Contributor Author


Original Redmine Comment
Author Name: Wilson Snyder (@wsnyder)
Original Date: 2019-09-23T23:27:52Z


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<const VerilatedScope*, std::vector<const VerilatedScope*> > {

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.

@veripoolbot
Copy link
Contributor Author


Original Redmine Comment
Author Name: Stefan Wallentowitz (@wallento)
Original Date: 2019-09-27T16:45:05Z


Hi,

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

Best,
Stefan

@veripoolbot
Copy link
Contributor Author


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


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

@veripoolbot
Copy link
Contributor Author


Original Redmine Comment
Author Name: Stefan Wallentowitz (@wallento)
Original Date: 2019-09-28T15:18:58Z


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

@veripoolbot
Copy link
Contributor Author


Original Redmine Comment
Author Name: Wilson Snyder (@wsnyder)
Original Date: 2019-09-28T16:11:37Z


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

@veripoolbot
Copy link
Contributor Author


Original Redmine Comment
Author Name: Stefan Wallentowitz (@wallento)
Original Date: 2019-09-29T15:11:19Z


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: wallento@4cdc848

Thanks a lot for your patience.

Best,
Stefan

@veripoolbot
Copy link
Contributor Author


Original Redmine Comment
Author Name: Wilson Snyder (@wsnyder)
Original Date: 2019-09-29T18:01:18Z


If you want to post github links the easiest thing to do is make a personal branch (e.g. called #�) 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.

@veripoolbot
Copy link
Contributor Author


Original Redmine Comment
Author Name: Stefan Wallentowitz (@wallento)
Original Date: 2019-09-29T18:25:42Z


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

@veripoolbot
Copy link
Contributor Author


Original Redmine Comment
Author Name: Wilson Snyder (@wsnyder)
Original Date: 2019-09-29T18:44:14Z


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.

@veripoolbot
Copy link
Contributor Author


Original Redmine Comment
Author Name: Stefan Wallentowitz (@wallento)
Original Date: 2019-09-29T18:49:43Z


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!

@veripoolbot
Copy link
Contributor Author


Original Redmine Comment
Author Name: Stefan Wallentowitz (@wallento)
Original Date: 2019-10-01T05:16:30Z


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/#�
It is also a fork from your github repo now, so that you probably can @git cherry-pick fdf8b55@ too.

Best,
Stefan

@veripoolbot
Copy link
Contributor Author


Original Redmine Comment
Author Name: Wilson Snyder (@wsnyder)
Original Date: 2019-10-02T01:58:47Z


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

Pushed to git towards eventual 4.020 release.

@veripoolbot
Copy link
Contributor Author


Original Redmine Comment
Author Name: Wilson Snyder (@wsnyder)
Original Date: 2019-10-02T11:16:11Z


I suspect you forgot to push #�-1.

@veripoolbot
Copy link
Contributor Author


Original Redmine Comment
Author Name: Stefan Wallentowitz (@wallento)
Original Date: 2019-10-02T13:59:01Z


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/#�-1, commit 65770ca.

Best,
Stefan

@veripoolbot
Copy link
Contributor Author


Original Redmine Comment
Author Name: Wilson Snyder (@wsnyder)
Original Date: 2019-10-02T22:48:42Z


#�-1 pushed.

@veripoolbot
Copy link
Contributor Author


Original Redmine Comment
Author Name: Wilson Snyder (@wsnyder)
Original Date: 2019-10-06T14:08:43Z


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

@veripoolbot veripoolbot added resolution: fixed Closed; fixed type: feature-IEEE Request to add new feature, described in IEEE 1800 labels Dec 22, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
resolution: fixed Closed; fixed type: feature-IEEE Request to add new feature, described in IEEE 1800
Projects
None yet
Development

No branches or pull requests

1 participant