[FIRRTL] Add LowerGroups Pass

[seldridge]

Add the LowerGroups pass that converts FIRRTL optional groups to bound
modules.

This is a re-opening of #5536 as I pushed to main and screwed up the PR from GitHub's perspective.

[dtzSiFive]

Not sure if can be confident strictconnect is safe re:uninferred widths (and passive?).
(and similarly for StrictConnect created above?)

EDIT: Oops, looks like we already restrict captures to be passive (?), so probably nevermind that bit 👍 .

[seldridge]

Yeah, this is super restricted in what it is allowed to be. This will cause problems as we start to open up this language feature.

[dtzSiFive]

Maybe use the instance location (would suggest the port locations if those were tracked for instances)?

[seldridge]

I audited these and should have everything correct. With some updates to missing emitter locations for modules and ports, this can now be shown.

Input FIRRTL:

FIRRTL version 3.1.0
circuit Foo: %[[
  {"class":"firrtl.transforms.DontTouchAnnotation", "target":"~Foo|Foo>A_a"},
  {"class":"firrtl.transforms.DontTouchAnnotation", "target":"~Foo|Foo>B_a"}
]]
  declgroup A, bind:
    declgroup B, bind:
    declgroup C, bind:

  module Foo:
    input a: UInt<1> @[a.scala 1:1]
    input b: UInt<1> @[b.scala 2:2]

    group A:          @[group_A.scala 4:4]
      node A_a = a    @[node_A_a 5:5]
      group B:        @[group_A_B.scala 6:6]
        node B_a = a  @[node_B_a 7:7]

FIRRTL after this pass (manually stripping sv.verbatim using firtool groups/Foo.fir -ir-fir -mlir-print-debuginfo | sed 's/^ *sv.verbatim.*//' | circt-translate -export-firrtl:

FIRRTL version 3.1.0
circuit Foo :
  module Foo_A_B : @[group_A_B.scala 6:6]
    input _a : UInt<1> @[node_B_a 7:7]

    node B_a = _a @[node_B_a 7:7]

  module Foo_A : @[group_A.scala 4:4]
    input _a : UInt<1> @[node_A_a 5:5]
    output _foo_A_B__a : Probe<UInt<1>> @[node_B_a 7:7]

    define _foo_A_B__a = probe(_a) @[node_B_a 7:7]
    node A_a = _a @[node_A_a 5:5]

  module Foo : @[groups/Foo.fir 10:10]
    input a : UInt<1> @[a.scala 1:1]
    input b : UInt<1> @[b.scala 2:2]

    inst foo_A_B of Foo_A_B @[group_A_B.scala 6:6]
    inst foo_A of Foo_A @[group_A.scala 4:4]
    connect foo_A_B._a, read(foo_A._foo_A_B__a) @[node_B_a 7:7]
    connect foo_A._a, a @[node_A_a 5:5]

Does this look correct to you?

[seldridge]

My idea here is to have all the ports, connects, and ref ops that are created take the source locator from the original capture. I.e., all of these should be either node_A_a or node_B_a.

[dtzSiFive]

Awesome, thank you for this care and for chasing this down! 🚀

[darthscsi]

:(

[seldridge]

I know...

I couldn't think of a way to avoid this without implementing all the sv.file stuff we talked about.

It may be better to NOT add the ifndef/endif for now. It's then user error if the wrong or incompatible things are included. WDYT?

[seldridge]

I will plan to revisit this in later commits.