Merge pull request #1974 from chipsalliance/fix-clonemoduleasrecord-totarget

Fix CloneModuleAsRecord support for .toTarget

6 files changed, 115 insertions, 26 deletions

diff --git a/core/src/main/scala/chisel3/BlackBox.scala b/core/src/main/scala/chisel3/BlackBox.scala
index 8ba4b612..0c42600f 100644
--- a/core/src/main/scala/chisel3/BlackBox.scala
+++ b/core/src/main/scala/chisel3/BlackBox.scala
@@ -62,7 +62,7 @@ package experimental {
     * @note The parameters API is experimental and may change
     */
   abstract class ExtModule(val params: Map[String, Param] = Map.empty[String, Param]) extends BaseBlackBox {
-    private[chisel3] override def generateComponent(): Component = {
+    private[chisel3] override def generateComponent(): Option[Component] = {
       require(!_closed, "Can't generate module more than once")
       _closed = true
 
@@ -86,7 +86,7 @@ package experimental {
       val firrtlPorts = getModulePorts map {port => Port(port, port.specifiedDirection)}
       val component = DefBlackBox(this, name, firrtlPorts, SpecifiedDirection.Unspecified, params)
       _component = Some(component)
-      component
+      _component
     }
 
     private[chisel3] def initializeInParent(parentCompileOptions: CompileOptions): Unit = {
@@ -145,7 +145,7 @@ abstract class BlackBox(val params: Map[String, Param] = Map.empty[String, Param
   // Allow access to bindings from the compatibility package
   protected def _compatIoPortBound() = portsContains(_io)
 
-  private[chisel3] override def generateComponent(): Component = {
+  private[chisel3] override def generateComponent(): Option[Component] = {
     _compatAutoWrapPorts()  // pre-IO(...) compatibility hack
 
     // Restrict IO to just io, clock, and reset
@@ -178,7 +178,7 @@ abstract class BlackBox(val params: Map[String, Param] = Map.empty[String, Param
     val firrtlPorts = namedPorts map {namedPort => Port(namedPort._2, namedPort._2.specifiedDirection)}
     val component = DefBlackBox(this, name, firrtlPorts, _io.specifiedDirection, params)
     _component = Some(component)
-    component
+    _component
   }
 
   private[chisel3] def initializeInParent(parentCompileOptions: CompileOptions): Unit = {
diff --git a/core/src/main/scala/chisel3/Module.scala b/core/src/main/scala/chisel3/Module.scala
index b204be8d..8a914fbc 100644
--- a/core/src/main/scala/chisel3/Module.scala
+++ b/core/src/main/scala/chisel3/Module.scala
@@ -64,13 +64,18 @@ object Module extends SourceInfoDoc {
     Builder.currentClock = saveClock   // Back to clock and reset scope
     Builder.currentReset = saveReset
 
-    val component = module.generateComponent()
-    Builder.components += component
+    // Only add the component if the module generates one
+    val componentOpt = module.generateComponent()
+    for (component <- componentOpt) {
+      Builder.components += component
+    }
 
     Builder.setPrefix(savePrefix)
 
     // Handle connections at enclosing scope
-    if(!Builder.currentModule.isEmpty) {
+    // We use _component because Modules that don't generate them may still have one
+    if (Builder.currentModule.isDefined && module._component.isDefined) {
+      val component = module._component.get
       pushCommand(DefInstance(sourceInfo, module, component.ports))
       module.initializeInParent(compileOptions)
     }
@@ -178,20 +183,73 @@ package internal {
   import chisel3.experimental.BaseModule
 
   object BaseModule {
+    // Private internal class to serve as a _parent for Data in cloned ports
+    private[chisel3] class ModuleClone(_proto: BaseModule) extends BaseModule {
+      // ClonePorts that hold the bound ports for this module
+      // Used for setting the refs of both this module and the Record
+      private[BaseModule] var _portsRecord: Record = _
+      // Don't generate a component, but point to the one for the cloned Module
+      private[chisel3] def generateComponent(): Option[Component] = {
+        _component = _proto._component
+        None
+      }
+      // This module doesn't acutally exist in the FIRRTL so no initialization to do
+      private[chisel3] def initializeInParent(parentCompileOptions: CompileOptions): Unit = ()
+
+      override def desiredName: String = _proto.name
+
+      private[chisel3] def setRefAndPortsRef(namespace: Namespace): Unit = {
+        val record = _portsRecord
+        // Use .forceName to re-use default name resolving behavior
+        record.forceName(None, default=this.desiredName, namespace)
+        // Now take the Ref that forceName set and convert it to the correct Arg
+        val instName = record.getRef match {
+          case Ref(name) => name
+          case bad => throwException(s"Internal Error! Cloned-module Record $record has unexpected ref $bad")
+        }
+        // Set both the record and the module to have the same instance name
+        record.setRef(ModuleCloneIO(_proto, instName), force=true) // force because we did .forceName first
+        this.setRef(Ref(instName))
+      }
+    }
+
+    /** Record type returned by CloneModuleAsRecord
+      *
+      * @note These are not true Data (the Record doesn't correspond to anything in the emitted
+      * FIRRTL yet its elements *do*) so have some very specialized behavior.
+      */
     private[chisel3] class ClonePorts (elts: Data*)(implicit compileOptions: CompileOptions) extends Record {
       val elements = ListMap(elts.map(d => d.instanceName -> d.cloneTypeFull): _*)
       def apply(field: String) = elements(field)
       override def cloneType = (new ClonePorts(elts: _*)).asInstanceOf[this.type]
     }
 
+    // Recursively set the parent of the start Data and any children (eg. in an Aggregate)
+    private def setAllParents(start: Data, parent: Option[BaseModule]): Unit = {
+      def rec(data: Data): Unit = {
+        data._parent = parent
+        data match {
+        case _: Element =>
+        case agg: Aggregate =>
+          agg.getElements.foreach(rec)
+        }
+      }
+      rec(start)
+    }
+
     private[chisel3] def cloneIORecord(proto: BaseModule)(implicit sourceInfo: SourceInfo, compileOptions: CompileOptions): ClonePorts = {
       require(proto.isClosed, "Can't clone a module before module close")
+      // Fake Module to serve as the _parent of the cloned ports
+      // We make this before clonePorts because we want it to come up first in naming in
+      // currentModule
+      val cloneParent = Module(new ModuleClone(proto))
+      // We don't create this inside the ModuleClone because we need the ref to be set by the
+      // currentModule (and not clonePorts)
       val clonePorts = new ClonePorts(proto.getModulePorts: _*)
-      clonePorts.bind(WireBinding(Builder.forcedUserModule, Builder.currentWhen()))
-      val cloneInstance = new DefInstance(sourceInfo, proto, proto._component.get.ports) {
-        override def name = clonePorts.getRef.name
-      }
-      pushCommand(cloneInstance)
+      clonePorts.bind(PortBinding(cloneParent))
+      setAllParents(clonePorts, Some(cloneParent))
+      cloneParent._portsRecord = clonePorts
+      // Normally handled during Module construction but ClonePorts really lives in its parent's parent
       if (!compileOptions.explicitInvalidate) {
         pushCommand(DefInvalid(sourceInfo, clonePorts.ref))
       }
@@ -270,7 +328,7 @@ package experimental {
     /** Generates the FIRRTL Component (Module or Blackbox) of this Module.
       * Also closes the module so no more construction can happen inside.
       */
-    private[chisel3] def generateComponent(): Component
+    private[chisel3] def generateComponent(): Option[Component]
 
     /** Sets up this module in the parent context
       */
@@ -308,9 +366,12 @@ package experimental {
 
     /** Legalized name of this module. */
     final lazy val name = try {
-      // If this is a module aspect, it should share the same name as the original module
-      // Thus, the desired name should be returned without uniquification
-      if(this.isInstanceOf[ModuleAspect]) desiredName else Builder.globalNamespace.name(desiredName)
+      // ModuleAspects and ModuleClones are not "true modules" and thus should share
+      // their original modules names without uniquification
+      this match {
+        case (_: ModuleAspect | _: internal.BaseModule.ModuleClone) => desiredName
+        case _ => Builder.globalNamespace.name(desiredName)
+      }
     } catch {
       case e: NullPointerException => throwException(
         s"Error: desiredName of ${this.getClass.getName} is null. Did you evaluate 'name' before all values needed by desiredName were available?", e)
diff --git a/core/src/main/scala/chisel3/RawModule.scala b/core/src/main/scala/chisel3/RawModule.scala
index de93e781..4a60ca47 100644
--- a/core/src/main/scala/chisel3/RawModule.scala
+++ b/core/src/main/scala/chisel3/RawModule.scala
@@ -7,6 +7,7 @@ import scala.util.Try
 import scala.language.experimental.macros
 import chisel3.experimental.{BaseModule, BaseSim}
 import chisel3.internal._
+import chisel3.internal.BaseModule.ModuleClone
 import chisel3.internal.Builder._
 import chisel3.internal.firrtl._
 import chisel3.internal.sourceinfo.UnlocatableSourceInfo
@@ -57,7 +58,7 @@ abstract class RawModule(implicit moduleCompileOptions: CompileOptions)
   }
 
 
-  private[chisel3] override def generateComponent(): Component = {
+  private[chisel3] override def generateComponent(): Option[Component] = {
     require(!_closed, "Can't generate module more than once")
     _closed = true
 
@@ -74,6 +75,7 @@ abstract class RawModule(implicit moduleCompileOptions: CompileOptions)
     // All suggestions are in, force names to every node.
     for (id <- getIds) {
       id match {
+        case id: ModuleClone => id.setRefAndPortsRef(_namespace) // special handling
         case id: BaseModule => id.forceName(None, default=id.desiredName, _namespace)
         case id: MemBase[_] => id.forceName(None, default="MEM", _namespace)
         case id: BaseSim => id.forceName(None, default="SIM", _namespace)
@@ -130,7 +132,7 @@ abstract class RawModule(implicit moduleCompileOptions: CompileOptions)
     }
     val component = DefModule(this, name, firrtlPorts, invalidateCommands ++ getCommands)
     _component = Some(component)
-    component
+    _component
   }
 
   private[chisel3] def initializeInParent(parentCompileOptions: CompileOptions): Unit = {
@@ -221,7 +223,7 @@ package object internal {
     // Allow access to bindings from the compatibility package
     protected def _compatIoPortBound() = portsContains(_io)
 
-    private[chisel3] override def generateComponent(): Component = {
+    private[chisel3] override def generateComponent(): Option[Component] = {
       _compatAutoWrapPorts()  // pre-IO(...) compatibility hack
 
       // Restrict IO to just io, clock, and reset
diff --git a/core/src/main/scala/chisel3/internal/Builder.scala b/core/src/main/scala/chisel3/internal/Builder.scala
index e1e4d460..35c4bdf9 100644
--- a/core/src/main/scala/chisel3/internal/Builder.scala
+++ b/core/src/main/scala/chisel3/internal/Builder.scala
@@ -84,7 +84,7 @@ trait InstanceId {
 
 private[chisel3] trait HasId extends InstanceId {
   private[chisel3] def _onModuleClose: Unit = {}
-  private[chisel3] val _parent: Option[BaseModule] = Builder.currentModule
+  private[chisel3] var _parent: Option[BaseModule] = Builder.currentModule
 
   private[chisel3] val _id: Long = Builder.idGen.next
 
diff --git a/core/src/main/scala/chisel3/internal/firrtl/Converter.scala b/core/src/main/scala/chisel3/internal/firrtl/Converter.scala
index 40d3691c..093d4848 100644
--- a/core/src/main/scala/chisel3/internal/firrtl/Converter.scala
+++ b/core/src/main/scala/chisel3/internal/firrtl/Converter.scala
@@ -24,16 +24,24 @@ private[chisel3] object Converter {
     case Percent => ("%%", List.empty)
   }
 
+  private def reportInternalError(msg: String): Nothing = {
+    val link = "https://github.com/chipsalliance/chisel3/issues/new"
+    val fullMsg = s"Internal Error! $msg This is a bug in Chisel, please file an issue at '$link'"
+    throwException(fullMsg)
+  }
+
   def getRef(id: HasId, sourceInfo: SourceInfo): Arg =
     id.getOptionRef.getOrElse {
       val module = id._parent.map(m => s" '$id' was defined in module '$m'.").getOrElse("")
       val loc = sourceInfo.makeMessage(" " + _)
-      val link = "https://github.com/chipsalliance/chisel3/issues/new"
-      val msg = s"Internal error! Could not get ref for '$id'$loc!$module " +
-                s"This is a bug in Chisel, please file an issue at '$link'."
-      throwException(msg)
+      reportInternalError(s"Could not get ref for '$id'$loc!$module")
     }
 
+  private def clonedModuleIOError(mod: BaseModule, name: String, sourceInfo: SourceInfo): Nothing = {
+    val loc = sourceInfo.makeMessage(" " + _)
+    reportInternalError(s"Trying to convert a cloned IO of $mod inside of $mod itself$loc!")
+  }
+
   def convert(info: SourceInfo): fir.Info = info match {
     case _: NoSourceInfo => fir.NoInfo
     case SourceLine(fn, line, col) => fir.FileInfo(fir.StringLit(s"$fn $line:$col"))
@@ -65,6 +73,9 @@ private[chisel3] object Converter {
     case ModuleIO(mod, name) =>
       if (mod eq ctx.id) fir.Reference(name, fir.UnknownType)
       else fir.SubField(fir.Reference(getRef(mod, info).name, fir.UnknownType), name, fir.UnknownType)
+    case ModuleCloneIO(mod, name) =>
+      if (mod eq ctx.id) clonedModuleIOError(mod, name, info)
+      else fir.Reference(name)
     case u @ ULit(n, UnknownWidth()) =>
       fir.UIntLiteral(n, fir.IntWidth(u.minWidth))
     case ULit(n, w) =>
diff --git a/core/src/main/scala/chisel3/internal/firrtl/IR.scala b/core/src/main/scala/chisel3/internal/firrtl/IR.scala
index 5dc72a43..a4f6d26d 100644
--- a/core/src/main/scala/chisel3/internal/firrtl/IR.scala
+++ b/core/src/main/scala/chisel3/internal/firrtl/IR.scala
@@ -161,13 +161,28 @@ case class IntervalLit(n: BigInt, w: Width, binaryPoint: BinaryPoint) extends Li
 }
 
 case class Ref(name: String) extends Arg
+/** Arg for ports of Modules
+  * @param mod the module this port belongs to
+  * @param name the name of the port
+  */
 case class ModuleIO(mod: BaseModule, name: String) extends Arg {
   override def fullName(ctx: Component): String =
     if (mod eq ctx.id) name else s"${mod.getRef.name}.$name"
 }
-case class Slot(imm: Node, name: String) extends Arg {
+/** Ports of cloned modules (CloneModuleAsRecord)
+  * @param mod The original module for which these ports are a clone
+  * @param name the name of the module instance
+  */
+case class ModuleCloneIO(mod: BaseModule, name: String) extends Arg {
   override def fullName(ctx: Component): String =
-    if (imm.fullName(ctx).isEmpty) name else s"${imm.fullName(ctx)}.${name}"
+    // NOTE: mod eq ctx.id only occurs in Target and Named-related APIs
+    if (mod eq ctx.id) "" else name
+}
+case class Slot(imm: Node, name: String) extends Arg {
+  override def fullName(ctx: Component): String = {
+    val immName = imm.fullName(ctx)
+    if (immName.isEmpty) name else s"$immName.$name"
+  }
 }
 case class Index(imm: Arg, value: Arg) extends Arg {
   def name: String = s"[$value]"