From 3b53ccb56f7a9e460c953450086df56fcc18c4a8 Mon Sep 17 00:00:00 2001
From: Will Dietz <will.dietz@sifive.com>
Date: Thu, 12 Sep 2024 19:01:39 -0500
Subject: [PATCH] Flush staged commands before/after enter/exit regions.
 (#4399)

Fix BoringUtils (see tests) placement of connection commands
so that they are added to the current command buffer.

Flush staged commands before changing regions to avoid needing
multiple "staging" buffer lists.
Alternatively the current list could be saved and restored.

Tests from #4108 are included and finished, with this change
two of them now pass (the ones encountered in practice, FWIW).
The third is included but marked "ignore" as future work.

This is a bit of a change regarding "staged" commands, as now
before the module is closed they are no longer "secret" since
they are added to the normal command buffers.
They are still staged (deferred) which appears to be useful
to ensure they appear after what's currently being constructed
(specifically, not before).
---
 core/src/main/scala/chisel3/RawModule.scala   |  5 +
 .../chiselTests/BoringUtilsTapSpec.scala      | 93 +++++++++++++++++++
 2 files changed, 98 insertions(+)

diff --git a/core/src/main/scala/chisel3/RawModule.scala b/core/src/main/scala/chisel3/RawModule.scala
index feba720e30f..d3ba4b5a62e 100644
--- a/core/src/main/scala/chisel3/RawModule.scala
+++ b/core/src/main/scala/chisel3/RawModule.scala
@@ -97,9 +97,13 @@ abstract class RawModule extends BaseModule {
   }
 
   private[chisel3] def withRegion[A](newRegion: VectorBuilder[Command])(thunk: => A): A = {
+    _currentRegion ++= stagedSecretCommands
+    stagedSecretCommands.clear()
     var oldRegion = _currentRegion
     changeRegion(newRegion)
     val result = thunk
+    _currentRegion ++= stagedSecretCommands
+    stagedSecretCommands.clear()
     changeRegion(oldRegion)
     result
   }
@@ -220,6 +224,7 @@ abstract class RawModule extends BaseModule {
 
     // Secret connections can be staged if user bored into children modules
     component.secretCommands ++= stagedSecretCommands
+    stagedSecretCommands.clear()
     _component = Some(component)
     _component
   }
diff --git a/src/test/scala/chiselTests/BoringUtilsTapSpec.scala b/src/test/scala/chiselTests/BoringUtilsTapSpec.scala
index 2bff53ae354..ce993125d07 100644
--- a/src/test/scala/chiselTests/BoringUtilsTapSpec.scala
+++ b/src/test/scala/chiselTests/BoringUtilsTapSpec.scala
@@ -57,6 +57,42 @@ class BoringUtilsTapSpec extends ChiselFlatSpec with ChiselRunners with Utils wi
     )()
   }
 
+  // This test requires ability to identify what region to add commands to,
+  // *after* building them.  This is not yet supported.
+  ignore should "work downwards from grandparent to grandchild through when" in {
+    class Bar extends RawModule {
+      val internalWire = Wire(Bool())
+    }
+    class Foo extends RawModule {
+      when(true.B) {
+        val bar = Module(new Bar)
+      }
+    }
+    class Top extends RawModule {
+      val foo = Module(new Foo)
+      val out = IO(Bool())
+      out := DontCare
+
+      when(true.B) {
+        val w = WireInit(
+          Bool(),
+          BoringUtils.tapAndRead((chisel3.aop.Select.collectDeep(foo) { case b: Bar => b }).head.internalWire)
+        )
+      }
+    }
+    val chirrtl = circt.stage.ChiselStage.emitCHIRRTL(new Top)
+
+    // The define should be at the end of the when block.
+    matchesAndOmits(chirrtl)(
+      "    when UInt<1>(0h1) :",
+      "      inst bar of Bar",
+      "      define w_bore = bar.w_bore"
+    )()
+
+    // Check is valid FIRRTL.
+    circt.stage.ChiselStage.emitFIRRTLDialect(new Top)
+  }
+
   it should "work upwards from child to parent" in {
     class Foo(parentData: Data) extends RawModule {
       val outProbe = IO(probe.Probe(Bool()))
@@ -109,6 +145,63 @@ class BoringUtilsTapSpec extends ChiselFlatSpec with ChiselRunners with Utils wi
     )()
   }
 
+  it should "work upwards from grandchild to grandparent through when" in {
+    class Bar(grandParentData: Data) extends RawModule {
+      val out = IO(Bool())
+      out := BoringUtils.tapAndRead(grandParentData)
+    }
+    class Foo(parentData: Data) extends RawModule {
+      when(true.B) {
+        val bar = Module(new Bar(parentData))
+      }
+    }
+    class Top extends RawModule {
+      val parentWire = Wire(Bool())
+      parentWire := DontCare
+      val foo = Module(new Foo(parentWire))
+    }
+    val chirrtl = circt.stage.ChiselStage.emitCHIRRTL(new Top)
+
+    // The connect should be at the end of the when block.
+    matchesAndOmits(chirrtl)(
+      "    when UInt<1>(0h1) :",
+      "      inst bar of Bar",
+      "      connect bar.out_bore, out_bore"
+    )()
+
+    // Check is valid FIRRTL.
+    circt.stage.ChiselStage.emitFIRRTLDialect(new Top)
+  }
+
+  it should "work upwards from grandchild to grandparent into layer" in {
+    object TestLayer extends layer.Layer(layer.LayerConfig.Extract())
+    class Bar(grandParentData: Data) extends RawModule {
+      val out = IO(Bool())
+      out := BoringUtils.tapAndRead(grandParentData)
+    }
+    class Foo(parentData: Data) extends RawModule {
+      layer.block(TestLayer) {
+        val bar = Module(new Bar(parentData))
+      }
+    }
+    class Top extends RawModule {
+      val parentWire = Wire(Bool())
+      parentWire := DontCare
+      val foo = Module(new Foo(parentWire))
+    }
+    val chirrtl = circt.stage.ChiselStage.emitCHIRRTL(new Top)
+
+    // The connect should be at the end of the layerblock.
+    matchesAndOmits(chirrtl)(
+      "    layerblock TestLayer :",
+      "      inst bar of Bar",
+      "      connect bar.out_bore, out_bore"
+    )()
+
+    // Check is valid FIRRTL and builds to SV.
+    circt.stage.ChiselStage.emitSystemVerilog(new Top)
+  }
+
   it should "work from child to its sibling" in {
     class Bar extends RawModule {
       val a = Wire(Bool())