From 2b51053fe7744d6860416c7de8bcb99d4aa9e532 Mon Sep 17 00:00:00 2001 From: Jack Koenig Date: Wed, 8 Sep 2021 14:31:10 -0700 Subject: Restore return type of BaseModule.toTarget to ModuleTarget (#2117) Definition/Instance introduced the need for representing the targets of instances as InstanceTargets. This original implementation changed the return type of BaseModule.toTarget to express this need. This is a backwards incompatible change that is actually unnecessary because it is impossible for users to get references to the internal InstanceClone objects, instead only accessing such modules via Instance[_] wrappers and cloned Data. We restored the old API by adding a new internal method "getTarget" which will give the correct targets for InstanceClones while maintaining the API of BaseModule.toTarget.--- core/src/main/scala/chisel3/Module.scala | 36 ++++++++++++++-------- .../experimental/hierarchy/Definition.scala | 7 +++-- .../chisel3/experimental/hierarchy/Instance.scala | 12 ++++---- core/src/main/scala/chisel3/internal/Builder.scala | 2 +- 4 files changed, 35 insertions(+), 22 deletions(-) (limited to 'core/src') diff --git a/core/src/main/scala/chisel3/Module.scala b/core/src/main/scala/chisel3/Module.scala index d0171693..3ae48821 100644 --- a/core/src/main/scala/chisel3/Module.scala +++ b/core/src/main/scala/chisel3/Module.scala @@ -469,17 +469,29 @@ package experimental { * * @note Should not be called until circuit elaboration is complete */ - final def toTarget: IsModule = { - this match { - case m: internal.BaseModule.InstanceClone[_] if m._parent.nonEmpty => m._parent.get.toTarget.instOf(instanceName, name) - case m: internal.BaseModule.InstanceClone[_] => ModuleTarget(this.circuitName, this.name) - case m: internal.BaseModule.ModuleClone[_] if m._madeFromDefinition => m._parent.get.toTarget.instOf(instanceName, name) - case m: internal.BaseModule.ModuleClone[_] => ModuleTarget(this.circuitName, this.name) - // Without this, we get the wrong CircuitName for the Definition - case m: internal.BaseModule.DefinitionClone[_] if m._circuit.nonEmpty => ModuleTarget(this._circuit.get.circuitName, this.name) - case m: internal.BaseModule.DefinitionClone[_] => ModuleTarget(this.circuitName, this.name) - case m => ModuleTarget(this.circuitName, this.name) - } + final def toTarget: ModuleTarget = this match { + case m: internal.BaseModule.InstanceClone[_] => throwException(s"Internal Error! It's not legal to call .toTarget on an InstanceClone. $m") + case m: internal.BaseModule.DefinitionClone[_] => throwException(s"Internal Error! It's not legal to call .toTarget on an DefinitionClone. $m") + case _ => ModuleTarget(this.circuitName, this.name) + } + + /** Returns the real target of a Module which may be an [[InstanceTarget]] + * + * BaseModule.toTarget returns a ModuleTarget because the classic Module(new MyModule) API elaborates + * Modules in a way that there is a 1:1 relationship between instances and elaborated definitions + * + * Instance/Definition introduced special internal modules [[InstanceClone]] and [[ModuleClone]] that + * do not have this 1:1 relationship so need the ability to return [[InstanceTarget]]s. + * Because users can never actually get references to these underlying objects, we can maintain + * BaseModule.toTarget's API returning [[ModuleTarget]] while providing an internal API for getting + * the correct [[InstanceTarget]]s whenever using the Definition/Instance API. + */ + private[chisel3] def getTarget: IsModule = this match { + case m: internal.BaseModule.InstanceClone[_] if m._parent.nonEmpty => m._parent.get.getTarget.instOf(instanceName, name) + case m: internal.BaseModule.ModuleClone[_] if m._madeFromDefinition => m._parent.get.getTarget.instOf(instanceName, name) + // Without this, we get the wrong CircuitName for the Definition + case m: internal.BaseModule.DefinitionClone[_] if m._circuit.nonEmpty => ModuleTarget(this._circuit.get.circuitName, this.name) + case _ => this.toTarget } /** Returns a FIRRTL ModuleTarget that references this object @@ -496,7 +508,7 @@ package experimental { // is always unambigous. However, legacy workarounds for Chisel's lack of an instance API // have lead some to use .toAbsoluteTarget as a workaround. A proper instance API will make // it possible to deprecate and remove .toAbsoluteTarget - if (this == ViewParent) ViewParent.absoluteTarget else toTarget + if (this == ViewParent) ViewParent.absoluteTarget else getTarget } } diff --git a/core/src/main/scala/chisel3/experimental/hierarchy/Definition.scala b/core/src/main/scala/chisel3/experimental/hierarchy/Definition.scala index 2e917dfa..0cc3d131 100644 --- a/core/src/main/scala/chisel3/experimental/hierarchy/Definition.scala +++ b/core/src/main/scala/chisel3/experimental/hierarchy/Definition.scala @@ -3,13 +3,14 @@ package chisel3.experimental.hierarchy import scala.language.experimental.macros - import chisel3._ + import scala.collection.mutable.HashMap import chisel3.internal.{Builder, DynamicContext} import chisel3.internal.sourceinfo.{DefinitionTransform, DefinitionWrapTransform, SourceInfo} import chisel3.experimental.BaseModule import chisel3.internal.BaseModule.IsClone +import firrtl.annotations.{IsModule, ModuleTarget} /** User-facing Definition type. * Represents a definition of an object of type [[A]] which are marked as @instantiable @@ -64,12 +65,12 @@ object Definition extends SourceInfoDoc { /** If this is an instance of a Module, returns the toTarget of this instance * @return target of this instance */ - def toTarget = d.proto.toTarget + def toTarget: ModuleTarget = d.proto.toTarget /** If this is an instance of a Module, returns the toAbsoluteTarget of this instance * @return absoluteTarget of this instance */ - def toAbsoluteTarget = d.proto.toAbsoluteTarget + def toAbsoluteTarget: IsModule = d.proto.toAbsoluteTarget } /** A construction method to build a Definition of a Module * diff --git a/core/src/main/scala/chisel3/experimental/hierarchy/Instance.scala b/core/src/main/scala/chisel3/experimental/hierarchy/Instance.scala index aec42da3..9b17bfce 100644 --- a/core/src/main/scala/chisel3/experimental/hierarchy/Instance.scala +++ b/core/src/main/scala/chisel3/experimental/hierarchy/Instance.scala @@ -4,11 +4,11 @@ package chisel3.experimental.hierarchy import scala.collection.mutable.{ArrayBuffer, HashMap} import scala.language.experimental.macros - import chisel3._ -import chisel3.internal.BaseModule.{ModuleClone, IsClone, InstantiableClone} +import chisel3.internal.BaseModule.{InstantiableClone, IsClone, ModuleClone} import chisel3.internal.sourceinfo.{InstanceTransform, SourceInfo} import chisel3.experimental.BaseModule +import firrtl.annotations.IsModule /** User-facing Instance type. * Represents a unique instance of type [[A]] which are marked as @instantiable @@ -75,15 +75,15 @@ object Instance extends SourceInfoDoc { /** If this is an instance of a Module, returns the toTarget of this instance * @return target of this instance */ - def toTarget = i.cloned match { - case Left(x: BaseModule) => x.toTarget - case Right(x: IsClone[_] with BaseModule) => x.toTarget + def toTarget: IsModule = i.cloned match { + case Left(x: BaseModule) => x.getTarget + case Right(x: IsClone[_] with BaseModule) => x.getTarget } /** If this is an instance of a Module, returns the toAbsoluteTarget of this instance * @return absoluteTarget of this instance */ - def toAbsoluteTarget = i.cloned match { + def toAbsoluteTarget: IsModule = i.cloned match { case Left(x) => x.toAbsoluteTarget case Right(x: IsClone[_] with BaseModule) => x.toAbsoluteTarget } diff --git a/core/src/main/scala/chisel3/internal/Builder.scala b/core/src/main/scala/chisel3/internal/Builder.scala index 1d15247d..441abc92 100644 --- a/core/src/main/scala/chisel3/internal/Builder.scala +++ b/core/src/main/scala/chisel3/internal/Builder.scala @@ -309,7 +309,7 @@ private[chisel3] trait NamedComponent extends HasId { val root = _parent.map { case ViewParent => reifyParent case other => other - }.get.toTarget // All NamedComponents will have a parent, only the top module can have None here + }.get.getTarget // All NamedComponents will have a parent, only the top module can have None here Target.toTargetTokens(name).toList match { case TargetToken.Ref(r) :: components => root.ref(r).copy(component = components) case other => -- cgit v1.2.3