SymbiFlow / yosys

SymbiFlow WIP changes for Yosys Open SYnthesis Suite
http://www.clifford.at/yosys/
ISC License
37 stars 9 forks source link

Support for attributes on parameters in Verilog #25

Closed mkurc-ant closed 5 years ago

mkurc-ant commented 5 years ago

This PR adds support for attributes on parameters for Verilog.

The Verilog frontend was augmented with parsing and storing attributes of parameters in their AST nodes. Parameter attributes are only stored for those that actually have them. The default value of a parameter is also preserved.

mkurc-ant commented 5 years ago

@mithro I didn't do anything to chparam and setparam because the actual parameter values are stored somehow differently. I'think there is no need to change that.

mkurc-ant commented 5 years ago

Now the RTLIL::Module class holds:

pool<RTLIL::IdString> avail_parameters;
dict<RTLIL::IdString, RTLIL::ParameterInfo> parameter_information;
dict<RTLIL::IdString, dict<RTLIL::IdString,RTLIL::Const>> parameter_attributes;

The functionality regarding avail_parameters is unchanged. The structure parameter_information is meant to hold "metadata" of a parameter (default value, width etc.) Currently only the default value is stored.

The dict parameter_attributes contains attributes of parameters but only for those which actually have them.

I also updated the JSON, ilang and Verilog backends. Attributes of parameters are now visible when using the dump command.

mkurc-ant commented 5 years ago

@litghost I already added some tests regarding parsing of parameter attributes and they are merged upstream. I suppose I should add additional tests which will include eg. reading Verilog, writing JSON/ILANG and comparing output with a reference. I haven't found any similar tests in the Yosys tests folder. What do you think ?

Regarding the "cost": I believe that the cost can only be observable when processing verilogs with huge number of attributes on parameters. I would need to think of some kind of memory usage and performance tests. I haven't found such tests either.

mkurc-ant commented 5 years ago

@litghost Do not merge. I recently discovered that floating point constants are handled differently than strings and integers. Also a floating point constant is a different object in the AST tree. I added a quick workaround which treats floating point constants as integer 0 but it is not an acceptable solution.

In order to fix that I would need either to refactor the RTLIL::Const class so it can store a float or the newly added RTLIL::ParameterInfo class which holds default value of a parameter. I think the latter will be less memory consuming.

Anyway, there is still some work to do with this PR.

litghost commented 5 years ago

@litghost In order to fix that I would need either to refactor the RTLIL::Const clas

Maybe we should not modify Yosys and simply extract the FASM_PARAM annotation using a coarse parser. I'm not a fan of keeping a Yosys fork forever, and the more that has to change, the less likely they will accept it. @mithro, thoughts?

mkurc-ant commented 5 years ago

Anyway I managed to do the fix. So the default value of a parameter can now be floating point. It is correctly loaded by the Verilog frontend and correctly outputted by the Verilog and the JSON backend.

The ilang frontend never parsed default values of parameters so I left its functionality intact.

mithro commented 5 years ago

@litghost Let's land this and then we can figure out an alternative approach.

mithro commented 5 years ago

The attribute is specified as a prefix to a declaration, a module item, a statement or a port connection. Three examples are:

The attribute is specified as a suffix to an operator or the function name in a function call.

mithro commented 5 years ago
ordered_port_connection ::= (From A.4.1)
{ attribute_instance } [ expression ]
named_port_connection ::=
{ attribute_instance } . port_identifier ( [ expression ] )
mithro commented 5 years ago

@mkurc-ant So, it appears you missed being able to add an attribute to a port connection. See the third item above. Could you fix that?

eddiehung commented 5 years ago

I've discussed this issue with Yosys upstream and we agree this is a good idea, however, the way we'd suggest implementing it would be to transform the parameter into a new wire (permanently) driven by the parameter value. That way, it appears post-elaboration, and in the backend too, so that it can be inspected during simulation or used during formal verification too.

Would you consider trying this approach instead?