[FIRRTL] SFCCompat: can select different values for a single invalid value

[youngar]

FIRRTL version 4.0.0
circuit Foo: %[[
]]
  public module Foo:
    input c : Clock
    input i0 : UInt<8>
    input i1 : UInt<8>
    input r : UInt<1>
    output o0 : UInt<8>
    output o1 : UInt<8>

    wire w : UInt<8>
    w is invalid

    reg reg0 : UInt<8>, c with:
      reset => (r, w)
    connect reg0, i0
    connect o0, reg0

    reg reg1 : UInt<8>, c with:
      reset => (r, w)
    connect reg1, i1
    connect o1, reg1

gives:

module Foo(
  input        c,
  input  [7:0] i0,
               i1,
  input        r,
  output [7:0] o0,
               o1
);

  reg [7:0] reg0;
  reg [7:0] reg1;
  always @(posedge c) begin
    reg0 <= i0;
    reg1 <= i1;
  end // always @(posedge)
  `ifdef ENABLE_INITIAL_REG_
    `ifdef FIRRTL_BEFORE_INITIAL
      `FIRRTL_BEFORE_INITIAL
    `endif // FIRRTL_BEFORE_INITIAL
    initial begin
      automatic logic [31:0] _RANDOM[0:0];
      `ifdef INIT_RANDOM_PROLOG_
        `INIT_RANDOM_PROLOG_
      `endif // INIT_RANDOM_PROLOG_
      `ifdef RANDOMIZE_REG_INIT
        _RANDOM[/*Zero width*/ 1'b0] = `RANDOM;
        reg0 = _RANDOM[/*Zero width*/ 1'b0][7:0];
        reg1 = _RANDOM[/*Zero width*/ 1'b0][15:8];
      `endif // RANDOMIZE_REG_INIT
    end // initial
    `ifdef FIRRTL_AFTER_INITIAL
      `FIRRTL_AFTER_INITIAL
    `endif // FIRRTL_AFTER_INITIAL
  `endif // ENABLE_INITIAL_REG_
  assign o0 = reg0;
  assign o1 = reg1;
endmodule

The resets for both registers are optimized away by SFCCompat, which implies that it selected w = reg0 when lowering reg0 and w = reg1 when lowering reg1. It should select a single value for the invalid value.

[youngar]

@jackkoenig points out that this behaviour is documented in the FIRRTL dialect rationale:

circt/docs/Dialects/FIRRTL/RationaleFIRRTL.md

Lines 956 to 958 in f75bbd7

	1. An invalid value driving the initialization value of a register (looking
	through wires and connections within module scope) removes the reset from the
	register.

[jackkoenig]

Right, I think we are violating the FIRRTL spec here, but what's missing is a reasonable way in the FIRRTL spec to define aggregate-typed registers that are partially reset. If we add a direct way to do that, we can remove that interpretation of invalid values and stop violating the spec 🙂.

[youngar]

relevant FIRRTLE spec section

[darthscsi]

Yea, "look through connects and find invalids" is just flat broken. I don't see why we are doing that, other than for backwards (no longer a good reason) compatibility. It should be enough to pick a value for w is invalid and then let that propagate to the registers. If it lines up with the assignment, great, if it doesn't, then we can't eliminate it.

[darthscsi]

In general, we need to be actively choosing values for invalids to minimize the circuit. We haven't been doing this for SFCCompat reasons.

[jackkoenig]

In general, we need to be actively choosing values for invalids to minimize the circuit. We haven't been doing this for SFCCompat reasons.

Incidentally that is what's happening here though. The looking across allows us to remove reset from 2 registers rather than resetting them to the same value.

I agree generally, but I maintain that we need the "invalid as no reset value" aspect of this until we have a better (and more direct) way to define partially reset registers.