[FIRRTL] FullResetAnnotation breaks multi-top

[seldridge]

Consider the following circuit:

FIRRTL version 6.0.0
circuit Foo: %[[
  {
    "class": "circt.FullResetAnnotation",
    "target": "~|Foo>reset",
    "resetType": "async"
  }
]]

  module Baz:
    inst bar of Bar

  module Bar:

  public module Foo:
    input reset: AsyncReset

    inst bar of Bar

When compiled with firtool Foo.fir -mlir-print-ir-after-failure, this will error out because this creates a port on Bar, but doesn't update all instances of Bar. See commentary later as I think there are actual problems with FullResetAnnotation here:

circt/async-reset-full-multi-top.fir:11:5: error: 'firrtl.instance' op has a wrong number of results; expected 1 but got 0
    inst bar of Bar
    ^
circt/async-reset-full-multi-top.fir:11:5: note: see current operation: "firrtl.instance"() <{annotations = [], domainInfo = [], layers = [], moduleName = @Bar, name = "bar", nameKind = #firrtl<name_kind droppable_name>, portAnnotations = [], portDirections = array<i1>, portNames = []}> : () -> ()
circt/async-reset-full-multi-top.fir:13:3: note: original module declared here
  module Bar:
  ^

MLIR dump:

"firrtl.circuit"() <{annotations = [], name = "Foo"}> ({
  "firrtl.module"() <{
    annotations = [],
    convention = #firrtl<convention internal>,
    domainInfo = [],
    layers = [],
    portAnnotations = [],
    portDirections = array<i1>,
    portLocations = [],
    portNames = [],
    portSymbols = [],
    portTypes = [],
    sym_name = "Baz"
  }> ({
    "firrtl.instance"() <{
      annotations = [],
      domainInfo = [],
      layers = [],
      moduleName = @Bar,
      name = "bar",
      nameKind = #firrtl<name_kind droppable_name>,
      portAnnotations = [],
      portDirections = array<i1>,
      portNames = []
    }> : () -> ()
  }) {sym_visibility = "private"} : () -> ()
  "firrtl.module"() <{
    annotations = [{class = "circt.FullResetAnnotation"}],
    convention = #firrtl<convention internal>,
    domainInfo = [],
    layers = [],
    portAnnotations = [],
    portDirections = array<i1: false>,
    portLocations = [loc("circt/async-reset-full-multi-top.fir":16:11)],
    portNames = ["reset"],
    portSymbols = [],
    portTypes = [!firrtl.asyncreset],
    sym_name = "Bar"
  }> ({
  ^bb0(%arg1: !firrtl.asyncreset):
  }) {sym_visibility = "private"} : () -> ()
  "firrtl.module"() <{
    annotations = [{class = "circt.FullResetAnnotation"}],
    convention = #firrtl<convention scalarized>,
    domainInfo = [[]],
    layers = [],
    portAnnotations = [[{class = "circt.FullResetAnnotation",
    resetType = "async"}]],
    portDirections = array<i1: false>,
    portLocations = [loc("circt/async-reset-full-multi-top.fir":16:11)],
    portNames = ["reset"],
    portSymbols = [],
    portTypes = [!firrtl.asyncreset],
    sym_name = "Foo"
  }> ({
  ^bb0(%arg0: !firrtl.asyncreset):
    %0 = "firrtl.instance"() <{
      annotations = [],
      domainInfo = [[]],
      layers = [],
      moduleName = @Bar,
      name = "bar",
      nameKind = #firrtl<name_kind droppable_name>,
      portAnnotations = [[]],
      portDirections = array<i1: false>,
      portNames = ["reset"]
    }> : () -> !firrtl.asyncreset
    "firrtl.matchingconnect"(%0, %arg0) : (!firrtl.asyncreset, !firrtl.asyncreset) -> ()
  }) : () -> ()
}) : () -> ()

The fundamental problem is that FullResetAnnotation is a "point-to-point" connection request. It's saying "Hook up this port to all the registers that don't have a reset under me." This has to create ports to do this. However, it assumes that this port can be connected to all of these registers. In the multi-top case, e.g., above where Bar is instantiated under Foo and Bar, it is completely ambiguous as to what should happen here. Specifically, it's unclear how the added port should be driven from an instantiation site that isn't reachable by the requested port.

Pragmatically, there are a couple of things that can help here, but don't address the fundamental problem:

1. Modify the InferResets pass to not create ports for modules which don't need the port. I.e., for modules which have no reset-less registers underneath them (in their body or in any body of a module they instantiate).
2. The InferResets pass shouldn't create invalid IR. This needs to update instances to add the port, even if it doesn't know what to do with it, other than leaving it unconnected. The phase ordering of InferResets before ExpandWhens will catch this. (Relying on the phase ordering is dangerous, though...)
3. The other instantiations can create the port on all instances and then tie it to zero. I don't exactly like this, but it's about the only sane thing to do here. A register with a reset tied to zero is the same as a register without a reset in simulation (and we already do this optimization when we see it).

Note: the same problem does exist when there is a register. I.e., the problem here can't just be ignored with a solution to (1):

FIRRTL version 6.0.0
circuit Foo: %[[
  {
    "class": "circt.FullResetAnnotation",
    "target": "~|Foo>reset",
    "resetType": "async"
  }
]]

  module Baz:
    input clock: Clock

    inst bar of Bar
    connect bar.clock, clock

  module Bar:
    input clock: Clock

    reg r: UInt<1>, clock

  public module Foo:
    input clock: Clock
    input reset: AsyncReset

    inst bar of Bar
    connect bar.clock, clock

[youngar]

Looks like it might be combination of these two old issues:

1. Since Baz is not under the main module hierarchy, it is never processed. I would be curious if instantiating Baz under Foo would give a proper error message. [FIRRTL] FART: Does not process modules not reachable from the main module #7674
2. This issue has a similar problem, but we properly detect that Bar is under multiple domains and emit an error message. The ticket suggests we should be cloning Bar when we add ports to it. [FIRRTL] InferResets Not Duplicating Modules When Necessary #4886