circt icon indicating copy to clipboard operation
circt copied to clipboard

[FIRRTL] FART: when annotated reset is under a when, always leads to uninitialized wires

Open youngar opened this issue 1 year ago • 0 comments

There is some logic when wiring the annotated reset to target registers, that we try to find a location to create a reset wire which dominates all use-sites. The problem is that there is no situation where this logic kicks in that won't later yield an unitialized signal error later on the pipeline.

FIRRTL version 4.0.0
circuit Foo: %[[
  {"class":"sifive.enterprise.firrtl.FullAsyncResetAnnotation", "target":"~Foo|Foo>n"}
]]
  public module Foo:
    input p : UInt<1>
    input r : AsyncReset
    input c : Clock
    when p:
      node n = r
    reg reg : UInt<8>, c

yields

// -----// IR Dump After InferResets (firrtl-infer-resets) //----- //
firrtl.circuit "Foo" {
  firrtl.module @Foo(in %p: !firrtl.uint<1>, in %r: !firrtl.asyncreset, in %c: !firrtl.clock) attributes {convention = #firrtl<convention scalarized>} {
    %n = firrtl.wire : !firrtl.asyncreset
    firrtl.when %p : !firrtl.uint<1> {
      %n_0 = firrtl.node %r {annotations = [{class = "sifive.enterprise.firrtl.FullAsyncResetAnnotation"}], name = "n"} : !firrtl.asyncreset
      firrtl.matchingconnect %n, %n_0 : !firrtl.asyncreset
    }
    %c0_ui8 = firrtl.constant 0 : !firrtl.const.uint<8>
    %reg = firrtl.regreset %c, %n, %c0_ui8 : !firrtl.clock, !firrtl.asyncreset, !firrtl.const.uint<8>, !firrtl.uint<8>
  }
}

A wire is created in the outmost scope, but only driven inside the WhenOp. During ExpandWhens, we hit the error:

./fart7.fir:10:7: error: sink "n" not fully initialized in "Foo"
      node n = r

As it stands, I don't think there is any way that the responsible logic can kick in and yield a circuit that will make it past ExpandWhens, making it rather useles.

A potential solution might be to invalidate the wire, which we know expand whens will lower to mux(p, n, DontCare) => n, but this is not guaranteed to always be the case, as in the future we hope to treat invalid values as they described in the spec. We could even optimize the DontCare to a time-varying signal.

A better solution might be to add a static connect operation which is not affected by containing when operations (h/t @dtzSiFive ).

This is heavily related to https://github.com/llvm/circt/issues/7264.

youngar avatar Jul 02 '24 21:07 youngar