BoringUtils.bore does not work properly in `when` scope

[Tang-Haojin]

Type of issue: Bug Report

Please provide the steps to reproduce the problem:

class Top extends Module {
  val io = IO(Input(Bool()))

  val inner = Module(new Inner)
  when(io) {
    val b = BoringUtils.bore(inner.a)
  }
}

class Inner extends Module {
  val a = WireDefault(false.B)
}

What is the current behavior?

STDERR:
/nfs-nvme/home/tanghaojin/chisel-template/Top.fir:22:13: error: use of unknown declaration 'b'
    connect b, inner.b_bore @[src/main/scala/gcd/GCD.scala 17:29]
            ^

And the firrtl output is

FIRRTL version 3.1.0
circuit Top :
  module Inner :
    input clock : Clock
    input reset : Reset
    output b_bore : UInt<1> @[src/main/scala/gcd/GCD.scala 22:29]

    wire a : UInt<1> @[src/main/scala/gcd/GCD.scala 27:22]
    connect a, UInt<1>(0h0) @[src/main/scala/gcd/GCD.scala 27:22]
    connect b_bore, a @[src/main/scala/gcd/GCD.scala 22:29]

  module Top :
    input clock : Clock
    input reset : UInt<1>
    input io : UInt<1> @[src/main/scala/gcd/GCD.scala 18:14]

    inst inner of Inner @[src/main/scala/gcd/GCD.scala 20:21]
    connect inner.clock, clock
    connect inner.reset, reset
    when io : @[src/main/scala/gcd/GCD.scala 21:12]
      wire b : UInt<1> @[src/main/scala/gcd/GCD.scala 22:29]
    connect b, inner.b_bore @[src/main/scala/gcd/GCD.scala 22:29]

What is the expected behavior? Pass verilog generation

Please tell us about your environment:

• version: 6.0.0-M3
• OS: Linux xxx 5.15.0-69-generic #76~20.04.1-Ubuntu SMP Mon Mar 20 15:54:19 UTC 2023 x86_64 x86_64 x86_64 GNU/Linux

Other Information

What is the use case for changing the behavior?

[sequencer]

OK this is certainly a bug. Like declaring register or memory inside when block. In the boring utils case, I personally think it should be forbidden.

[Tang-Haojin]

OK this is certainly a bug. Like declaring register or memory inside when block. In the boring utils case, I personally think it should be forbidden.

I agree that boring utils should not be used in when scope directly. But in our case, it is wrapped in some APIs, and these APIs are used in when. Maybe we still need boring utils working in when properly.

[jackkoenig]

Discussing in Chisel dev, we can probably fix this with a new internal* API that allows optionally instantiating a new Wire or Reg (or IO?) outside of the when scope. BoringUtils would then use this API to ensure there is a wire to connect to outside of the when.

[sequencer]

Just noticed, did we have same issue for Mem.read, Mem.write in when block sometime ago?

[sequencer]

FYI, https://github.com/llvm/circt/issues/1561 may also relate to this issue.

[dtzSiFive]

While sharing kinda related bits--

I'm not sure if it's reachable or good Chisel if it's possible, but FWIW CIRCT's wiring for taps is careful to insert connect's in a place that is safe re:the things it's connecting, primarily to ensure can plumb probes to declarations under when and especially to ensure could XMR into a module instantiated under a when, see: https://github.com/llvm/circt/pull/4323/files#diff-8d84fe9849e43b9a0268cc454d6dc82d0dbe16e18392f355ff1d155f41c323b9 -- idea being if you can instantiate a module under a when that shouldn't mean you can't access its contents via tap/probe/XMR. Is this something that could be done here (also, "should" it?)?

I'll see if can put together an example for being able to tap declarations under when (vs here where the boring command itself is in the when), since as long as we can declare things that way they seem like they should be tap-able, but if that's obviously the same scenario please LMK :wink: . If nothing else should be able to define them out manually if needed, hopefully.

Never taught it to insert temporary/wire for case of wiring between two declarations under sibling regions (when/else blocks, "scopes"?), partly because this is only likely to be a thing to route out XMR's, just a heads-up (there's a test for this in that link commit FWIW).

[seldridge]

@dtzSiFive: having Chisel be able to add statements to do the connections inside the when (carefully and safely!) is the best outcome here. This, however, very much goes against the design of Chisel's internals which wants to assume that the construction of Chisel's IR is created from a serial execution of a Chisel module's constructor that: (1) changes builder state to indicate that we are now in a new module, (2) serially "pushes" commands into a command array for that module, (3) "closes" the module when the constructor finishes, and (4) pops back to the previous module. Technologies (D/I) were then built on top of this which assumed that modules were unchanging after (3) happens.

To get around this, two separate approaches have been tried:

1. Injecting Aspects pack FIRRTL text into a separate area and then rely on a FIRRTL compiler to append this text. This cannot add ports. This is now largely deprecated.
2. Modern BoringUtils adds secret connections and secret ports alongside the normal builder command array to allow for more stuff to be added to a module (with restrictions on adding ports in certain situations).

When (2) was added, this was a compromise because of concerns around Chisel's internals and the incompatibility of arbitrary modifications with D/I.

That said, we're rapidly approaching a point where rethinking Chisel's internals to use a more standard compiler architecture makes sense. (Contrary to common wisdom that linked lists are never used outside of coding interviews, there is a sound reason why intrusive linked lists are the building blocks of both LLVM and MLIR compiler IRs. 😉)

tl;dr: Yes, we should be able to add these directly from Chisel, inside the when blocks, without resorting to clever tricks.

[dtzSiFive]

This specific example was fixed by #4399, FWIW :

//> using repository "sonatype-s01:snapshots"
//> using scala "2.13.14"
//> using dep "org.chipsalliance::chisel:7.0.0-M2+76-ecda00a5-SNAPSHOT"
//> using plugin "org.chipsalliance:::chisel-plugin:7.0.0-M2+76-ecda00a5-SNAPSHOT"
//> using options "-unchecked", "-deprecation", "-language:reflectiveCalls", "-feature", "-Xcheckinit", "-Xfatal-warnings", "-Ywarn-dead-code", "-Ywarn-unused", "-Ymacro-annotations"

import chisel3._
import chisel3.layer.{Layer, LayerConfig}
import circt.stage.ChiselStage
import chisel3.probe.{define, read, Probe, ProbeValue}
import chisel3.util.experimental.BoringUtils

class Top extends Module {
  val io = IO(Input(Bool()))

  val inner = Module(new Inner)
  when(io) {
    val b = BoringUtils.bore(inner.a)
  }
}

class Inner extends Module {
  val a = WireDefault(false.B)
}

object Main extends App {
  println(ChiselStage.emitCHIRRTL(new Top))
  println(ChiselStage.emitSystemVerilog(new Top))
}

Which now produces:

FIRRTL version 4.0.0
circuit Top :
  layer Verification, bind, "Verification" :
    layer Assert, bind, "Verification/Assert" :
    layer Assume, bind, "Verification/Assume" :
    layer Cover, bind, "Verification/Cover" :
  module Inner : @[issue-3523.scala 22:7]
    input clock : Clock @[issue-3523.scala 22:7]
    input reset : Reset @[issue-3523.scala 22:7]
    output b_bore : UInt<1> @[issue-3523.scala 18:29]

    wire a : UInt<1> @[issue-3523.scala 23:22]
    connect a, UInt<1>(0h0) @[issue-3523.scala 23:22]
    connect b_bore, a @[issue-3523.scala 18:29]

  public module Top : @[issue-3523.scala 13:7]
    input clock : Clock @[issue-3523.scala 13:7]
    input reset : UInt<1> @[issue-3523.scala 13:7]
    input io : UInt<1> @[issue-3523.scala 14:14]

    inst inner of Inner @[issue-3523.scala 16:21]
    connect inner.clock, clock
    connect inner.reset, reset
    when io : @[issue-3523.scala 17:12]
      wire b : UInt<1> @[issue-3523.scala 18:29]
      connect b, inner.b_bore @[issue-3523.scala 18:29]

What doesn't work still, and partially captured by the "ignore"d test in that PR, is boring into when's/layers AFTER they're built.