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

Add --public-flat-rw flag

Added by Stefan Wallentowitz 3 months ago. Updated 2 months ago.

Status:
Closed
Priority:
Normal
Category:
Invoking/Options
% Done:

0%


Description

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

0001-vpi-Add-public-flat-rw-switch.patch View (3.55 KB) Stefan Wallentowitz, 09/19/2019 05:04 AM

0002-Make-public-flat-rw-not-VPI-specific.patch View (1.96 KB) Stefan Wallentowitz, 09/19/2019 05:04 AM

0003-Add-documentation-for-public-flat-rw.patch View (2.02 KB) Stefan Wallentowitz, 09/19/2019 05:04 AM

0005-tests-Overwrite-PLI-filename.patch View (1.46 KB) Stefan Wallentowitz, 09/19/2019 05:04 AM

0006-Test-public-flat-rw.patch View (4.87 KB) Stefan Wallentowitz, 09/19/2019 05:04 AM

0004-tests-Set-VM_PREFIX-from-test-script.patch View (814 Bytes) Stefan Wallentowitz, 09/19/2019 05:04 AM

public_flat_rw_switch.patch View (9.61 KB) Stefan Wallentowitz, 09/20/2019 05:01 AM

tests_vmprefix_accessor.patch View (702 Bytes) Stefan Wallentowitz, 09/20/2019 05:01 AM

public_flat_rw_switch2.patch View (9.92 KB) Wilson Snyder, 09/21/2019 12:49 PM

public_flat_rw_switch2.patch View (9.92 KB) Wilson Snyder, 09/21/2019 12:51 PM

History

#1 Updated by Stefan Wallentowitz 3 months ago

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.

#2 Updated by Wilson Snyder 3 months ago

  • Status changed from New to Assigned
  • Assignee set to Stefan Wallentowitz

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.

#3 Updated by Stefan Wallentowitz 3 months ago

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!

#4 Updated by Wilson Snyder 3 months ago

#5 Updated by Wilson Snyder 3 months ago

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

#6 Updated by Stefan Wallentowitz 3 months ago

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.

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

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!

#7 Updated by Wilson Snyder 3 months ago

  • Status changed from Assigned to Resolved

Thanks all.

Pushed to git towards eventual 4.020 release.

#8 Updated by Wilson Snyder 2 months 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