[FIRRTL] FART: not totally idempotent - but may be close enough

[youngar]

The FullAsynchronousResetTransform no longer removes its annotation, which can lead to problems where the pass is no longer idempotent. It seems like some work has gone in to minimizing the amount of work the pass does on subsequent runs, but there is at least one small issue, and possibly others. given: /bin/circt-opt --pass-pipeline="builtin.module(firrtl.circuit(firrtl-infer-resets))"

firrtl.circuit "Foo" {
  firrtl.module @Foo(in %p: !firrtl.uint<1>, in %r: !firrtl.asyncreset [{class = "sifive.enterprise.firrtl.FullAsyncResetAnnotation"}]) attributes {convention = #firrtl<convention scalarized>} {
    %bar_clock = firrtl.instance bar @Bar(in clock: !firrtl.clock)
  }
  firrtl.module private @Bar(in %clock: !firrtl.clock) {
    %r = firrtl.reg %clock : !firrtl.clock, !firrtl.uint<8>
  }
}

the first run yields:

  firrtl.circuit "Foo" {
    firrtl.module @Foo(in %p: !firrtl.uint<1>, in %r: !firrtl.asyncreset [{class = "sifive.enterprise.firrtl.FullAsyncResetAnnotation"}]) attributes {convention = #firrtl<convention scalarized>} {
      %bar_r, %bar_clock = firrtl.instance bar @Bar(in r: !firrtl.asyncreset, in clock: !firrtl.clock)
      firrtl.matchingconnect %bar_r, %r : !firrtl.asyncreset
    }
    firrtl.module private @Bar(in %r: !firrtl.asyncreset, in %clock: !firrtl.clock) {
      %c0_ui8 = firrtl.constant 0 : !firrtl.const.uint<8>
      %r_0 = firrtl.regreset %clock, %r, %c0_ui8 {name = "r"} : !firrtl.clock, !firrtl.asyncreset, !firrtl.const.uint<8>, !firrtl.uint<8>
    }
  }

the second run yields:

   firrtl.circuit "Foo" {
    firrtl.module @Foo(in %p: !firrtl.uint<1>, in %r: !firrtl.asyncreset [{class = "sifive.enterprise.firrtl.FullAsyncResetAnnotation"}]) attributes {convention = #firrtl<convention scalarized>} {
      %bar_r, %bar_clock = firrtl.instance bar @Bar(in r: !firrtl.asyncreset, in clock: !firrtl.clock)
      firrtl.matchingconnect %bar_r, %r : !firrtl.asyncreset
      firrtl.matchingconnect %bar_r, %r : !firrtl.asyncreset
    }
    firrtl.module private @Bar(in %r: !firrtl.asyncreset, in %clock: !firrtl.clock) {
      %c0_ui8 = firrtl.constant 0 : !firrtl.const.uint<8>
      %r_0 = firrtl.regreset %clock, %r, %c0_ui8 {name = "r"} : !firrtl.clock, !firrtl.asyncreset, !firrtl.const.uint<8>, !firrtl.uint<8>
    }
  }

Each run adds a new connect to the instance op. Although functionally equivalent, it would be best if the pass did nothing at all on the second run. We should audit other parts of this code to make sure that there aren't larger issues related to this. Not removing the annotation is required as SFCCompat needs to be aware of its existence.

[youngar]

Another example where it keeps drilling new reset ports:

firrtl.circuit "Foo" {
  firrtl.module @Foo(in %p: !firrtl.uint<1>, in %r: !firrtl.asyncreset [{class = "sifive.enterprise.firrtl.FullAsyncResetAnnotation"}], in %c: !firrtl.clock) attributes {convention = #firrtl<convention scalarized>} {
    %bar_c, %bar_r = firrtl.instance bar @Bar(in c: !firrtl.clock, in r: !firrtl.uint<1>)
    %c0_ui1 = firrtl.constant 0 : !firrtl.const.uint<1>
    %0 = firrtl.constCast %c0_ui1 : (!firrtl.const.uint<1>) -> !firrtl.uint<1>
    firrtl.matchingconnect %bar_r, %0 : !firrtl.uint<1>
    firrtl.matchingconnect %bar_c, %c : !firrtl.clock
  }
  firrtl.module private @Bar(in %c: !firrtl.clock, in %r: !firrtl.uint<1>) {
    %reg = firrtl.reg %c : !firrtl.clock, !firrtl.uint<8>
  }
}

Running this pass multiple times yeilds: ./bin/circt-opt --pass-pipeline="builtin.module(firrtl.circuit(firrtl-infer-resets))" ./fart6.mlir | ./bin/circt-opt --pass-pipeline="builtin.module(firrtl.circuit(firrtl-infer-resets))" | ./bin/circt-opt --pass-pipeline="builtin.module(firrtl.circuit(firrtl-infer-resets))"

module {
  firrtl.circuit "Foo" {
    firrtl.module @Foo(in %p: !firrtl.uint<1>, in %r: !firrtl.asyncreset [{class = "sifive.enterprise.firrtl.FullAsyncResetAnnotation"}], in %c: !firrtl.clock) attributes {convention = #firrtl<convention scalarized>} {
      %bar_r_2, %bar_r_1, %bar_r_0, %bar_c, %bar_r = firrtl.instance bar @Bar(in r_2: !firrtl.asyncreset, in r_1: !firrtl.asyncreset, in r_0: !firrtl.asyncreset, in c: !firrtl.clock, in r: !firrtl.uint<1>)
      firrtl.matchingconnect %bar_r_2, %r : !firrtl.asyncreset
      firrtl.matchingconnect %bar_r_1, %r : !firrtl.asyncreset
      firrtl.matchingconnect %bar_r_0, %r : !firrtl.asyncreset
      %c0_ui1 = firrtl.constant 0 : !firrtl.const.uint<1>
      %0 = firrtl.constCast %c0_ui1 : (!firrtl.const.uint<1>) -> !firrtl.uint<1>
      firrtl.matchingconnect %bar_r, %0 : !firrtl.uint<1>
      firrtl.matchingconnect %bar_c, %c : !firrtl.clock
    }
    firrtl.module private @Bar(in %r_2: !firrtl.asyncreset, in %r_1: !firrtl.asyncreset, in %r_0: !firrtl.asyncreset, in %c: !firrtl.clock, in %r: !firrtl.uint<1>) {
      %c0_ui8 = firrtl.constant 0 : !firrtl.const.uint<8>
      %reg = firrtl.regreset %c, %r_0, %c0_ui8 : !firrtl.clock, !firrtl.asyncreset, !firrtl.const.uint<8>, !firrtl.uint<8>
    }
  }
}

We end up with r_2, r_1, r_0 reset ports, instead of re-using them.