[FIRRTL] FRT: treats different encodings of register resets differently

[youngar]

Both registers have a reset, although the first uses the regreset representation, and the latter uses a regular reg driven by a mux.

FIRRTL version 4.0.0
circuit Foo: %[[
  {
   "class": "circt.FullResetAnnotation",
   "target": "~Foo|Foo>r1",
   "resetType": "sync"
  }
]]
  public module Foo:
    input c : Clock
    input i : UInt<8>
    input r1 : UInt<1>
    input r2 : UInt<1>
    output o0 : UInt<8>
    output o1 : UInt<8>

    reg reg0 : UInt<8>, c with:
      reset => (r2, UInt<8>(2))
    connect reg0, i
    connect o0, reg0

    reg reg1 : UInt<8>, c
    connect reg1, mux(r2, UInt<8>(3), i)
    connect o1, reg1

gives:

module Foo(
  input        c,
  input  [7:0] i,
  input        r1,
               r2,
  output [7:0] o0,
               o1
);

  reg [7:0] reg0;
  reg [7:0] reg1;
  always @(posedge c) begin
    if (r2)
      reg0 <= 8'h2;
    else
      reg0 <= i;
    if (r1)
      reg1 <= 8'h0;
    else
      reg1 <= r2 ? 8'h3 : i;
  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 second register ends up with a needless double-reset added by FRT, when it already had a reset.

[youngar]

One of the reasons why I want to drill in to this issue, is because its hard for the FullResetTransform to determine if a register already has a reset or not (when determining if it should add one), and one valid option is for it to always add a reset unconditionally. We can then let regular optimizations handle the Invalid value. So:

wire reset_value : UInt<8>
reset_value is invalid
reg r : UInt<8>, c, next, reset1, reset_value

After FullResetTransform:

wire reset_value : UInt<8>
reset_value is invalid
reg r : UInt<8>, c, mux(reset1, reset_value, next), reset2, UInt<8>(0)

and then, through regular evaluation:

reg r : UInt<8>, c, mux(reset1, invalid, next), reset2, UInt<8>(0)

reg r : UInt<8>, c, next, reset2, UInt<8>(0)

[jackkoenig]

one valid option is for it to always add a reset unconditionally.

That's not valid because if the reset signal is the same as the one used by the register to specify a different (i.e. non-zero) reset value, it will straight up override the user-specified reset.

And I think it's actually worse than that. Because sync reset is just a mux, the reset used for FSRT could be further up the chain of muxes feeding the input to the register. I wouldn't call those muxes "reset", but if it's the same signal, then FSRT has now inserted a coverage hole. Consider the following:

FIRRTL version 4.0.0
circuit Foo: %[[
  {
   "class": "circt.FullResetAnnotation",
   "target": "~Foo|Foo>extra_reset",
   "resetType": "sync"
  }
]]
  public module Foo:
    input c : Clock
    input in0 : UInt<8>
    input in1 : UInt<8>
    input reset : UInt<1>
    input extra_reset : UInt<1>
    output o : UInt<8>
    reg reg : UInt<8>, c
    connect reg, mux(reset, in0, mux(extra_reset, in1, reg))
    connect o, reg

This gives:

// Generated by CIRCT firtool-1.87.0
module Foo(
  input        c,
  input  [7:0] in0,
               in1,
  input        reset,
               extra_reset,
  output [7:0] o
);

  reg [7:0] reg_0;
  always @(posedge c) begin
    if (extra_reset)
      reg_0 <= 8'h0;
    else if (reset)
      reg_0 <= in0;
    else if (extra_reset)
      reg_0 <= in1;
  end // always @(posedge)
  assign o = reg_0;
endmodule

The mux branch assigning in1 is unreachable.

[youngar]

Yeah, I sorta chalk this one up to "you get what you asked for" :)

And I am not sure if this proves anything interesting, but you can get the same problems with async reset as well:

FIRRTL version 4.0.0
circuit Foo: %[[
  {
   "class": "sifive.enterprise.firrtl.FullAsyncResetAnnotation",
   "target": "~Foo|Foo>extra_reset"
  }
]]
  public module Foo:
    input c : Clock
    input in0 : UInt<8>
    input in1 : UInt<8>
    input reset : UInt<1>
    output o : UInt<8>
    node extra_reset = asAsyncReset(reset)
    reg reg : UInt<8>, c
    connect reg, mux(reset, in0, in1)
    connect o, reg

module Foo(
  input        c,
  input  [7:0] in0,
               in1,
  input        reset,
  output [7:0] o
);

  reg [7:0] reg_0;
  always @(posedge c or posedge reset) begin
    if (reset)
      reg_0 <= 8'h0;
    else
      reg_0 <= reset ? in0 : in1;
  end // always @(posedge, posedge)
  `ifdef ENABLE_INITIAL_REG_
    `ifdef FIRRTL_BEFORE_INITIAL
      `FIRRTL_BEFORE_INITIAL
    `endif // FIRRTL_BEFORE_INITIAL
    initial begin
      if (reset)
        reg_0 = 8'h0;
    end // initial
    `ifdef FIRRTL_AFTER_INITIAL
      `FIRRTL_AFTER_INITIAL
    `endif // FIRRTL_AFTER_INITIAL
  `endif // ENABLE_INITIAL_REG_
  assign o = reg_0;
endmodule