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
Comments
Original Redmine Comment 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. |
Original Redmine Comment 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. |
Original Redmine Comment 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. |
Original Redmine Comment Oh, I forgot to add: Major reason for this proposal was that nothing outside a VerilatedScope can be accessed anyways, right? |
Original Redmine Comment Thanks for the background. I agree adding the type to VpiScope seems most preferred. |
Original Redmine Comment 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, |
Original Redmine Comment 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 Below are nits.
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.
Can revert this.
Perhaps a typedef to cleanup that long std::map line.
Instead please define VERILATED_VPI_CPP (e.g. this filename) then edit the assertion this
Spaces after commas.
Comment all members (multiple places). Should be m_scopep;
Not sure there's an advantage of making ScopeType and these case
Remove extra space ater string. Should be const char& argument.
!scp.empty() (multiple places - check all size() and length() for this)
Move && on beginning of next line.
What's the " -- " here? (Guessing you're not done yet.)
Space after comma (except when making typedef types). (multiple places) General - add newlines to avoid > 100 char lines. |
Original Redmine Comment Hi, thanks for your input. I have updated the patch with your input and also test the escaped identifier. Best, |
Original Redmine Comment Thanks, very close.
Is this correct - what do other simulators do with escaped names in vpi module names?
Please add comments to all member variables (other places too). Thanks.
Should be "scopep() const". |
Original Redmine Comment 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, |
Original Redmine Comment 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). |
Original Redmine Comment 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, |
Original Redmine Comment 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.
Note when you add a member pointer you also need the below. This makes sure the pointer is never dangling.
Once I do that a lot of tests fail, so something isn't tracked properly with adding this. |
Original Redmine Comment Thanks, Wilson. Just to confirm: The tests are broken with the dangling pointer check in and not before, right? |
Original Redmine Comment 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. |
Original Redmine Comment Wilson Snyder wrote:
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! |
Original Redmine Comment 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/#� Best, |
Original Redmine Comment Great, looks good now thanks for taking all the feedback. Pushed to git towards eventual 4.020 release. |
Original Redmine Comment I suspect you forgot to push #�-1. |
Original Redmine Comment 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, |
Original Redmine Comment #�-1 pushed. |
Original Redmine Comment In 4.020. Thanks for reporting this; if there are additional related problems, please open a new issue. |
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:
What would be left is the question of how to represent hierarchical relations. I think it could be done in two ways:
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
The text was updated successfully, but these errors were encountered: