[FIRRTL][InferResets] Disallow external modules on InstanceChoice?

[uenoku]

When an instance choice was used for normal module and external module,
InferReset with FullAsyncResetTransform adds a reset port to only normal ports.
I don't consider this is a bug of InferResets since this is fundamentally not fixable, and I think for predicability we should simply ban external modules on InstanceChoice and let users to create a wrapper module for that.

FIRRTL version 5.1.0
circuit test :%[[
{ "class":"circt.FullResetAnnotation",
  "target":"~test|test>reset",
  "resetType":"async" }]]
  option Platform:
    FPGA

  extmodule Foo:
    input clock : Clock
    input a: UInt<1>
    output b: UInt<1>

  module Bar:
    input clock : Clock
    input a: UInt<1>
    output b: UInt<1>
    reg r: UInt<1>, clock
    connect r, a
    connect b, r

  public module test :
    input clock : Clock
    input reset : AsyncReset
    input in : UInt<1>
    output out : UInt<1>
    instchoice proc of Bar, Platform:
      FPGA => Foo
    connect proc.a, in
    connect proc.clock, clock
    connect out, proc.b

bar.fir:30:5: error: 'firrtl.instance_choice' op has a wrong number of results; expected 3 but got 4
    instchoice proc of Bar, Platform:
    ^
bar.fir:30:5: note: see current operation: %0:4 = "firrtl.instance_choice"() <{annotations = [], caseNames = [@Platform::@FPGA], domainInfo = [[], [], [], []], layers = [], moduleNames = [@Bar, @Foo], name = "proc", nameKind = #firrtl<name_kind droppable_name>, portAnnotations = [[], [], [], []], portDirections = array<i1: false, false, false, true>, portNames = ["reset", "clock", "a", "b"]}> : () -> (!firrtl.asyncreset, !firrtl.clock, !firrtl.uint<1>, !firrtl.uint<1>)
bar.fir:12:3: note: original module declared here
  extmodule Foo:
  ^
// -----// IR Dump After InferResets Failed (firrtl-infer-resets) //----- //
"firrtl.circuit"() <{annotations = [], name = "test"}> ({
  "firrtl.option"() <{sym_name = "Platform"}> ({
    "firrtl.option_case"() <{sym_name = "FPGA"}> : () -> ()
  }) : () -> ()
  "firrtl.extmodule"() <{annotations = [], convention = #firrtl<convention scalarized>, domainInfo = [[], [], []], externalRequirements = [], knownLayers = [], layers = [], parameters = [], portAnnotations = [[], [], []], portDirections = array<i1: false, false, true>, portLocations = [loc("bar.fir":13:11), loc("bar.fir":14:11), loc("bar.fir":15:12)], portNames = ["clock", "a", "b"], portSymbols = [], portTypes = [!firrtl.clock, !firrtl.uint<1>, !firrtl.uint<1>], sym_name = "Foo"}> ({
  }) {sym_visibility = "private"} : () -> ()
  "firrtl.module"() <{annotations = [{class = "circt.FullResetAnnotation"}], convention = #firrtl<convention internal>, domainInfo = [], layers = [], portAnnotations = [], portDirections = array<i1: false, false, false, true>, portLocations = [loc("bar.fir":27:11), loc("bar.fir":18:11), loc("bar.fir":19:11), loc("bar.fir":20:12)], portNames = ["reset", "clock", "a", "b"], portSymbols = [], portTypes = [!firrtl.asyncreset, !firrtl.clock, !firrtl.uint<1>, !firrtl.uint<1>], sym_name = "Bar"}> ({
  ^bb0(%arg4: !firrtl.asyncreset, %arg5: !firrtl.clock, %arg6: !firrtl.uint<1>, %arg7: !firrtl.uint<1>):
    %1 = "firrtl.constant"() <{value = 0 : ui1}> : () -> !firrtl.const.uint<1>
    %2 = "firrtl.regreset"(%arg5, %arg4, %1) <{annotations = [], name = "r", nameKind = #firrtl<name_kind droppable_name>}> : (!firrtl.clock, !firrtl.asyncreset, !firrtl.const.uint<1>) -> !firrtl.uint<1>
    "firrtl.matchingconnect"(%2, %arg6) : (!firrtl.uint<1>, !firrtl.uint<1>) -> ()
    "firrtl.matchingconnect"(%arg7, %2) : (!firrtl.uint<1>, !firrtl.uint<1>) -> ()
  }) {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, false, false, true>, portLocations = [loc("bar.fir":26:11), loc("bar.fir":27:11), loc("bar.fir":28:11), loc("bar.fir":29:12)], portNames = ["clock", "reset", "in", "out"], portSymbols = [], portTypes = [!firrtl.clock, !firrtl.asyncreset, !firrtl.uint<1>, !firrtl.uint<1>], sym_name = "test"}> ({
  ^bb0(%arg0: !firrtl.clock, %arg1: !firrtl.asyncreset, %arg2: !firrtl.uint<1>, %arg3: !firrtl.uint<1>):
    %0:4 = "firrtl.instance_choice"() <{annotations = [], caseNames = [@Platform::@FPGA], domainInfo = [[], [], [], []], layers = [], moduleNames = [@Bar, @Foo], name = "proc", nameKind = #firrtl<name_kind droppable_name>, portAnnotations = [[], [], [], []], portDirections = array<i1: false, false, false, true>, portNames = ["reset", "clock", "a", "b"]}> : () -> (!firrtl.asyncreset, !firrtl.clock, !firrtl.uint<1>, !firrtl.uint<1>)
    "firrtl.matchingconnect"(%0#0, %arg1) : (!firrtl.asyncreset, !firrtl.asyncreset) -> ()
    "firrtl.matchingconnect"(%0#2, %arg2) : (!firrtl.uint<1>, !firrtl.uint<1>) -> ()
    "firrtl.matchingconnect"(%0#1, %arg0) : (!firrtl.clock, !firrtl.clock) -> ()
    "firrtl.matchingconnect"(%arg3, %0#3) : (!firrtl.uint<1>, !firrtl.uint<1>) -> ()
  }) : () -> ()
}) : () -> ()

uenoku@pop-os ~/d/circt (dev/hidetou/rewrite-db) [1]> 

[seldridge]

I regrettably think that this may be a reasonable restriction to apply (and to enshrine in the FIRRTL specification) while we figure out how to drop port punching passes, which arguably should be deleted and moved to the front end language / metaprogramming on the front end language.

We can then eventually roll this out for external modules once we get a handle on this.

Is Full Reset ABI-breaking? I.e., does it apply to public modules?