opt_dff: skip convertion to dffe for keep wires

[phsauter]

Assuming a module with a FF input marked as keep like this:

module test (
    input  wire clk_i,
    input  wire data_i,
    input  wire select_i,
    output wire out_o
);
    (* keep *) wire ff_d;
    reg ff_q;

    assign ff_d = select_i ? data_i : ff_q;
    always @(posedge clk_i) ff_q <= ff_d;

    assign out_o = ff_q;
endmodule

Currently opt_dff will convert this unconditionally into a enable flop but because the signal is marked as keep no actual optimization is possible and the preceding MUX must be kept.
This results in something like this:

module test(clk_i, data_i, select_i, out_o);
  input clk_i;
  wire clk_i;
  input data_i;
  wire data_i;
  (* keep = 32'd1 *)
  (* unused_bits = "0" *)
  wire ff_d;
  wire ff_q;
  output out_o;
  wire out_o;
  input select_i;
  wire select_i;
  \$_MUX_  mux  (
    .A(ff_q),
    .B(data_i),
    .S(select_i),
    .Y(ff_d)
  );
  \$_DFFE_PP_  dffe  (
    .C(clk_i),
    .D(data_i),
    .E(select_i),
    .Q(ff_q)
  );
  assign out_o = ff_q;
endmodule

Note that ff_d is kept but is not functional anymore, instead the control signal directly controls the enable of the FF.

In my opinion, this violates what keep is meant to do (even though technically it is 'kept') as it changes the signals functions.
Sometimes it is important to keep these kind of signals exactly as-is, for example if constraints need to be applied at a later stage.

While searching for places to perform optimizations in opt_dff, it checks if the signal is marked as keep and skips it (does not add it to bit2mux which is then used to check for the specific optimizations).
This should disable this specific dff->dffe conversion but also all others that may violate the keep (this could lose some optimization potential but since keep signals should be sparse, the penalty is likely extremely small).

Make sure your change comes with tests. If not possible, share how a reviewer might evaluate it.

[widlarizer]

We might actually have a lot of problems of this sort. A lot of manual juggling of these rules in lots of places. For flops, specifically, we could instead tackle keep centrally in kernel/ff.h, we get consistency in all FfData-based rewrites, and potentially even extra consistency in src attribute transfer. The logic would be:

• create new cells as specified
• move all user wires of the old cells to use the new cell instead
• if old cells have no users and aren't keep, remove them

This is like regular garbage collection / dead code elimination / opt_clean but on a limited window of the design. It may need a topo sort and a loop since removing one will make another one unused. If the old cell set can be cyclical, that can still be dealt with. Changing the FfData interface may be tricky but not too tricky. Let me know what you think, I might work on this myself but no promises

[phsauter]

I think ultimately, and especially with regard to SDC constraint support, the 'proper' way to do this would be to have a special cell that functions as an optimization guard where no optimization may go through it (kinda like a module port today). This would always make sure some specific wire, instance or whatever is kept exactly as-is.
I guess this could even be implemented fairly cheap by literally modelling it as a single-input single-output module that is functionally just a passthrough but intentionally doesn't expose this to the rest of the code.

Edit: To elaborate a bit, this special stopper cell is functionally a passhrough and essentially inherits the reason why it was placed (eg as attribute on it) with some additional annotation if necessary (eg for 'keep' it could be annotated with the RTL name of the signal to make sure this isn't lost either).

[whitequark]

This is like regular garbage collection / dead code elimination / opt_clean but on a limited window of the design.

We do something similar in prjunnamed.

[widlarizer]

@phsauter we have #4763 for that, I'm out of touch with how it gets things done and out of capacity to move it forward at the moment... not in the near future, but adding barriers like that seems inevitable