[FIRRTL] Analog wires can go wrong on 'LowerSignatures' pass

[Nergy-TCGeneric]

Issue

From commit 479e9f315e7eb840caa3cbd0617197d23d709c97, this chisel code:

class Inner extends RawModule {
  val auto = IO(new Bundle {
    val in = Flipped(new Bundle {
      val PAD = Analog(1.W)
    })
  })
}

class Outer extends RawModule {
  val inner = Module(new Inner())
}

Gets lowered into following FIRRTL:

FIRRTL version 6.0.0
circuit Outer :
  layer Verification, bind, "verification" :
    layer Assert, bind, "verification/assert" :
    layer Assume, bind, "verification/assume" :
    layer Cover, bind, "verification/cover" :

  module Inner :
    output auto : { flip in : { PAD : Analog<1>}}
    skip

  public module Outer :
    inst inner of Inner

And if passed to firtool, it emits an error:

./temp/TOP-carved.fir:8:3: error: analog types may not be connected
  module Inner : 
  ^
./temp/TOP-carved.fir:8:3: note: see current operation: "firrtl.matchingconnect"(%1, %arg0) : (!firrtl.bundle<PAD: analog<1>>, !firrtl.bundle<PAD: analog<1>>) -> ()
./temp/TOP-carved.fir:13:5: error: analog types may not be connected
    inst inner of Inner
    ^
./temp/TOP-carved.fir:13:5: note: see current operation: "firrtl.matchingconnect"(%2, %0) : (!firrtl.bundle<PAD: analog<1>>, !firrtl.bundle<PAD: analog<1>>) -> ()

With this broken IR (supplied --mlir-ir-print-after-failure option):

// -----// IR Dump After LowerSignatures Failed (firrtl-lower-signatures) //----- //
"firrtl.circuit"() <{annotations = [], name = "Outer"}> ({
  "firrtl.layer"() <{convention = #firrtl<layerconvention bind>, sym_name = "Verification"}> ({
    "firrtl.layer"() <{convention = #firrtl<layerconvention bind>, sym_name = "Assert"}> ({
    ^bb0:
    }) {output_file = #hw.output_file<"verification/assert/">} : () -> ()
    "firrtl.layer"() <{convention = #firrtl<layerconvention bind>, sym_name = "Assume"}> ({
    ^bb0:
    }) {output_file = #hw.output_file<"verification/assume/">} : () -> ()
    "firrtl.layer"() <{convention = #firrtl<layerconvention bind>, sym_name = "Cover"}> ({
    ^bb0:
    }) {output_file = #hw.output_file<"verification/cover/">} : () -> ()
  }) {output_file = #hw.output_file<"verification/">} : () -> ()
  "firrtl.module"() <{annotations = [], convention = #firrtl<convention internal>, domainInfo = [[]], layers = [], portAnnotations = [[]], portDirections = array<i1: false>, portLocations = [loc("./temp/TOP-carved.fir":9:12)], portNames = ["auto_in"], portSymbols = [], portTypes = [!firrtl.bundle<PAD: analog<1>>], sym_name = "Inner"}> ({
  ^bb0(%arg0: !firrtl.bundle<PAD: analog<1>>):
    %3 = "firrtl.wire"() <{annotations = [], name = "auto", nameKind = #firrtl<name_kind interesting_name>}> : () -> !firrtl.bundle<in flip: bundle<PAD: analog<1>>>
    %4 = "firrtl.subfield"(%3) <{fieldIndex = 0 : i32}> : (!firrtl.bundle<in flip: bundle<PAD: analog<1>>>) -> !firrtl.bundle<PAD: analog<1>>
    "firrtl.matchingconnect"(%4, %arg0) : (!firrtl.bundle<PAD: analog<1>>, !firrtl.bundle<PAD: analog<1>>) -> ()
    "firrtl.skip"() : () -> ()
  }) {sym_visibility = "private"} : () -> ()
  "firrtl.module"() <{annotations = [], convention = #firrtl<convention scalarized>, domainInfo = [], layers = [], portAnnotations = [], portDirections = array<i1>, portLocations = [], portNames = [], portSymbols = [], portTypes = [], sym_name = "Outer"}> ({
    %0 = "firrtl.instance"() <{annotations = [], domainInfo = [[]], layers = [], moduleName = @Inner, name = "inner", nameKind = #firrtl<name_kind interesting_name>, portAnnotations = [[]], portDirections = array<i1: false>, portNames = ["auto_in"]}> : () -> !firrtl.bundle<PAD: analog<1>>
    %1 = "firrtl.wire"() <{annotations = [], name = "inner.auto", nameKind = #firrtl<name_kind droppable_name>}> : () -> !firrtl.bundle<in flip: bundle<PAD: analog<1>>>
    %2 = "firrtl.subfield"(%1) <{fieldIndex = 0 : i32}> : (!firrtl.bundle<in flip: bundle<PAD: analog<1>>>) -> !firrtl.bundle<PAD: analog<1>>
    "firrtl.matchingconnect"(%2, %0) : (!firrtl.bundle<PAD: analog<1>>, !firrtl.bundle<PAD: analog<1>>) -> ()
  }) : () -> ()
}) : () -> ()

Reason this issue should be fixed

We're using a diplomacy package for building a SoC environment. LazyModule's AutoBundle aggregates wires into one bundle, like:

output auto : { flip third_pad_in : { flip Y : UInt<1>, OE : UInt<1>, IE : UInt<1>, A : UInt<1>}, third_pad_out : { PAD : Analog<1>} ...

Just having these wires hanging around individually is simply infeasible, because we want to get the most out of parameter negotiation.

Possible workarounds

https://circt.llvm.org/docs/Dialects/FIRRTL/FIRRTLAnnotations/#convention

Use convention.scalarized API.

class Inner extends RawModule {
  val auto = IO(new Bundle {
    val in = Flipped(new Bundle {
      val PAD = Analog(1.W)
    })
  })
}

class Outer extends RawModule {
  val inner = Module(new Inner())
  convention.scalarized(inner)
}

Now yields following FIRRTL (with ChiselStage.emitCHIRRTL):

FIRRTL version 6.0.0
circuit Outer :%[[
  {
    "class":"circt.ConventionAnnotation",
    "target":"~|Inner",
    "convention":"scalarized"
  }
]]
  layer Verification, bind, "verification" :
    layer Assert, bind, "verification/assert" :
    layer Assume, bind, "verification/assume" :
    layer Cover, bind, "verification/cover" :

  module Inner :
    output auto : { flip in : { PAD : Analog<1>}}
    skip

  public module Outer :
    inst inner of Inner

And now firtool happily accepts it:

./temp/lower-sig-test-with-anno.fir:18:10: warning: module `Outer` is empty but cannot be removed because the module is public
  public module Outer :
         ^
module {
  firrtl.circuit "Outer" {
    firrtl.layer @Verification bind attributes {output_file = #hw.output_file<"verification/">} {
      firrtl.layer @Assert bind attributes {output_file = #hw.output_file<"verification/assert/">} {
      }
      firrtl.layer @Assume bind attributes {output_file = #hw.output_file<"verification/assume/">} {
      }
      firrtl.layer @Cover bind attributes {output_file = #hw.output_file<"verification/cover/">} {
      }
    }
    firrtl.module @Outer() attributes {convention = #firrtl<convention scalarized>} {
    }
  }
}

However, one who have no knowledge on these may be surprised by this result, so I suggest resolving this issue so future developers won't get trapped again by the same problem.

EDIT-1) I accidentially omitted Flipped on workaround section. Still works as intended though.

[seldridge]

Yeah, this is a bug in LowerSignatures. It looks like it's creating a matchingconnect when it's not safe and should create a connect. Alternatively, it could generate a more "blown out" representation and use attach here.

We're using a diplomacy package for building a SoC environment. LazyModule's AutoBundle aggregates wires into one bundle, like:

I've been thinking about this a bit and I think that the diplomacy likely needs to change. It's very, very constraining that the result of an auto bundle (or really any edge computation) has to be a Bundle as opposed to being a sequence of types that can be connected (in the general sense, not in the Chisel3 Connectable sense). This is a pretty major change, though. However, I'm considering that it may have to happen as things like the nascent Chisel domains come online. We already ran into this with probe types where we didn't want to support them in bundles, but were force to do so because of diplomacy (which could be, more positively, viewed as making them work cohesively with all Chisel types).

[Nergy-TCGeneric]

Seems that there has been a precedent:

chipsalliance/chisel#4775

The author said he used firtool 1.75.0, which ships LowerSignatures pass. AFAIK this pass was introduced since firtool 1.61.1 so it makes sense.

https://github.com/llvm/circt/releases/tag/firtool-1.61.0