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

Add --public-flat-rw flag #1511

Closed
veripoolbot opened this issue Sep 19, 2019 · 7 comments
Closed

Add --public-flat-rw flag #1511

veripoolbot opened this issue Sep 19, 2019 · 7 comments

Comments

@veripoolbot
Copy link
Contributor

@veripoolbot veripoolbot commented Sep 19, 2019


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

Original Assignee: Stefan Wallentowitz (@wallento)


Hi,

the purpose of these patches is to add a new switch --public-flat-rw that changes the behavior as if /verilator public_flat_rw/ was applied to all vars, ports and wires. This is in particular useful for VPI (cocotb port in our case), where the code base may not be (yet) designed for Verilator.

Along come minor changes to the test infrastructure for adding one further variable and an accessor.

Cheers,
Stefan

@veripoolbot
Copy link
Contributor Author

@veripoolbot veripoolbot commented Sep 19, 2019


Original Redmine Comment
Author Name: Stefan Wallentowitz (@wallento)
Original Date: 2019-09-19T05:16:44Z


This is a compromise between --public (deprecated) and requiring the designer to add the comments. By restricting it to those types only (for vpi var access) it is probably very useful, yet not fully crushing performance. An alternative is to name it differently in the --vpi-* space, but I believe it can be useful beyond that.

@veripoolbot
Copy link
Contributor Author

@veripoolbot veripoolbot commented Sep 19, 2019


Original Redmine Comment
Author Name: Wilson Snyder (@wsnyder)
Original Date: 2019-09-19T22:52:47Z


I really don't like expanding the userfullness use of public. There's two problems:

  1. Users will probably set it then not ever tune more finely (how many designs have +acc+rw forever) and get bad performance forever.

  2. Making some things public such as signals used to generate clocks can cause incorrect behavior.

I do understand however that users might not know what variables to r/w.

Another counter argument maybe that some people are using --public now and this is better from a performance perspective.

At a minimum the docs need to better discourage this.

Is there any other option open?

BTW if you can please squash so you make one patch file it would be helpful. But 0005 seems separate and was applied.

@veripoolbot
Copy link
Contributor Author

@veripoolbot veripoolbot commented Sep 20, 2019


Original Redmine Comment
Author Name: Stefan Wallentowitz (@wallento)
Original Date: 2019-09-20T05:01:33Z


Thanks for your insights, I am a bit torn apart on this one. But I believe overall the usability wins in this case. The flexibility of VPI already comes with a performance loss, and a lot of the power of cocotb power gets lost, I believe, when people need to instrument their code.

Can you please elaborate a bit on the issue with the clocks or point me to an example, to check if I can workaround it?

My vision is that we can have users define public signals via a file with patterns etc. But this is so far down my TODO list, it probably won't see light for a couple of months. Plus, it would not protect users from the same issue (if a user chooses to put "*" into that file, which I would for a start). I have idea for further variations of this on the cocotb side where it iteratively generates the file and recompiles, but that needs much more thoughts and the problems remain.

Another option could be to not expose this feature via a switch but use an environment variable, so that users better acknowledge the issue from documentation by having it technically separate and reconsider. But that's probably just making things worse.

It is indeed a very good improvement over public, to replace many usages of this, we probably should add a --public-modules, too. But on the other hand, as you say, we should discourage it for the general usage.

I have reworked the documentation to be more clear about the issue.

Sorry, I did not squash them before because their have been multiple authors, the basic ideas comes from the Antmicro guys, I added documentation and tests only. Anyhow, after squash Lukasz remains author :)

Still two patches, the VMPREFIX accessor patch is still independent (but only used by this test).

Thanks a lot for your help!

@veripoolbot
Copy link
Contributor Author

@veripoolbot veripoolbot commented Sep 21, 2019


Original Redmine Comment
Author Name: Wilson Snyder (@wsnyder)
Original Date: 2019-09-21T12:51:41Z


Ok, given the trade-offs seems reasonable.

If this starts getting a lot of use we might want an option to at runtime record what really gets accessed, then write out a .vlt control file that makes only those things accessed public_flat_rw.

  1. Pushed VM_PREFIX.

  2. Attached is a proposed revised patch that fixes some trivial spacing/style differences.

  3. As Lukasz is a new author to us, I need to satisfy the lawyers, so need either (him?) to post agreement, or have the patch also add (him?) to docs/CONTRIBUTORS to acknowledge this and future contributions are made under the Developer Certificate of Origin (https://developercertificate.org/). [Or, perhaps your signing off implies that, not sure, new to this stuff.]

@veripoolbot
Copy link
Contributor Author

@veripoolbot veripoolbot commented Sep 21, 2019


Original Redmine Comment
Author Name: Stefan Wallentowitz (@wallento)
Original Date: 2019-09-21T17:41:10Z


Wilson Snyder wrote:

Ok, given the trade-offs seems reasonable.

If this starts getting a lot of use we might want an option to at runtime record what really gets accessed, then write out a .vlt control file that makes only those things accessed public_flat_rw.

Yes, that's exactly along the lines I was thinking. Starting with all public, let a couple of tests run through and then recompile with that file. I will add two issues for this.

  1. Pushed VM_PREFIX.

  2. Attached is a proposed revised patch that fixes some trivial spacing/style differences.

Thanks, lgtm.

  1. As Lukasz is a new author to us, I need to satisfy the lawyers, so need either (him?) to post agreement, or have the patch also add (him?) to docs/CONTRIBUTORS to acknowledge this and future contributions are made under the Developer Certificate of Origin (https://developercertificate.org/). [Or, perhaps your signing off implies that, not sure, new to this stuff.]

I signed off because the documentation and tests are actually my contribution. From context I think the Lukasz uses the sign-off as with Linux, i.e. acknowledging his contribution is under the certificate (as described here: https://github.com/wking/signed-off-by/blob/master/Documentation/SubmittingPatches). I will drop him a mail with you in CC to acknowledge this explicitly, plus I propose we add a CONTRIBUTING file and will open a new issue for that. We recently added one in cocotb: https://github.com/cocotb/cocotb/blob/master/CONTRIBUTING.md. I think this is pretty useful for new contributors to describe the procedure to submit patches.

Thanks a lot for your quick responses!

@veripoolbot
Copy link
Contributor Author

@veripoolbot veripoolbot commented Sep 23, 2019


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


Thanks all.

Pushed to git towards eventual 4.020 release.

@veripoolbot
Copy link
Contributor Author

@veripoolbot veripoolbot commented Oct 6, 2019


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


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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
1 participant
You can’t perform that action at this time.