From 5f0049dbb958154edd1526a27c9abbefaad91317 Mon Sep 17 00:00:00 2001 From: Jiuyang Liu Date: Wed, 24 Apr 2024 14:16:53 +0800 Subject: [PATCH] Fix Nested Instantiate (#4018) * minimal reproduce Nested Instantiate bug * expose cache and elaborated defination to inner defination * fix up (cherry picked from commit 02b01e8b6f8f0fb46ed4fb985029e754c187c540) --- .../hierarchy/core/Definition.scala | 7 +++--- .../hierarchy/core/Instance.scala | 2 +- .../main/scala/chisel3/internal/Builder.scala | 11 +++------ .../internal/BuilderContextCache.scala | 2 +- .../aop/injecting/InjectingAspect.scala | 10 +++++--- .../chisel3/stage/phases/Elaborate.scala | 10 +++++--- .../hierarchy/InstantiateSpec.scala | 24 +++++++++++++++++++ 7 files changed, 47 insertions(+), 19 deletions(-) diff --git a/core/src/main/scala/chisel3/experimental/hierarchy/core/Definition.scala b/core/src/main/scala/chisel3/experimental/hierarchy/core/Definition.scala index 41d7f2fbc34..6ca182822da 100644 --- a/core/src/main/scala/chisel3/experimental/hierarchy/core/Definition.scala +++ b/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 diff --git a/core/src/main/scala/chisel3/experimental/hierarchy/core/Instance.scala b/core/src/main/scala/chisel3/experimental/hierarchy/core/Instance.scala index 82442d443ec..574d36d29c7 100644 --- a/core/src/main/scala/chisel3/experimental/hierarchy/core/Instance.scala +++ b/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 diff --git a/core/src/main/scala/chisel3/internal/Builder.scala b/core/src/main/scala/chisel3/internal/Builder.scala index 4c9bb893b0e..70202d8af59 100644 --- a/core/src/main/scala/chisel3/internal/Builder.scala +++ b/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 diff --git a/core/src/main/scala/chisel3/internal/BuilderContextCache.scala b/core/src/main/scala/chisel3/internal/BuilderContextCache.scala index 45166829eeb..7ab4f7bfd6a 100644 --- a/core/src/main/scala/chisel3/internal/BuilderContextCache.scala +++ b/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 diff --git a/src/main/scala/chisel3/aop/injecting/InjectingAspect.scala b/src/main/scala/chisel3/aop/injecting/InjectingAspect.scala index 24b7fc0d2f4..9de238830bd 100644 --- a/src/main/scala/chisel3/aop/injecting/InjectingAspect.scala +++ b/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 diff --git a/src/main/scala/chisel3/stage/phases/Elaborate.scala b/src/main/scala/chisel3/stage/phases/Elaborate.scala index 06b19197d3e..a62a9e30abd 100644 --- a/src/main/scala/chisel3/stage/phases/Elaborate.scala +++ b/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) diff --git a/src/test/scala/chiselTests/experimental/hierarchy/InstantiateSpec.scala b/src/test/scala/chiselTests/experimental/hierarchy/InstantiateSpec.scala index 726582afe5f..c3e059c17c9 100644 --- a/src/test/scala/chiselTests/experimental/hierarchy/InstantiateSpec.scala +++ b/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,6 +164,20 @@ 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 { @@ -170,6 +185,7 @@ class ParameterizedReset(hasAsyncNotSyncReset: Boolean) extends Module { } 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) + } }