UCSBarchlab / PyRTL

A collection of classes providing simple hardware specification, simulation, tracing, and testing suitable for teaching and research. Simplicity, usability, clarity, and extensibility are the overarching goals, rather than performance or optimization.
http://ucsbarchlab.github.io/PyRTL
BSD 3-Clause "New" or "Revised" License
253 stars 76 forks source link

Missing SDFF_PP1 circuit model yosys primitive #411

Closed JulianKemmerer closed 2 years ago

JulianKemmerer commented 2 years ago

Hello there

I am using PyRTL with .blif files output from yosys

Circuits look like so in terms of cell usage:

Number of cells:                147
     $_ANDNOT_                      25
     $_AND_                          1
     $_DFF_P_                       69
     $_MUX_                          3
     $_NAND_                         1
     $_NOT_                          1
     $_ORNOT_                       21
     $_OR_                           9
     $_SDFF_PP1_                     1
     $_XNOR_                         7
     $_XOR_                          9

For whatever reason yosys uses a $_SDFF_PP1_ flip flop of some kind.

And from PyRTL I get

  File "/home/julian/.local/lib/python3.8/site-packages/pyrtl/importexport.py", line 397, in extract_model_reference
    subckt = Subcircuit(models[command['model_name']], clk_set=formal_clks, block=block)
KeyError: '$_SDFF_PP1_'

Any suggestions on what to do about this SDFF_PP1?

Thanks!

timsherwood commented 2 years ago

Hi Julian, this is interesting -- Do you have a small example .blif file we could use to test? I am guessing this is a problem with "reset to 1" style dffs. Does the source verilog have a dff with that behavior by chance? PyRTL supports that, but I am not sure we ever revisited the blif converter to take such designs. That is just a guess though, @mdko has run more designs through the .blif path than anyone else.

mdko commented 2 years ago

Here's a simple file that will produce a similar problem (reset to 0, so this produces $_SDFF_PP0_'s):

module counter (clk, rst, en, count);

    input clk, rst, en;
    output reg [3:0] count;

    always @(posedge clk)
        if (rst)
            count <= 4'd0;
        else if (en)
            count <= count + 4'd1;

endmodule

The issue is that the input_from_verilog() function calls out to yosys to produce the BLIF file, which in turn is just imported using input_from_blif(). input_from_blif() was previously the only way to do this, and input_from_verilog() was added as a wrapper to make developers' lives a little easier by supplying the necessary yosys arguments/commands to produce an importable BLIF (though obviously not in this case).

I'm still trying to figure out why this is happening; I'm not sure if it's because of the recent yosys version updates or something else, but I don't remember this failing previously.

Solution 1

We can fix this by changing the yosys script located here to be a little more verbose, to something like this:

yosys_arg_template = (
  "read_verilog -nolatches %s"
  "hierarchy -check %s"
  "proc;"
  "opt_expr; opt_merge; opt_muxtree; opt_reduce; opt_merge; opt_expr; opt_clean;"
  "memory;"
  "opt_expr; opt_merge; opt_muxtree; opt_reduce; opt_merge; opt_expr; opt_clean;"
  "fsm;"
  "opt_expr; opt_merge; opt_muxtree; opt_reduce; opt_merge; opt_expr; opt_clean;"
  "techmap;"
  "opt_expr; opt_merge; opt_muxtree; opt_reduce; opt_merge; opt_expr; opt_clean;"
  "flatten;"
  "opt_expr; opt_merge; opt_muxtree; opt_reduce; opt_merge; opt_expr; opt_clean;"
  "write_blif %s"
)

I don't know exactly why I can't just replace those long lines of "opt_*" commands with a single "opt" command (when I do so, we get back those subcircuits for dffs and have the same problem we're seeing); one or more of the optimization loops by the vanilla "opt" command is doing something funky.

Solution 2

Another solution would be to alter the synchronous dffs we recognize in the BLIF parser (see here); we already do something similar for recognizing certain asynchronous subcircuits. This is less than ideal because there are lot of different cells Yosys can produce (see here), and it might end up being a game of cat-and-mouse always adding more that we find get generated inadvertently. Though maybe we just specifically recognize the synchronous "reset to 0" and "reset to 1" dffs for now.

Solution 1 is probably easier right now; ideally, though, we'd be able to figure out why the vanilla yosys script we're using is still producing those subckt $_SDFF_PP{0,1}'s and then not produce them (I'd like to just be seeing .latch with init vals of 0 or 1, if possible, but maybe handling $_SDFF_PP{0,1} is fine too). What do you think?

timsherwood commented 2 years ago

Thanks Michael! I am fine with either solution. As a third option (or maybe an extension of 1) I am wondering though if we can pass some option to techmap to make sure yosys only generates a subset of the full technology map? I don't know if they are still on there, but it used to be you could get an answer on the yosys subreddit in a couple of minutes :)

mdko commented 2 years ago

I decided to go with Option 2 for now as it might be a little more robust (see #413). Though I think figuring out the Yosys options to generate a subset of the tech map (@timsherwood 's option 3) would be a good complement to this too, for added assurance we're generating stuff our BLIF importer can handle.

JulianKemmerer commented 2 years ago

Thanks for the quick fix yall!