Fix Nested Instantiate (#4018) (#4027)

* minimal reproduce Nested Instantiate bug

* expose cache and elaborated defination to inner defination

* fix up

(cherry picked from commit 02b01e8)

Co-authored-by: Jiuyang Liu <liu@jiuyang.me>

core/src/main/scala/chisel3/experimental/hierarchy/core/Definition.scala

@@ -5,7 +5,7 @@ package chisel3.experimental.hierarchy.core
 import scala.language.experimental.macros
 import chisel3._
 
-import scala.collection.mutable.HashMap
+import scala.collection.mutable.{ArrayBuffer, HashMap}
 import chisel3.internal.{Builder, DynamicContext}
 import chisel3.internal.sourceinfo.{DefinitionTransform, DefinitionWrapTransform}
 import chisel3.experimental.{BaseModule, SourceInfo}
@@ -107,8 +107,9 @@ object Definition extends SourceInfoDoc {
         context.warningFilters,
         context.sourceRoots,
         Some(context.globalNamespace),
-        Builder.allDefinitions,
-        context.loggerOptions
+        context.loggerOptions,
+        context.definitions,
+        context.contextCache
       )
     }
     dynamicContext.inDefinition = true

core/src/main/scala/chisel3/experimental/hierarchy/core/Instance.scala

@@ -122,7 +122,7 @@ object Instance extends SourceInfoDoc {
     implicit sourceInfo: SourceInfo
   ): Instance[T] = {
     // Check to see if the module is already defined internally or externally
-    val existingMod = Builder.allDefinitions.view.flatten.map(_.proto).exists {
+    val existingMod = Builder.definitions.view.map(_.proto).exists {
       case c: Class               => c == definition.proto
       case c: RawModule           => c == definition.proto
       case c: BaseBlackBox        => c.name == definition.proto.name

core/src/main/scala/chisel3/internal/Builder.scala

@@ -453,8 +453,9 @@ private[chisel3] class DynamicContext(
   val sourceRoots:       Seq[File],
   val defaultNamespace:  Option[Namespace],
   // Definitions from other scopes in the same elaboration, use allDefinitions below
-  val outerScopeDefinitions: List[Iterable[Definition[_]]],
-  val loggerOptions:         LoggerOptions) {
+  val loggerOptions: LoggerOptions,
+  val definitions:   ArrayBuffer[Definition[_]],
+  val contextCache:  BuilderContextCache) {
   val importedDefinitionAnnos = annotationSeq.collect { case a: ImportDefinitionAnnotation[_] => a }
 
   // Map from proto module name to ext-module name
@@ -505,7 +506,6 @@ private[chisel3] class DynamicContext(
   }
 
   val components = ArrayBuffer[Component]()
-  val definitions = ArrayBuffer[Definition[_]]()
   val annotations = ArrayBuffer[ChiselAnnotation]()
   val newAnnotations = ArrayBuffer[ChiselMultiAnnotation]()
   val layers = mutable.LinkedHashSet[layer.Layer]()
@@ -520,8 +520,6 @@ private[chisel3] class DynamicContext(
   // Views that do not correspond to a single ReferenceTarget and thus require renaming
   val unnamedViews: ArrayBuffer[Data] = ArrayBuffer.empty
 
-  val contextCache: BuilderContextCache = BuilderContextCache.empty
-
   // Set by object Module.apply before calling class Module constructor
   // Used to distinguish between no Module() wrapping, multiple wrappings, and rewrapping
   var readyForModuleConstr: Boolean = false
@@ -594,9 +592,6 @@ private[chisel3] object Builder extends LazyLogging {
   def components:  ArrayBuffer[Component] = dynamicContext.components
   def definitions: ArrayBuffer[Definition[_]] = dynamicContext.definitions
 
-  /** All definitions from current elaboration, including Definitions passed as an argument to this one */
-  def allDefinitions: List[Iterable[Definition[_]]] = definitions :: dynamicContext.outerScopeDefinitions
-
   def annotations: ArrayBuffer[ChiselAnnotation] = dynamicContext.annotations
 
   def layers:  mutable.LinkedHashSet[layer.Layer] = dynamicContext.layers

core/src/main/scala/chisel3/internal/BuilderContextCache.scala

@@ -9,7 +9,7 @@ private[chisel3] object BuilderContextCache {
   /** Users of the [[BuilderContextCache]] must use a subclass of this type as a map key */
   abstract class Key[A]
 
-  private[internal] def empty = new BuilderContextCache
+  def empty = new BuilderContextCache
 }
 
 import BuilderContextCache.Key

src/main/scala/chisel3/aop/injecting/InjectingAspect.scala

@@ -4,7 +4,8 @@ package chisel3.aop.injecting
 
 import chisel3.{withClockAndReset, Module, ModuleAspect, RawModule}
 import chisel3.aop._
-import chisel3.internal.{Builder, DynamicContext}
+import chisel3.experimental.hierarchy.core.Definition
+import chisel3.internal.{Builder, BuilderContextCache, DynamicContext}
 import chisel3.internal.firrtl.ir.DefModule
 import chisel3.stage.{ChiselOptions, DesignAnnotation}
 import firrtl.annotations.{Annotation, ModuleTarget}
@@ -14,6 +15,8 @@ import firrtl.{ir, _}
 import scala.collection.mutable
 import logger.LoggerOptions
 
+import scala.collection.mutable.ArrayBuffer
+
 /** Aspect to inject Chisel code into a module of type M
   *
   * @param selectRoots Given top-level module, pick the instances of a module to apply the aspect (root module)
@@ -66,8 +69,9 @@ abstract class InjectorAspect[T <: RawModule, M <: RawModule](
           chiselOptions.warningFilters,
           chiselOptions.sourceRoots,
           None,
-          Nil, // FIXME this maybe should somehow grab definitions from earlier elaboration
-          loggerOptions
+          loggerOptions,
+          ArrayBuffer[Definition[_]](),
+          BuilderContextCache.empty
         )
       // Add existing module names into the namespace. If injection logic instantiates new modules
       //  which would share the same name, they will get uniquified accordingly

src/main/scala/chisel3/stage/phases/Elaborate.scala

@@ -3,8 +3,9 @@
 package chisel3.stage.phases
 
 import chisel3.Module
+import chisel3.experimental.hierarchy.core.Definition
 import chisel3.internal.ExceptionHelpers.ThrowableHelpers
-import chisel3.internal.{Builder, DynamicContext}
+import chisel3.internal.{Builder, BuilderContextCache, DynamicContext}
 import chisel3.stage.{
   ChiselCircuitAnnotation,
   ChiselGeneratorAnnotation,
@@ -17,6 +18,8 @@ import firrtl.options.{Dependency, Phase}
 import firrtl.options.Viewer.view
 import logger.LoggerOptions
 
+import scala.collection.mutable.ArrayBuffer
+
 /** Elaborate all [[chisel3.stage.ChiselGeneratorAnnotation]]s into [[chisel3.stage.ChiselCircuitAnnotation]]s.
   */
 class Elaborate extends Phase {
@@ -41,8 +44,9 @@ class Elaborate extends Phase {
             chiselOptions.warningFilters,
             chiselOptions.sourceRoots,
             None,
-            Nil,
-            loggerOptions
+            loggerOptions,
+            ArrayBuffer[Definition[_]](),
+            BuilderContextCache.empty
           )
         val (circuit, dut) =
           Builder.build(Module(gen()), context)

src/test/scala/chiselTests/experimental/hierarchy/InstantiateSpec.scala

@@ -5,6 +5,7 @@ package experimental.hierarchy
 
 import chisel3._
 import chisel3.util.Valid
+import chisel3.properties._
 import chisel3.experimental.hierarchy._
 import circt.stage.ChiselStage.convert
 import chisel3.experimental.{ExtModule, IntrinsicModule}
@@ -163,13 +164,28 @@ object InstantiateSpec {
     @public val in = IO(Input(UInt(8.W)))
     @public val out = IO(Output(UInt(8.W)))
   }
+
+  @instantiable
+  class Baz extends Module
+
+  @instantiable
+  class Bar(i: Int) extends Module {
+    val baz = Instantiate(new Baz)
+  }
+  @instantiable
+  class Foo(i: Int) extends Module {
+    val bar0 = Instantiate(new Bar(0))
+    val bar1 = Instantiate(new Bar(1))
+    val bar11 = Instantiate(new Bar(1))
+  }
 }
 
 class ParameterizedReset(hasAsyncNotSyncReset: Boolean) extends Module {
   override def resetType = if (hasAsyncNotSyncReset) Module.ResetType.Asynchronous else Module.ResetType.Synchronous
 }
 
 class InstantiateSpec extends ChiselFunSpec with Utils {
+
   import InstantiateSpec._
 
   describe("Module classes that take no arguments") {
@@ -475,4 +491,12 @@ class InstantiateSpec extends ChiselFunSpec with Utils {
       )
     )
   }
+
+  it("Nested Instantiate should work") {
+    class MyTop extends Top {
+      val inst0 = Instantiate(new Foo(0))
+      val inst1 = Instantiate(new Foo(1))
+    }
+    assert(convert(new MyTop).modules.map(_.name).sorted == Seq("Bar", "Bar_1", "Baz", "Foo", "Foo_1", "Top").sorted)
+  }
 }