[FIRRTL] InferResets FullReset (formerly FART) does not add resets to not reset fields of partially reset registers

[jackkoenig]

The original iteration of this bug was fixed in https://github.com/llvm/circt/pull/6912, but it appears that fix only works for registers in modules contain the wire or port marked with a circt.FullResetAnnotation, but does not work for children of the module with the annotated port or wire.

Consider the following FIRRTL:

FIRRTL version 3.3.0
; CHECK-LABEL: module test(
circuit top :%[[{
    "class":"circt.FullResetAnnotation",
    "target":"~top|top>reset",
    "resetType":"async"
  }]]
  module test :
    input clock : Clock
    input reset : AsyncReset
    input in : { foo : UInt<8>, bar : UInt<8>}
    output out : { foo : UInt<8>, bar : UInt<8>}

    wire reg1_w : { foo : UInt<8>, bar : UInt<8>}
    invalidate reg1_w
    ; CHECK: reg1_foo <= 8'hC;
    ; CHECK: reg1_bar <= 8'h0;
    connect reg1_w.foo, UInt<8>(0hc)
    invalidate reg1_w.bar
    ; CHECK: reg1_foo = 8'hC;
    ; CHECK: reg1_bar = 8'h0;
    regreset reg1 : { foo : UInt<8>, bar : UInt<8>}, clock, reset, reg1_w
    wire reg2 : { foo : UInt<8>, bar : UInt<8>}
    connect reg1, in
    connect reg2, reg1
    connect out, reg2

  module top :
    input clock : Clock
    input reset : AsyncReset
    input in : { foo : UInt<8>, bar : UInt<8>}
    output out : { foo : UInt<8>, bar : UInt<8>}

    inst child of test
    connect child.clock, clock
    connect child.reset, reset
    connect child.in, in
    connect out, child.out

Note that this is exactly the same as the first test in https://github.com/llvm/circt/blob/f75bbd7986d4d5ab254ce428492f0e2366c16055/test/firtool/async-reset.fir#L5, except that there is a new top module wrapping the module with registers, and the annotated port is in the top module.

Currently, reg1.bar does not properly get the expected async reset to 0, but it should. If you change the annotation to target ~top|test>reset, you will see the correct output.

[youngar]

Smaller testcase that narrows down the issue:

FIRRTL version 4.0.0
circuit top :%[[{
    "class":"circt.FullResetAnnotation",
    "target":"~top|top>reset",
    "resetType":"async"
  }]]
  module test :
    input clock : Clock
    input reset : AsyncReset
    input in : UInt<8>
    output out : UInt<8>

    wire resetvalue : UInt<8>
    invalidate resetvalue

    regreset reg1 : UInt<8>, clock, reset, resetvalue

    connect reg1, in
    connect out, reg1

  public module top :
    input clock : Clock
    input reset : AsyncReset
    input in : UInt<8>
    output out : UInt<8>

    inst child of test
    connect child.clock, clock
    connect child.reset, reset
    connect child.in, in
    connect out, child.out

In the resulting verilog reg1 does not have a reset, and it should have been given one by the FRT:

   module test(
  input        clock,
  input  [7:0] in,
  output [7:0] out
);

  reg [7:0] reg1;
  always @(posedge clock)
    reg1 <= in;
  `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;
        reg1 = _RANDOM[/*Zero width*/ 1'b0][7:0];
      `endif // RANDOMIZE_REG_INIT
    end // initial
    `ifdef FIRRTL_AFTER_INITIAL
      `FIRRTL_AFTER_INITIAL
    `endif // FIRRTL_AFTER_INITIAL
  `endif // ENABLE_INITIAL_REG_
  assign out = reg1;
endmodule

module top(
  input        clock,
               reset,
  input  [7:0] in,
  output [7:0] out
);

  test child (
    .clock (clock),
    .in    (in),
    .out   (out)
  );
endmodule

As Jack points out, in https://github.com/llvm/circt/issues/7677, a reset-value of invalid value is used to indicate that this part of the register has no reset. While against spec, this is important legacy behaviour that is needed to encode partially reset aggregate registers, where only a few of the elements have a proper reset.

The way this is supposed to work is, FRT is not supposed to add a reset if a register already has one, but it isn't smart enough to understand that an invalid value corresponds to a register without a reset. As a result, it will not add a reset to this register. Later on, in SFC compat, when choosing how to lower the Invalid value, it should "see" that the registers should all have a reset, by looking for the FRT annotation, and choose to lower the reset value to 0 or remove the reset accordingly. The "bug" is that the FRT annotation on a parent module won't be discovered on the child module.

The logic for handling invalid value this way, is supposed to be limited to the module scope, can see through wires, nodes and connects, and should not be trying to look through other identities such as:

wire w : UInt<8>
w is invalid
node reset = and(w, w)

We are currently a little bit safe with invalid values, because SFCCompat lowers all remaining Invalid values to 0, but if we relax this restriction and perform more aggressive invalid value optimizations, I think it will make it "impossible" for InferResets to understand if a register has a reset or not, as it has to accurately predict what our optimizations will do later. We do currently have problems such as in https://github.com/llvm/circt/issues/7679.