dfflibmap: enable inference

[Ravenslofty]

What are the reasons/motivation for this change? dfflibmap can't map flops with enables, so yosys has to use muxes. By using flops that have enables, we can save area and possibly delay.

Explain how this is achieved. The next_state attribute of the ff block needs to be evaluated to see if it's an enable expression. This means a bunch of infrastructure related to parsing liberty expressions.

If applicable, please suggest to reviewers how they can test the change. Run dfflibmap -liberty foo.lib on various liberty files you can find, and see if they a) parse without crashing and b) successfully detect the enable flops in the file.

Still to do:

• a bunch of log_warnings are commented out because they are extremely noisy.
  • sky130 has some scan chain flops with the expression (D&DE&!SCE)|(IQ&!DE&!SCE)|(SCD&SCE), which the parser chokes on because of three ands instead of two - to investigate and fix. (to be clear, this won't infer, but it shouldn't issue the nonsense warning of "Inference failed on expression '(D&DE&!SCE)|(IQ&!DE&!SCE)|(SCD&SCE)' in next_state attribute of cell 'sky130_fd_sc_hd__sedfxtp_4' because it does not contain ff output 'IQ' - skipping." when it does. continue in a do/while loop did not do what I thought it did.
  • more generally, because the code loops through all cells to find each cell type, this results in a lot of duplicate warnings. there's now a lookup to avoid duplicate warnings.
• flops with enable and reset - the infrastructure is there, I just don't know of any test cases to check they work, since sky130 just has edfxtp/edfxbp.

[QuantamHD]

It would also be okay to skip scan flops. DFT is implemented in OpenROAD, and will replace flops with their scan equivalents.

[widlarizer]

Should we also extend techlibs/common/cells.lib with DFFE.* to make default synth represent realistic flows better now that we have this capability?

[povik]

Should we also extend techlibs/common/cells.lib with DFFE.* to make default synth represent realistic flows better now that we have this capability?

I don't think cells.lib has anything to do with synth

[widlarizer]

Well, seeing that cells.lib isn't referenced anywhere in the codebase, that certainly raises the question: what does cells.lib have to do with anything? But that's not relevant for this PR specifically.

[KrystalDelusion]

cells.lib provides synthesis models used for simulation (and possibly formal verification?) I'm thinking of simcells.v and simlib.v, though I suspect cells.lib is similar for liberty (albeit with far fewer cells)

[widlarizer]

The build failure should be possible to resolve with a rebase BTW

[Ravenslofty]

rebased, with log_assert in find_cell_sr when enapol is true.

[widlarizer]

Unfortunately, when used in ORFS, turns out this feature regresses area usage on ibex on sky130. When dfflibmap matches a $dffe of the opposite enable polarity, it emits an $_NOT_ and the PDK flop with enable. In ORFS, dfflibmap is ran before abc. When the mux is hidden in the PDK dffe cell, abc can't (in effect) make use of ITE(!a, x, y) = ITE(a, y, x) to eliminate the inverter by swapping the mux inputs, unlike when it emitted a regular flop and multiplexer.

Repro:

module foo(
    input se,
    input clk,
    input xi,
    input [8:0] a,
    input [8:0] b,
    output xo,
);
  // To test with more logic around,
  // uncomment and replace se with mod:

  // wire mod;
  // assign mod = ((a + b) % 7) == 3;

  initial begin
    xo = 0;
  end

  always @(posedge clk)
  begin
    if (se)
        xo <= xi;
  end
endmodule

read_verilog dff_regr.v
proc
opt
techmap
opt
stat
dfflibmap -liberty sky130_fd_sc_hd__tt_025C_1v80.lib
stat
abc -liberty sky130_fd_sc_hd__tt_025C_1v80.lib
stat -liberty sky130_fd_sc_hd__tt_025C_1v80.lib

I think we have work to do in dfflegalize to prevent this (maybe configurably)

[Ravenslofty]

I'm...not sure that's a valid transformation when the enable mux is baked into the flop?

But I can see that it can be an issue if the polarity doesn't exactly match. Hmm.

[widlarizer]

Yeah the idea is we probably should prevent baking the muxes into dffe PDK cells if it creates an inverter outside the PDK cell

[widlarizer]

I'm not so sure about this anymore. The regression doesn't reproduce on commits of this PR branch, because it's based on an older main commit. Hilariously, when qwp was removed, a constid was removed too, which made the ibex synthesize better, but not when DFFEs are inferred. Apparently, the 1% regression we saw is indistinguishable from hash perturbations. Can't wait to have --hash-seed available on main so we can chase fewer ghosts

[povik]

On nangate45:

12. Executing DFFLIBMAP pass (mapping DFF cells to sequential cells from liberty file).
Warning: Inference failed on expression '((SE*SI)+(D*!SE))' in next_state attribute of cell 'SDFFRS_X1' because it does not contain ff output 'IQ' - skipping.
Warning: Inference failed on expression '((SE*SI)+(D*!SE))' in next_state attribute of cell 'SDFFRS_X2' because it does not contain ff output 'IQ' - skipping.
Warning: Inference failed on expression '((SE*SI)+(D*!SE))' in next_state attribute of cell 'SDFFR_X1' because it does not contain ff output 'IQ' - skipping.
Warning: Inference failed on expression '((SE*SI)+(D*!SE))' in next_state attribute of cell 'SDFFR_X2' because it does not contain ff output 'IQ' - skipping.
Warning: Inference failed on expression '((SE*SI)+(D*!SE))' in next_state attribute of cell 'SDFFS_X1' because it does not contain ff output 'IQ' - skipping.
Warning: Inference failed on expression '((SE*SI)+(D*!SE))' in next_state attribute of cell 'SDFFS_X2' because it does not contain ff output 'IQ' - skipping.
Warning: Inference failed on expression '((SE*SI)+(D*!SE))' in next_state attribute of cell 'SDFF_X1' because it does not contain ff output 'IQ' - skipping.
Warning: Inference failed on expression '((SE*SI)+(D*!SE))' in next_state attribute of cell 'SDFF_X2' because it does not contain ff output 'IQ' - skipping.

I think we need to demote those to log_debug