Project

General

Profile

[logo] 
 
Home
News
Activity
About/Contact
Major Tools
  Dinotrace
  Verilator
  Verilog-mode
  Verilog-Perl
Other Tools
  BugVise
  CovVise
  Force-Gate-Sim
  Gspice
  IPC::Locker
  Rsvn
  SVN::S4
  Voneline
  WFH
General Info
  Papers

Issue #1045

Set parameters as plusarg at compile time

Added by Stefan Wallentowitz over 1 year ago. Updated about 1 year ago.

Status:
Closed
Priority:
Low
Category:
Unsupported
% Done:

0%


Description

Hi Wilson,

I am planning to add the capability to set toplevel parameters on the command line. For that I have been playing around a bit: https://github.com/wallento/verilator/tree/parameters

Here is what I did:

  • Add a parameter command line option: https://github.com/wallento/verilator/commit/ba694010495fb94317f10fda7daa1fdb1e95fb74#diff-00709bc2a581a43e1dae69aa32ec12edR621
  • Store the name=value pairs into a map: https://github.com/wallento/verilator/commit/ba694010495fb94317f10fda7daa1fdb1e95fb74#diff-00709bc2a581a43e1dae69aa32ec12edR133
  • During Linkdot FindVisitor detect if a parameter declaration is in the toplevel: https://github.com/wallento/verilator/commit/ba694010495fb94317f10fda7daa1fdb1e95fb74#diff-2679e807dce49f07773ee6e413f40432R877

Now I am trying to replace the existing value (must be AstConst, right?) with a new one, but cannot use valuep(AstNode *) as it already assigned (here: https://github.com/wallento/verilator/blob/master/src/V3Ast.cpp#L329).

Can you tell me if I am entirely off the track or give me a hint how to get one from there?

Thanks!

Best, Stefan

Overwrite-toplevel-parameteres-at-compilation.patch View (11.9 KB) Stefan Wallentowitz, 03/16/2016 02:37 PM

0001-Make-parseDouble-static-and-add-parameter.patch View (2.32 KB) Stefan Wallentowitz, 03/22/2016 09:22 AM

0002-Overwrite-toplevel-parameteres-at-compilation.patch View (16.2 KB) Stefan Wallentowitz, 03/22/2016 09:22 AM

History

#1 Updated by Stefan Wallentowitz over 1 year ago

Hi,

I managed to change the AstNode properly (?): https://github.com/wallento/verilator/commit/acaae5a32b7f5b8d083caf870abe0fe25fa244ff#diff-2679e807dce49f07773ee6e413f40432R880

It now supports setting a parameter with a plusarg at compile time: +parameter+name=value

What I want to add now is support for non-decimals and for hierarchical names. I am not sure about the latter, probably I have to move to another Visitor then.

Best, Stefan

#2 Updated by Wilson Snyder over 1 year ago

Great, when you have all that you want please make a patch with everything in it.

You create a float like this:

new AstConst(fileline,AstConst::RealDouble(),string)

I'm OK with adding a command line parameter for the top level; that makes sense since you may be building a model to instantiate.

However I don't understand why you would hierarchically override parameters; does any simulator allow that? Seems very dangerous and confusing.

#3 Updated by Wilson Snyder over 1 year ago

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

#4 Updated by Stefan Wallentowitz over 1 year ago

Yes, I agree now after putting some more thoughts into it that hierachical names are a bad idea. I will keep it simple for the moment and also restrict the parameter value to be of basic data types. This feature is probably most powerful for simple automated parameter swapping.

Cheers, Stefan

#5 Updated by Stefan Wallentowitz over 1 year ago

Hi Wilson,

attached please find my proposal for the patch. There are some things I am not entirely sure about:

  • I needed to extend the switch expansion in the Perl wrapper to allow for Verilog notation (32'h0) on the command line. I assume there is better Perl code to do this.
  • Added a test after the LinkDot phase to check if all parameters were actually used. I am not sure if calling v3fatal is the proper reaction.

Looking forward to your comments!

Cheers, Stefan

#6 Updated by Wilson Snyder over 1 year ago

 +     -G<name>=<value>           Overwrite toplevel parameter
 +     +parameter+<name>=<value>  Overwrite toplevel parameter
 +=item +parameter+I<name>=I<value>
 +=item +parameter+I<name>=I<value>+I<name2>=I<value2>...

I looked at three simulators. NCVerilog doesn't support this. VCS supports:

    -pvalue+<parameter_hierarchical_name>=<value>

Modelsim supports:

    -g<Name>=<Value>
    -G<Name>=<Value>

So I would remove the I<name2>... support, and add an alias to -pvalue. Unless some other simulator parameter I would remove that.

+           if (svalue[0] == '"') {
+               // This is a string
+               string v = svalue.substr(1, svalue.find('"', 1) - 1);
+               V3Number n(V3Number::VerilogStringLiteral(),nodep->fileline(),v);
+               value = new AstConst(nodep->fileline(),n);
+           } else if ((svalue.find(".") != string::npos) ||
+                   (svalue.find("e") != string::npos)) {
+               double v = V3ParseImp::parseDouble(svalue.c_str(), svalue.length());
+               value = new AstConst(nodep->fileline(),AstConst::RealDouble(),v);
+           } else {
+               int v = strtol(svalue.c_str(), NULL, 0);
+               if (v != 0) {
+                   V3Number n(fl, 32, v);
+                   value = new AstConst(nodep->fileline(),n);
+                   UINFO(4,value<<endl);
+               } else {
+                   V3Number n(fl, svalue.c_str());
+                   value = new AstConst(nodep->fileline(),n);
+               }
+           }

Please move this block a parseParamLiteral or something function that returns a AstConst*. Also I think it should check the strtol return pointer for errors and bail with an error accordingly.

+           else if ( !strncmp (sw, "+parameter+", 11)) {
+                addParameter (string (sw+strlen("+parameter+")), true);
+           }

Nit, use !strncmp (sw, "+parameter+", strlen("+parameter+")). I know I didn't do that elsewhere but should have.

Please add a t/t_flag_parameter test for each flavor of switch (e.g. can be just one test using -pvalue -g and -G on three different parameters.)

#7 Updated by Stefan Wallentowitz over 1 year ago

Hi Wilson,

thanks for your input. I have updated the patch accordingly and the regression test tests all cases. As probably expected by you writing the regression test revealed a few problems, so that I extended it a bit. As it does not parse the literal to determine its type, it is pretty much trial-and-error now, with falling back to parsing it as Verilog number. The last errors out then.

Attached please find it split into the patch to the lexer's parseDouble and the parameters patch.

Cheers, Stefan

#8 Updated by Stefan Wallentowitz over 1 year ago

Oh, and one remaining problem is that it cannot use double quotes strings in parameter files (.vc). I think there are three options:

  • Live with it
  • Remove the error thrown when a double quote is found in the vc file
  • Test the vc file mroe detailed, which essentially can mean parsing it

How would you like it solved?

#9 Updated by Wilson Snyder over 1 year ago

Patch looks good. Yes, please fix .vc files either way that works, as they are quite commonly used and expected to work.

#10 Updated by Wilson Snyder over 1 year ago

Pushed your patch 0001 to trunk, thanks.

#11 Updated by Stefan Wallentowitz over 1 year ago

In V3Options.cpp:1082 it says:

// Split into argument list and process
// Note we don't respect quotes.  It seems most simulators dont.
// Woez those that expect it; we'll at least complain.
if (whole_file.find("\"") != string::npos) {
fl->v3error("Double quotes in -f files cause unspecified behavior.");
}

I think we can stick with this and the patch is fine then.

Best, Stefan

#12 Updated by Wilson Snyder over 1 year ago

  • Status changed from Assigned to Resolved

Good stuff. Pushed to git towards 3.883.

#13 Updated by Wilson Snyder about 1 year ago

  • Status changed from Resolved to Closed

In 3.884.

Also available in: Atom