Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix Nested Instantiate #4018

Merged
merged 3 commits into from
Apr 24, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -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}
Expand Down Expand Up @@ -108,8 +108,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
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
11 changes: 3 additions & 8 deletions core/src/main/scala/chisel3/internal/Builder.scala
Original file line number Diff line number Diff line change
Expand Up @@ -454,8 +454,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
Expand Down Expand Up @@ -506,7 +507,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]()
Expand All @@ -521,8 +521,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
Expand Down Expand Up @@ -596,9 +594,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
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
10 changes: 7 additions & 3 deletions src/main/scala/chisel3/aop/injecting/InjectingAspect.scala
Original file line number Diff line number Diff line change
Expand Up @@ -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}
Expand All @@ -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)
Expand Down Expand Up @@ -67,8 +70,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
Expand Down
10 changes: 7 additions & 3 deletions src/main/scala/chisel3/stage/phases/Elaborate.scala
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -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 {
Expand All @@ -42,8 +45,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)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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}
Expand Down Expand Up @@ -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") {
Expand Down Expand Up @@ -475,4 +491,12 @@ class InstantiateSpec extends ChiselFunSpec with Utils {
)
)
}

it("Nested Instantiate should work") {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reproduce via:

mill chiselut[2.13.12].testOnly chiselTests.experimental.hierarchy.InstantiateSpec

Here is log:

List("Bar", "Bar_1", "Bar_2", "Bar_3", "Baz", "Baz_1", "Baz_2", "Baz_3", "Foo", "Foo_1", "Top") did not equal List("Bar", "Bar_1", "Baz", "Foo", "Foo_1", "Top")
ScalaTestFailureLocation: chiselTests.experimental.hierarchy.InstantiateSpec at (InstantiateSpec.scala:500)
Expected :List("Bar", "Bar_1", "Baz", "Foo", "Foo_1", "Top")
Actual   :List("Bar", "Bar_1", "Bar_2", "Bar_3", "Baz", "Baz_1", "Baz_2", "Baz_3", "Foo", "Foo_1", "Top")

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)
}
}
Loading