Bugfix - definition name index skipping with D/I (#2249)

* Bugfix - definition name index skipping with D/I

* Add tests to DefinitionSpec

* Add failing test

* Fix failing test

* Update core/src/main/scala/chisel3/internal/Builder.scala

Co-authored-by: Jack Koenig <jack.koenig3@gmail.com>

* whitespace

* revert package private val

Co-authored-by: Jack Koenig <jack.koenig3@gmail.com>
Co-authored-by: Jack Koenig <koenig@sifive.com>

5 files changed, 88 insertions, 20 deletions

diff --git a/core/src/main/scala/chisel3/experimental/hierarchy/Definition.scala b/core/src/main/scala/chisel3/experimental/hierarchy/Definition.scala
index 2ac61807..c7b51072 100644
--- a/core/src/main/scala/chisel3/experimental/hierarchy/Definition.scala
+++ b/core/src/main/scala/chisel3/experimental/hierarchy/Definition.scala
@@ -86,7 +86,7 @@ object Definition extends SourceInfoDoc {
     val dynamicContext = new DynamicContext(Nil)
     Builder.globalNamespace.copyTo(dynamicContext.globalNamespace)
     dynamicContext.inDefinition = true
-    val (ir, module) = Builder.build(Module(proto), dynamicContext)
+    val (ir, module) = Builder.build(Module(proto), dynamicContext, false)
     Builder.components ++= ir.components
     Builder.annotations ++= ir.annotations
     module._circuit = Builder.currentModule
diff --git a/core/src/main/scala/chisel3/internal/Builder.scala b/core/src/main/scala/chisel3/internal/Builder.scala
index 55f89ae7..966e60d6 100644
--- a/core/src/main/scala/chisel3/internal/Builder.scala
+++ b/core/src/main/scala/chisel3/internal/Builder.scala
@@ -740,14 +740,16 @@ private[chisel3] object Builder extends LazyLogging {
     renames
   }
 
-  private [chisel3] def build[T <: BaseModule](f: => T, dynamicContext: DynamicContext): (Circuit, T) = {
+  private[chisel3] def build[T <: BaseModule](f: => T, dynamicContext: DynamicContext, forceModName: Boolean = true): (Circuit, T) = {
     dynamicContextVar.withValue(Some(dynamicContext)) {
       ViewParent // Must initialize the singleton in a Builder context or weird things can happen
                  // in tiny designs/testcases that never access anything in chisel3.internal
       checkScalaVersion()
       logger.info("Elaborating design...")
       val mod = f
-      mod.forceName(None, mod.name, globalNamespace)
+      if (forceModName) { // This avoids definition name index skipping with D/I
+        mod.forceName(None, mod.name, globalNamespace)
+      }
       errors.checkpoint(logger)
       logger.info("Done elaborating.")
 
diff --git a/src/test/scala/chiselTests/experimental/hierarchy/DefinitionSpec.scala b/src/test/scala/chiselTests/experimental/hierarchy/DefinitionSpec.scala
index 4eb77c8a..f33f7869 100644
--- a/src/test/scala/chiselTests/experimental/hierarchy/DefinitionSpec.scala
+++ b/src/test/scala/chiselTests/experimental/hierarchy/DefinitionSpec.scala
@@ -40,6 +40,44 @@ class DefinitionSpec extends ChiselFunSpec with Utils {
       val (chirrtl, _) = getFirrtlAndAnnos(new Top)
       chirrtl.serialize should include ("inst i0 of HasUninferredReset")
     }
+    it("0.3: module names of repeated definition should be sequential") {
+      class Top extends Module {
+        val k = Module(new AddTwoParameterized(4, (x: Int) => Seq.tabulate(x){j =>
+          val addOneDef = Definition(new AddOneParameterized(x+j))
+          val addOne = Instance(addOneDef)
+          addOne
+        }))
+      }
+      val (chirrtl, _) = getFirrtlAndAnnos(new Top)
+      chirrtl.serialize should include ("module AddOneParameterized :")
+      chirrtl.serialize should include ("module AddOneParameterized_1 :")
+      chirrtl.serialize should include ("module AddOneParameterized_2 :")
+      chirrtl.serialize should include ("module AddOneParameterized_3 :")
+    }
+    it("0.4: multiple instantiations should have sequential names") {
+      class Top extends Module {
+        val addOneDef = Definition(new AddOneParameterized(4))
+        val addOne = Instance(addOneDef)
+        val otherAddOne = Module(new AddOneParameterized(4))
+      }
+      val (chirrtl, _) = getFirrtlAndAnnos(new Top)
+      chirrtl.serialize should include ("module AddOneParameterized :")
+      chirrtl.serialize should include ("module AddOneParameterized_1 :")
+    }
+    it("0.5: nested definitions should have sequential names") {
+      class Top extends Module {
+        val k = Module(new AddTwoWithNested(4, (x: Int) => Seq.tabulate(x){j =>
+          val addOneDef = Definition(new AddOneWithNested(x+j))
+          val addOne = Instance(addOneDef)
+          addOne
+        }))
+      }
+      val (chirrtl, _) = getFirrtlAndAnnos(new Top)
+      chirrtl.serialize should include ("module AddOneWithNested :")
+      chirrtl.serialize should include ("module AddOneWithNested_1 :")
+      chirrtl.serialize should include ("module AddOneWithNested_2 :")
+      chirrtl.serialize should include ("module AddOneWithNested_3 :")
+    }
   }
   describe("1: Annotations on definitions in same chisel compilation") {
     it("1.0: should work on a single definition, annotating the definition") {
@@ -97,7 +135,7 @@ class DefinitionSpec extends ChiselFunSpec with Utils {
         mark(definition.i1, "i0.i1")
       }
       val (_, annos) = getFirrtlAndAnnos(new Top)
-      annos should contain(MarkAnnotation("~Top|AddTwoMixedModules/i1:AddOne_2".it, "i0.i1"))
+      annos should contain(MarkAnnotation("~Top|AddTwoMixedModules/i1:AddOne_1".it, "i0.i1"))
     }
     // Can you define an instantiable container? I think not.
     // Instead, we can test the instantiable container in a definition
@@ -322,7 +360,7 @@ class DefinitionSpec extends ChiselFunSpec with Utils {
         amark(i.i1.in, "blah")
       }
       val (_, annos) = getFirrtlAndAnnos(new Top)
-      annos should contain(MarkAnnotation("~Top|AddTwoMixedModules/i1:AddOne_2>in".rt, "blah"))
+      annos should contain(MarkAnnotation("~Top|AddTwoMixedModules/i1:AddOne_1>in".rt, "blah"))
     }
     it("5.3: toAbsoluteTarget on a submodule's data, in an aggregate, within a definition") {
       class Top() extends Module {
@@ -330,7 +368,7 @@ class DefinitionSpec extends ChiselFunSpec with Utils {
         amark(i.i1.x.head, "blah")
       }
       val (_, annos) = getFirrtlAndAnnos(new Top)
-      annos should contain(MarkAnnotation("~Top|InstantiatesHasVec/i1:HasVec_2>x[0]".rt, "blah"))
+      annos should contain(MarkAnnotation("~Top|InstantiatesHasVec/i1:HasVec_1>x[0]".rt, "blah"))
     }
   }
   describe("6: @instantiable traits should work as expected") {
diff --git a/src/test/scala/chiselTests/experimental/hierarchy/Examples.scala b/src/test/scala/chiselTests/experimental/hierarchy/Examples.scala
index d8ae7322..c0f504ff 100644
--- a/src/test/scala/chiselTests/experimental/hierarchy/Examples.scala
+++ b/src/test/scala/chiselTests/experimental/hierarchy/Examples.scala
@@ -36,6 +36,19 @@ object Examples {
     out := innerWire
   }
   @instantiable
+  class AddOneParameterized(width: Int) extends Module {
+    @public val in  = IO(Input(UInt(width.W)))
+    @public val out = IO(Output(UInt(width.W)))
+    out := in + 1.U
+  }
+  class AddOneWithNested(width: Int) extends Module {
+    @public val in  = IO(Input(UInt(width.W)))
+    @public val out = IO(Output(UInt(width.W)))
+    val addOneDef = Seq.fill(3)(Definition(new AddOne))
+    out := in + 1.U
+  }
+
+  @instantiable
   class AddTwo extends Module {
     @public val in  = IO(Input(UInt(32.W)))
     @public val out = IO(Output(UInt(32.W)))
@@ -57,6 +70,21 @@ object Examples {
     i1.in := i0.out
     out := i1.out
   }
+  @instantiable
+  class AddTwoParameterized(width: Int, makeParameterizedOnes: Int => Seq[Instance[AddOneParameterized]]) extends Module {
+    val in  = IO(Input(UInt(width.W)))
+    val out = IO(Output(UInt(width.W)))
+    val addOnes = makeParameterizedOnes(width)
+    addOnes.head.in := in
+    out := addOnes.last.out
+    addOnes.zip(addOnes.tail).foreach{ case (head, tail) => tail.in := head.out}
+  }
+  @instantiable
+  class AddTwoWithNested(width: Int, makeParameterizedOnes: Int => Seq[Instance[AddOneWithNested]]) extends Module {
+    val in  = IO(Input(UInt(width.W)))
+    val out = IO(Output(UInt(width.W)))
+    val addOnes = makeParameterizedOnes(width)
+  }
 
   @instantiable
   class AddFour extends Module {
diff --git a/src/test/scala/chiselTests/experimental/hierarchy/InstanceSpec.scala b/src/test/scala/chiselTests/experimental/hierarchy/InstanceSpec.scala
index 83084468..929e3875 100644
--- a/src/test/scala/chiselTests/experimental/hierarchy/InstanceSpec.scala
+++ b/src/test/scala/chiselTests/experimental/hierarchy/InstanceSpec.scala
@@ -89,7 +89,7 @@ class InstanceSpec extends ChiselFunSpec with Utils {
         mark(i0.i1, "i0.i1")
       }
       val (_, annos) = getFirrtlAndAnnos(new Top)
-      annos should contain (MarkAnnotation("~Top|Top/i0:AddTwoMixedModules/i1:AddOne_2".it, "i0.i1"))
+      annos should contain (MarkAnnotation("~Top|Top/i0:AddTwoMixedModules/i1:AddOne_1".it, "i0.i1"))
     }
     it("1.5: should work on an instantiable container, annotating a wire") {
       class Top extends Module {
@@ -362,7 +362,7 @@ class InstanceSpec extends ChiselFunSpec with Utils {
         amark(i.i1.in, "blah")
       }
       val (_, annos) = getFirrtlAndAnnos(new Top)
-      annos should contain(MarkAnnotation("~Top|Top/i:AddTwoMixedModules/i1:AddOne_2>in".rt, "blah"))
+      annos should contain(MarkAnnotation("~Top|Top/i:AddTwoMixedModules/i1:AddOne_1>in".rt, "blah"))
     }
     it("5.3: toAbsoluteTarget on a submodule's data, in an aggregate, within an instance") {
       class Top() extends Module {
@@ -370,7 +370,7 @@ class InstanceSpec extends ChiselFunSpec with Utils {
         amark(i.i1.x.head, "blah")
       }
       val (_, annos) = getFirrtlAndAnnos(new Top)
-      annos should contain(MarkAnnotation("~Top|Top/i:InstantiatesHasVec/i1:HasVec_2>x[0]".rt, "blah"))
+      annos should contain(MarkAnnotation("~Top|Top/i:InstantiatesHasVec/i1:HasVec_1>x[0]".rt, "blah"))
     }
     it("5.4: toAbsoluteTarget on a submodule's data, in an aggregate, within an instance, ILit") {
       class MyBundle extends Bundle { val x = UInt(3.W) }
@@ -388,7 +388,7 @@ class InstanceSpec extends ChiselFunSpec with Utils {
         amark(i.i1.x.head.x, "blah")
       }
       val (_, annos) = getFirrtlAndAnnos(new Top)
-      annos should contain(MarkAnnotation("~Top|Top/i:InstantiatesHasVec/i1:HasVec_2>x[0].x".rt, "blah"))
+      annos should contain(MarkAnnotation("~Top|Top/i:InstantiatesHasVec/i1:HasVec_1>x[0].x".rt, "blah"))
     }
     it("5.5: toAbsoluteTarget on a subinstance") {
       class Top() extends Module {
@@ -761,7 +761,7 @@ class InstanceSpec extends ChiselFunSpec with Utils {
         val targets = aop.Select.instancesOf[AddOne](m.toDefinition).map { i: Instance[AddOne] => i.toTarget }
         targets should be (Seq(
           "~AddTwoMixedModules|AddTwoMixedModules/i0:AddOne".it,
-          "~AddTwoMixedModules|AddTwoMixedModules/i1:AddOne_2".it,
+          "~AddTwoMixedModules|AddTwoMixedModules/i1:AddOne_1".it,
         ))
       })
       getFirrtlAndAnnos(new AddTwoMixedModules, Seq(aspect))
@@ -773,11 +773,11 @@ class InstanceSpec extends ChiselFunSpec with Utils {
         val rel = insts.map { i: Instance[BaseModule] => i.toTarget }
         abs should be (Seq(
           "~AddTwoMixedModules|AddTwoMixedModules/i0:AddOne".it,
-          "~AddTwoMixedModules|AddTwoMixedModules/i1:AddOne_2".it,
+          "~AddTwoMixedModules|AddTwoMixedModules/i1:AddOne_1".it,
         ))
         rel should be (Seq(
           "~AddTwoMixedModules|AddTwoMixedModules/i0:AddOne".it,
-          "~AddTwoMixedModules|AddTwoMixedModules/i1:AddOne_2".it,
+          "~AddTwoMixedModules|AddTwoMixedModules/i1:AddOne_1".it,
         ))
       })
       getFirrtlAndAnnos(new AddTwoMixedModules, Seq(aspect))
@@ -789,15 +789,15 @@ class InstanceSpec extends ChiselFunSpec with Utils {
         val rel = insts.map { i: Instance[AddOne] => i.in.toTarget }
         rel should be (Seq(
           "~AddFour|AddFour/i0:AddTwoMixedModules/i0:AddOne>in".rt,
-          "~AddFour|AddFour/i0:AddTwoMixedModules/i1:AddOne_2>in".rt,
+          "~AddFour|AddFour/i0:AddTwoMixedModules/i1:AddOne_1>in".rt,
           "~AddFour|AddFour/i1:AddTwoMixedModules/i0:AddOne>in".rt,
-          "~AddFour|AddFour/i1:AddTwoMixedModules/i1:AddOne_2>in".rt,
+          "~AddFour|AddFour/i1:AddTwoMixedModules/i1:AddOne_1>in".rt,
         ))
         abs should be (Seq(
           "~AddFour|AddFour/i0:AddTwoMixedModules/i0:AddOne>in".rt,
-          "~AddFour|AddFour/i0:AddTwoMixedModules/i1:AddOne_2>in".rt,
+          "~AddFour|AddFour/i0:AddTwoMixedModules/i1:AddOne_1>in".rt,
           "~AddFour|AddFour/i1:AddTwoMixedModules/i0:AddOne>in".rt,
-          "~AddFour|AddFour/i1:AddTwoMixedModules/i1:AddOne_2>in".rt,
+          "~AddFour|AddFour/i1:AddTwoMixedModules/i1:AddOne_1>in".rt,
         ))
       })
       getFirrtlAndAnnos(new AddFour, Seq(aspect))
@@ -807,7 +807,7 @@ class InstanceSpec extends ChiselFunSpec with Utils {
         val targets = aop.Select.definitionsOf[AddOne](m.toDefinition).map { i: Definition[AddOne] => i.in.toTarget }
         targets should be (Seq(
           "~AddTwoMixedModules|AddOne>in".rt,
-          "~AddTwoMixedModules|AddOne_2>in".rt,
+          "~AddTwoMixedModules|AddOne_1>in".rt,
         ))
       })
       getFirrtlAndAnnos(new AddTwoMixedModules, Seq(aspect))
@@ -817,7 +817,7 @@ class InstanceSpec extends ChiselFunSpec with Utils {
         val targets = aop.Select.definitionsIn(m.toDefinition).map { i: Definition[BaseModule] => i.toTarget }
         targets should be (Seq(
           "~AddTwoMixedModules|AddOne".mt,
-          "~AddTwoMixedModules|AddOne_2".mt,
+          "~AddTwoMixedModules|AddOne_1".mt,
         ))
       })
       getFirrtlAndAnnos(new AddTwoMixedModules, Seq(aspect))
@@ -827,7 +827,7 @@ class InstanceSpec extends ChiselFunSpec with Utils {
         val targets = aop.Select.allDefinitionsOf[AddOne](m.toDefinition).map { i: Definition[AddOne] => i.in.toTarget }
         targets should be (Seq(
           "~AddFour|AddOne>in".rt,
-          "~AddFour|AddOne_2>in".rt,
+          "~AddFour|AddOne_1>in".rt,
         ))
       })
       getFirrtlAndAnnos(new AddFour, Seq(aspect))