-
Notifications
You must be signed in to change notification settings - Fork 890
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
Parameters as wires #1106
Parameters as wires #1106
Conversation
…rams. Those wires are connected to constant values which represent their default values. No connection is made for string and real parameters Signed-off-by: Maciej Kurc <mkurc@antmicro.com>
…log backend Signed-off-by: Maciej Kurc <mkurc@antmicro.com>
Signed-off-by: Maciej Kurc <mkurc@antmicro.com>
Signed-off-by: Maciej Kurc <mkurc@antmicro.com>
… the wire mocks a parameter of its module. Signed-off-by: Maciej Kurc <mkurc@antmicro.com>
…ROTOBUF, SIMPLEC and SMT2 Signed-off-by: Maciej Kurc <mkurc@antmicro.com>
Signed-off-by: Maciej Kurc <mkurc@antmicro.com>
Signed-off-by: Maciej Kurc <mkurc@antmicro.com>
Signed-off-by: Maciej Kurc <mkurc@antmicro.com>
…nd. Default value is not stored anywhere yet. Signed-off-by: Maciej Kurc <mkurc@antmicro.com>
Signed-off-by: Maciej Kurc <mkurc@antmicro.com>
Signed-off-by: Maciej Kurc <mkurc@antmicro.com>
Signed-off-by: Maciej Kurc <mkurc@antmicro.com>
Signed-off-by: Maciej Kurc <mkurc@antmicro.com>
… reference Signed-off-by: Maciej Kurc <mkurc@antmicro.com>
|
||
bool RTLIL::Wire::is_mockup() const { | ||
return is_parameter() || is_localparam(); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is no need to add any of those three functions or any of the many uses you add for them. I have no idea why you think you need to add hundreds of lines of code all over the place for this. The reason we have suggested the use of an attribute is so that this can be accomplished with a small non-intrusive change.
Signed-off-by: Clifford Wolf <clifford@clifford.at>
See #1109, which adds <50 lines instead of >1100. |
Calculations;
Excluding tests, this PR adds 195 lines of code. |
Excluding the infrastructure for making this an optional feature, which your patch is lacking, and excluding handling of the case of attributes on Verilog-2001-style parameters, which your patch is lacking as well, and excluding the added documentation for the new parameters, which is also missing in your patch, my patch adds about 20 lines. |
@cliffordwolf I appreciate that you were able to implement almost the same functionality within ~20 lines of code. But your patch targets only the Verilog frontend. What about backends? With your patch those wires that correspond to parameters will appear as wires and there is no way to distinguish them from actual wires of a module. The change I proposed also affects backends such as Verilog, Ilang an JSON so module parameters appear in their output as parameters. That's why the code is longer. If you want the feature of having those wires to be optional it can be easily added as well as support for attributes on Verilog 2001 style parameters. |
No. That's why we have the attribute. That's the whole point for it.
We proposed the attribute solution specifically because we did not want to have intrusive changes all over the place for an obscure feature that besides you nobody is using. And we've been very clear about that context. I you don't like it I can also remove the feature altogether again. |
This reverts commit ec45650.
This reverts commit ec45650.
This PR allows to store attributes of a Verilog module parameter in a wire object. For each parameter a mocking wire is created with the same name. Those mocking wires are connected to constants which correspond to default values of parameters. Parameters with their default values are outputted via Ilang, Verilog and JSON backends.
The PR also adds a small test suite which compares various backend outputs with a golden reference.