diff options
4 files changed, 71 insertions, 10 deletions
diff --git a/chiselFrontend/src/main/scala/chisel3/core/Aggregate.scala b/chiselFrontend/src/main/scala/chisel3/core/Aggregate.scala index 732bf8fc..efb30326 100644 --- a/chiselFrontend/src/main/scala/chisel3/core/Aggregate.scala +++ b/chiselFrontend/src/main/scala/chisel3/core/Aggregate.scala @@ -374,7 +374,10 @@ abstract class Record extends Aggregate { // NOTE: This sets up dependent references, it can be done before closing the Module private[chisel3] override def _onModuleClose: Unit = { // scalastyle:ignore method.name - val _namespace = Builder.globalNamespace.child + // Since Bundle names this via reflection, it is impossible for two elements to have the same + // identifier; however, Namespace sanitizes identifiers to make them legal for Firrtl/Verilog + // which can cause collisions + val _namespace = Namespace.empty for ((name, elt) <- elements) { elt.setRef(this, _namespace.name(name)) } } diff --git a/chiselFrontend/src/main/scala/chisel3/core/Module.scala b/chiselFrontend/src/main/scala/chisel3/core/Module.scala index ca7c8abd..de13c078 100644 --- a/chiselFrontend/src/main/scala/chisel3/core/Module.scala +++ b/chiselFrontend/src/main/scala/chisel3/core/Module.scala @@ -113,7 +113,8 @@ extends HasId { Port(iodef) } - private[core] val _namespace = Builder.globalNamespace.child + // Fresh Namespace because in Firrtl, Modules namespaces are disjoint with the global namespace + private[core] val _namespace = Namespace.empty private[chisel3] val _commands = ArrayBuffer[Command]() private[core] val _ids = ArrayBuffer[HasId]() Builder.currentModule = Some(this) @@ -210,7 +211,7 @@ extends HasId { // For Module instances we haven't named, suggest the name of the Module _ids foreach { - case m: Module => m.suggestName(m.name) + case m: Module => m.suggestName(m.desiredName) case _ => } diff --git a/chiselFrontend/src/main/scala/chisel3/internal/Builder.scala b/chiselFrontend/src/main/scala/chisel3/internal/Builder.scala index 5d841941..d8202da6 100644 --- a/chiselFrontend/src/main/scala/chisel3/internal/Builder.scala +++ b/chiselFrontend/src/main/scala/chisel3/internal/Builder.scala @@ -9,7 +9,7 @@ import chisel3._ import core._ import firrtl._ -private[chisel3] class Namespace(parent: Option[Namespace], keywords: Set[String]) { +private[chisel3] class Namespace(keywords: Set[String]) { private val names = collection.mutable.HashMap[String, Long]() for (keyword <- keywords) names(keyword) = 1 @@ -29,9 +29,7 @@ private[chisel3] class Namespace(parent: Option[Namespace], keywords: Set[String if (res.isEmpty || !legalStart(res.head)) s"_$res" else res } - def contains(elem: String): Boolean = { - names.contains(elem) || parent.map(_ contains elem).getOrElse(false) - } + def contains(elem: String): Boolean = names.contains(elem) def name(elem: String): String = { val sanitized = sanitize(elem) @@ -42,9 +40,11 @@ private[chisel3] class Namespace(parent: Option[Namespace], keywords: Set[String sanitized } } +} - def child(kws: Set[String]): Namespace = new Namespace(Some(this), kws) - def child: Namespace = child(Set()) +private[chisel3] object Namespace { + /** Constructs an empty Namespace */ + def empty: Namespace = new Namespace(Set.empty[String]) } private[chisel3] class IdGen { @@ -143,7 +143,7 @@ private[chisel3] trait HasId extends InstanceId { private[chisel3] class DynamicContext() { val idGen = new IdGen - val globalNamespace = new Namespace(None, Set()) + val globalNamespace = Namespace.empty val components = ArrayBuffer[Component]() val annotations = ArrayBuffer[ChiselAnnotation]() var currentModule: Option[Module] = None diff --git a/src/test/scala/chiselTests/DedupSpec.scala b/src/test/scala/chiselTests/DedupSpec.scala new file mode 100644 index 00000000..b8fe075e --- /dev/null +++ b/src/test/scala/chiselTests/DedupSpec.scala @@ -0,0 +1,57 @@ +// See LICENSE for license details. + +package chiselTests + +import chisel3._ +import chisel3.util._ + +class DedupIO extends Bundle { + val in = Flipped(Decoupled(UInt(32.W))) + val out = Decoupled(UInt(32.W)) +} + +class DedupQueues(n: Int) extends Module { + require(n > 0) + val io = IO(new DedupIO) + val queues = Seq.fill(n)(Module(new Queue(UInt(32.W), 4))) + var port = io.in + for (q <- queues) { + q.io.enq <> port + port = q.io.deq + } + io.out <> port +} + +/* This module creates a Queue in a nested function (such that it is not named via reflection). The + * default naming for instances prior to #470 caused otherwise identical instantiations of this + * module to have different instance names for the queues which prevented deduplication. + * NestedDedup instantiates this module twice to ensure it is deduplicated properly. + */ +class DedupSubModule extends Module { + val io = IO(new DedupIO) + io.out <> Queue(io.in, 4) +} + +class NestedDedup extends Module { + val io = IO(new DedupIO) + val inst0 = Module(new DedupSubModule) + val inst1 = Module(new DedupSubModule) + inst0.io.in <> io.in + inst1.io.in <> inst0.io.out + io.out <> inst1.io.out +} + +class DedupSpec extends ChiselFlatSpec { + private val ModuleRegex = """\s*module\s+(\w+)\b.*""".r + def countModules(verilog: String): Int = + (verilog split "\n" collect { case ModuleRegex(name) => name }).size + + "Deduplication" should "occur" in { + assert(countModules(compile { new DedupQueues(4) }) === 2) + } + + it should "properly dedup modules with deduped submodules" in { + assert(countModules(compile { new NestedDedup }) === 3) + } +} + |