BoringUtils + when/layer/region produces invalid FIRRTL

[dtzSiFive]

Type of issue: Bug Report

Please provide the steps to reproduce the problem:

Consider these tests: https://github.com/dtzSiFive/chisel3/commit/d8a3f1213c32a9e69fd6e15eaf4c9030ce2235fb .

Steps to reproduce are to checkout that branch / apply that commit and run the tests.

What is the current behavior?

The tests fail due to generating invalid FIRRTL because BoringUtils appends connect statements to the end where not all declarations are visible.

Example FIRRTL for the layer test example:

FIRRTL version 4.0.0
circuit Top :
  layer TestLayer, bind : @[src/test/scala/chiselTests/BoringUtilsTapSpec.scala 167:36]
  module Bar : @[src/test/scala/chiselTests/BoringUtilsTapSpec.scala 168:11]
    output out : UInt<1> @[src/test/scala/chiselTests/BoringUtilsTapSpec.scala 169:19]
    input out_bore : UInt<1> @[src/test/scala/chiselTests/BoringUtilsTapSpec.scala 170:36]

    connect out, out_bore @[src/test/scala/chiselTests/BoringUtilsTapSpec.scala 170:11]

  module Foo : @[src/test/scala/chiselTests/BoringUtilsTapSpec.scala 172:11]
    input out_bore : UInt<1> @[src/test/scala/chiselTests/BoringUtilsTapSpec.scala 170:36]

    layerblock TestLayer : @[src/test/scala/chiselTests/BoringUtilsTapSpec.scala 173:30]
      inst bar of Bar @[src/test/scala/chiselTests/BoringUtilsTapSpec.scala 174:25]

    connect bar.out_bore, out_bore @[src/test/scala/chiselTests/BoringUtilsTapSpec.scala 170:36]

  public module Top : @[src/test/scala/chiselTests/BoringUtilsTapSpec.scala 177:11]

    wire parentWire : UInt<1> @[src/test/scala/chiselTests/BoringUtilsTapSpec.scala 178:28]
    inst foo of Foo @[src/test/scala/chiselTests/BoringUtilsTapSpec.scala 179:23]
    connect foo.out_bore, parentWire @[src/test/scala/chiselTests/BoringUtilsTapSpec.scala 170:36]

connect bar.out_bore, out_bore is invalid, bar is only visible within the layer.

What is the expected behavior?

Boring produces diagnostic instead of illegal FIRRTL (as reasonable), and for probes this should work.

Other Information

Encountered this migrating to boring probes, specifically involving a layer.

What is the use case for changing the behavior?

[dtzSiFive]

cc https://github.com/chipsalliance/chisel/issues/3523 .

FWIW FIRRTL also doesn't make it possible to wire some things together at least not without basically "lowering" when's on-the-fly to get something we can wire. Example:

when cond:
  inst f of Foo
else:
  inst b of Bar

Wiring from something within "Foo" to something within "Bar" can't easily be done as we lack a mechanism to send a signal out from under a when (unconditionally, not only "active" when the condition is true) such that it could be routed down into the instance of Bar.

This is somewhat separate from the issue I reported above, that's more about BoringUtils and Chisel IR not being well-suited to putting the new connect where they need to go to wire what we /can/ simply represent with our current language/IR/operations.

For a bit of completeness, we could actually -- for passive and at least hardware-y (not probes or properties) -- with a bit of tricker accomplish this -- we probe a value, use define to route it out from under the when, and read the value so we can plumb it down again:

wire bounce : Probe<UInt<5>>
when cond:
  inst f of Foo
  define bounce = probe(f.bored_signal)
else:
  inst b of Bar
  connect b.bored_signal, read(bounce)

Anyway.

[seldridge]

While this was hugely problematic previously, this is much easier now with the addition of Region in Chisel IR. This provides infrastructure such that operations can be appended at different locations in the IR and not just at the end of the module.

I think this can be made to work by:

1. Change layerblocks to contain a region (VectorBuilder[Command]) where statements can be pushed. Remove the token-based IR of LayerBegin/LayerEnd. See https://github.com/chipsalliance/chisel/pull/4184 which does the same thing, but for when.
2. Modify boring utils to do the right thing here by inserting commands inside the correct region.

This doesn't solve everything, specifically boring out of a when block. That should be illegal and prevented by Chisel at runtime if it is not already. (You aren't supposed to exfiltrate anything from a when block. It should be the same for layer blocks if it isn't already.)

This can go further and create the commands directly in the appropriate region. This is cleaner, IMO, but drew the ire of reviewers last time.