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

Find solution to translate values to strings for Symbiyosys vcd files #254

Closed
nmigen-issue-migration opened this issue Oct 14, 2019 · 24 comments
Milestone

Comments

@nmigen-issue-migration
Copy link

nmigen-issue-migration commented Oct 14, 2019

Issue by RobertBaruch
Monday Oct 14, 2019 at 14:23 GMT
Originally opened as m-labs/nmigen#254


When dumping the result of covers and counterexamples, Symbiyosys writes .vcd files. However, any signals that were enums or FSM states are no longer strings, but just plain old binary.

In gtkwave, it's possible to select a signal and then Edit > Data Format > Translate Filter File. You can then select a filter file, which is just a map of original value to string value:

00 ZERO
01 FIRST
02 TOOOO

The configuration can also be saved in a .gtkw file. However, this only affects displayed signals. If you delete the signal and display it again, it is not translated. This is horrible.

Proposal:

Better might be to just go into the vcd file and edit the values. Although it would require an extra step, I wouldn't object to something like this:

python3 <toplevel.py> stringify <input_vcd> -o <output_vcd>

This would parse the input vcd file, replace numeric values with string values for enum and fsm states, and output the result.

@nmigen-issue-migration
Copy link
Author

nmigen-issue-migration commented Oct 14, 2019

Comment by whitequark
Monday Oct 14, 2019 at 15:26 GMT


Even if reading and altering VCD files was in scope for nMigen (which I don't think it is), the pyvcd library we are using does not support reading VCD files. I believe that the appropriate solution for this is altering SymbiYosys such that it would support symbolic mappings, one way or another.

@nmigen-issue-migration
Copy link
Author

nmigen-issue-migration commented Oct 15, 2019

Comment by RobertBaruch
Tuesday Oct 15, 2019 at 20:32 GMT


Do the symbolic names carry over from Python to the IL that Symbiyosys uses?

@nmigen-issue-migration
Copy link
Author

nmigen-issue-migration commented Oct 15, 2019

Comment by whitequark
Tuesday Oct 15, 2019 at 22:45 GMT


Right now that only happens for the nmigen.decoding attribute, which is set on switch cases. But it would be quite easy to add a way to serialize a dict of every possible decoder value into an attribute on a signal. The reason it's not done right now is that e.g. a 16-bit signal would result in a very large dict, and a 32-bit signal would result in unparsable RTLIL. Whereas in case of a switch case it always results in reasonably sized RTLIL, which is not much larger than the one before the attribute is added.

A proper solution for this issue is quite challenging. Your suggestion of post-processing VCD files is actually not that bad, but there is the very real problem that we can't easily implement it.

@nmigen-issue-migration
Copy link
Author

nmigen-issue-migration commented Nov 10, 2019

Comment by whitequark
Sunday Nov 10, 2019 at 08:18 GMT


I've looked closer at what SymbiYosys is doing. I believe there is a solution that is fairly elegant, would benefit everyone in FOSS FPGA community, and would solve 90% of the problem here. That is:

  • Yosys should support SystemVerilog enumerations, probably by storing the name-to-value map in an attribute applied to every reg or wire of the enum type.
  • yosys-smtbmc should use this attribute when writing VCD files. (Or better, write FST files, which natively support enums.)
  • nMigen should emit this attribute.

Would you be interested in tackling this? I could provide guidance and/or review of Yosys patches.

@nmigen-issue-migration
Copy link
Author

nmigen-issue-migration commented Nov 16, 2019

Comment by RobertBaruch
Saturday Nov 16, 2019 at 00:37 GMT


I could try. But I wouldn't even start unless I knew such a feature would be accepted into yosys.

@nmigen-issue-migration
Copy link
Author

nmigen-issue-migration commented Nov 16, 2019

Comment by whitequark
Saturday Nov 16, 2019 at 00:40 GMT


I'm confident it would. SystemVerilog features in general are something Yosys is sorely lacking.

@nmigen-issue-migration
Copy link
Author

nmigen-issue-migration commented Nov 16, 2019

Comment by RobertBaruch
Saturday Nov 16, 2019 at 01:53 GMT


It seems like this has been tried several times already: YosysHQ/yosys#248 so I'm hesitant to spend a lot of time on it.

@nmigen-issue-migration
Copy link
Author

nmigen-issue-migration commented Nov 16, 2019

Comment by mithro
Saturday Nov 16, 2019 at 01:55 GMT


@RobertBaruch You could try a Yosys plugin?

FYI -- Google also recently released https://github.com/google/verible which is a SystemVerilog parser, linter and code formatter. It does a pretty good job at https://github.com/SymbiFlow/sv-tests but would still need quite a bit of work to hook into Yosys for synthesis. @hzeller is currently looking at what is needed to make that happen.

@nmigen-issue-migration
Copy link
Author

nmigen-issue-migration commented Nov 16, 2019

Comment by whitequark
Saturday Nov 16, 2019 at 01:59 GMT


@RobertBaruch Looks like we can skip doing anything about SystemVerilog then, and only add an attribute. What do you think?

@nmigen-issue-migration
Copy link
Author

nmigen-issue-migration commented Nov 16, 2019

Comment by RobertBaruch
Saturday Nov 16, 2019 at 02:16 GMT


Perhaps a good starting place would be kernel/rtlil.h. Add a struct to hold the value<->string map (is RTLIL::IdString useful?) for an enum, and make that a top-level entity, or a module-level entity. Add nullable pointers to RTLIL::Wire and then make sure this can be written from and read to text.

@nmigen-issue-migration
Copy link
Author

nmigen-issue-migration commented Nov 16, 2019

Comment by whitequark
Saturday Nov 16, 2019 at 02:22 GMT


I've asked Clifford what would be the best approach here, your suggestion vs an attribute.

@nmigen-issue-migration
Copy link
Author

nmigen-issue-migration commented Nov 18, 2019

Comment by whitequark
Monday Nov 18, 2019 at 11:23 GMT


@RobertBaruch As I expected Clifford thinks this should be done using just attributes.

@nmigen-issue-migration
Copy link
Author

nmigen-issue-migration commented Dec 15, 2019

Comment by whitequark
Sunday Dec 15, 2019 at 11:12 GMT


@RobertBaruch Friendly ping--have you looked into implementing this?

@nmigen-issue-migration
Copy link
Author

nmigen-issue-migration commented Dec 15, 2019

Comment by RobertBaruch
Sunday Dec 15, 2019 at 17:35 GMT


I have some time next week that I can devote to working on this.

@nmigen-issue-migration
Copy link
Author

nmigen-issue-migration commented Dec 15, 2019

Comment by whitequark
Sunday Dec 15, 2019 at 17:41 GMT


Excellent, thanks!

@nmigen-issue-migration
Copy link
Author

nmigen-issue-migration commented Dec 23, 2019

Comment by RobertBaruch
Monday Dec 23, 2019 at 18:32 GMT


I did find where the traces for cover mode are written (backends/smt2/smtio.py), and where the actual value is written (bv2bin), but that value appears to come from another process, No clue where that is, or whether those values are always assumed to be integers. Thoughts?

@nmigen-issue-migration
Copy link
Author

nmigen-issue-migration commented Dec 23, 2019

Comment by RobertBaruch
Monday Dec 23, 2019 at 18:33 GMT


(and really, considering Clifford wrote yosys, wouldn't he be in the best position to just solve this in, like, a few minutes?)

@nmigen-issue-migration
Copy link
Author

nmigen-issue-migration commented Dec 23, 2019

Comment by RobertBaruch
Monday Dec 23, 2019 at 22:27 GMT


I've written up a proposal at YosysHQ/yosys#1594. Hopefully this will start a discussion about the feature, and whether it's even feasible in yosys.

@nmigen-issue-migration
Copy link
Author

nmigen-issue-migration commented Dec 24, 2019

Comment by whitequark
Tuesday Dec 24, 2019 at 03:37 GMT


Thanks.

@whitequark
Copy link
Member

whitequark commented Feb 24, 2020

@RobertBaruch Yosys now has enum support!

@whitequark
Copy link
Member

whitequark commented Apr 12, 2020

This is no longer gated on upstream since YosysHQ/yosys#1642 was merged. It should be fairly straightforward to add support for RTLIL enum representation to nMigen, but I have a question about the syntax (YosysHQ/yosys#1594 (comment)).

@whitequark
Copy link
Member

whitequark commented Apr 14, 2020

Proposed new syntax in YosysHQ/yosys#1918.

@whitequark
Copy link
Member

whitequark commented Apr 15, 2020

The new syntax was accepted.

@whitequark
Copy link
Member

whitequark commented Apr 15, 2020

And nMigen now translates the attributes. E.g.:

import enum
from nmigen import *
from nmigen.back import verilog


class Color(enum.IntEnum):
    RED = 1
    GREEN = 2
    BLUE = 3


color = Signal(Color)
print(verilog.convert(Module(), ports=[color]))
/* Generated by Yosys 0.9+2406 (git sha1 88c622b8f, clang 7.0.1-8 -fPIC -Os) */

(* generator = "nMigen" *)
(* top =  1  *)
(* \nmigen.hierarchy  = "top" *)
module top(color);
  (* enum_base_type = "Color" *)
  (* enum_value_01 = "RED" *)
  (* enum_value_10 = "GREEN" *)
  (* enum_value_11 = "BLUE" *)
  (* src = "bug.py:12" *)
  input [1:0] color;
endmodule

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

No branches or pull requests

2 participants