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

Switch for file to read public signals from #1514

Closed
veripoolbot opened this issue Sep 21, 2019 · 4 comments
Closed

Switch for file to read public signals from #1514

veripoolbot opened this issue Sep 21, 2019 · 4 comments
Assignees
Labels
area: invoking/options Issue involves options passed to Verilator area: usability Issue involves general usability effort: hours Expect this issue to require roughly hours of invested effort to resolve type: feature-non-IEEE Request to add new feature, outside IEEE 1800

Comments

@veripoolbot
Copy link
Contributor


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

Original Assignee: Stefan Wallentowitz (@wallento)


We should have a switch that lets the user give a file with names of signals (including whitespaces) that are treated as public-flat-rw (or even beyond that allow to set flat/rw/clk). For example this could be such a file:

*/inst_0/A
*/inst_1/*
toplevel_inst/** // recursive

Just a quick braindump, this is just supposed to kick off the discussion.

@veripoolbot
Copy link
Contributor Author


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


Agreed, and suggest this should be handled by adding a config file parse rule to the bottom of verilog.y.

Ideally we'd allow every Verilator metacomment attribute instead come from config files, not just the limited subset (lint/coverage/tracing) now in configs. Perhaps some refactoring of the parser would allow sharing the same rules for the metacomments and config parsing, rather than their being done differently (parser versus lexer) as is now.

@veripoolbot
Copy link
Contributor Author


Original Redmine Comment
Author Name: Stefan Wallentowitz (@wallento)
Original Date: 2019-12-13T15:55:02Z


I have started basic prototyping of this feature here: https://github.com/wallento/verilator/tree/vlt-attrs

I believe the separation is good as it is as the comments directly dump into the Ast. Similar to the coverage/lint comments my prototype hooks up in the early stages and gets called for every AstVar (not every FileLine) to check if it has a configured public attribute, then adds it similarly to what the parser did.

My original idea was to have identifiers of the elaborated design in the attribute directives, but then found that this would be a huge impact, so it is now based on (module,varname) tuples, which is probably more natural and waaay easiert to implement. Needs more polishing and testing of course, but getting there.

Missing is public modules and then wildcards are probably useful and rather simple to add. Will look into other attributes as well.

@veripoolbot
Copy link
Contributor Author


Original Redmine Comment
Author Name: Wilson Snyder (@wsnyder)
Original Date: 2019-12-14T15:11:57Z


Good work.

 +++ b/test_regress/t/t_vlt_public.vlt
 +public "A" "b"
 +public_flat "C" "c"

I don't think this is easily human readable. Maybe something like:?

 public --module "A" --variable "b"
 public_flat --module "C" --variable "c"


 +class V3ConfigPublic {
 ...
 +    void applyPublicVariable(AstVar* varp, AstNodeModule* modp) {
 +        if ((m_varPublics.find(modp->name()) != m_varPublics.end())
 +            && (m_varPublics[modp->name()].find(varp->name()) != m_varPublics[modp->name()].end())) {
 +            if (varp->attrsp()) {
 +                cout << "Already have an attribute" << endl;

I'm sure you intend to add something here - please have a FIXME in the code for incomplete stuff so can't get forgotten.

 +++ b/src/verilog.l
 @@ -141,10 +141,16 @@ vnum    {vnum1}|{vnum2}|{vnum3}|{vnum4}|{vnum5}
    "tracing_off"         { FL; return yVLT_TRACING_OFF; }
    "tracing_on"          { FL; return yVLT_TRACING_ON; }
 +  "public"              { FL; return yVLT_PUBLIC; }
 +  "public_flat"         { FL; return yVLT_PUBLIC_FLAT; }
 +  "public_flat_rd"      { FL; return yVLT_PUBLIC_FLAT_RD; }
 +  "public_flat_rw"      { FL; return yVLT_PUBLIC_FLAT_RW; }
 +  "public_module"       { FL; return yVLT_PUBLIC_MODULE; }

    -?"-file"             { FL; return yVLT_D_FILE; }
    -?"-lines"            { FL; return yVLT_D_LINES; }
    -?"-msg"              { FL; return yVLT_D_MSG; }
 +  -?"-module"           { FL; return yVLT_D_MODULE; }

Please insert these in sorted order (within the section between newlines).

 +++ b/src/verilog.y
 @@ -279,10 +279,16 @@ class AstSenTree;
  %token<fl>         yVLT_TRACING_ON     "tracing_on"
 +%token<fl>         yVLT_PUBLIC                 "public"

Likewise sorted in section.

 +++ b/test_regress/t/t_vlt_public.v

I think this test needs to somehow make sure the public attributes got applied appropriately. One way would be to have a CPP file that checks the public applied. Perhaps a cleaner test would be to change expose the public attributes in the XML file (V3EmitXml) then just compare the XML against a golden file.

@veripoolbot veripoolbot added area: invoking/options Issue involves options passed to Verilator area: usability Issue involves general usability effort: hours Expect this issue to require roughly hours of invested effort to resolve type: feature-non-IEEE Request to add new feature, outside IEEE 1800 labels Dec 22, 2019
@wallento wallento self-assigned this Dec 22, 2019
wallento added a commit that referenced this issue Jan 12, 2020
* Add more directives to configuration files

Allow to set the same directives in configuration files that can also
be set by comment attributes (such as /* verilator public */ etc).

* Add support for lint messsage waivers

Add configuration file switch '-match' for lint_off. It takes a string
with wildcards allowed and warnings will be matched against it (if
rule and file also match). If it matches, the warning is waived.

Fixes #1649 and #1514 
Closes #2072
@wsnyder
Copy link
Member

wsnyder commented Mar 2, 2020

I think @wallento's #2072 basically solved this, as a .vlt file can now have a list of public vars.

@wsnyder wsnyder closed this as completed Mar 2, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: invoking/options Issue involves options passed to Verilator area: usability Issue involves general usability effort: hours Expect this issue to require roughly hours of invested effort to resolve type: feature-non-IEEE Request to add new feature, outside IEEE 1800
Projects
None yet
Development

No branches or pull requests

3 participants