From 3927f5e14051f56c4f5833e848a151b060650ddb Mon Sep 17 00:00:00 2001 From: Girish Pai Date: Tue, 21 Jun 2022 05:53:09 -0700 Subject: [PATCH 01/11] Adding support for passing name through ImportDutDefinition anno --- .../hierarchy/core/Definition.scala | 2 +- .../experimental/hierarchy/core/Instance.scala | 4 +++- .../main/scala/chisel3/internal/Builder.scala | 17 +++++++++++++---- 3 files changed, 17 insertions(+), 6 deletions(-) diff --git a/core/src/main/scala/chisel3/experimental/hierarchy/core/Definition.scala b/core/src/main/scala/chisel3/experimental/hierarchy/core/Definition.scala index 867330d2297..05abaeeb0f8 100644 --- a/core/src/main/scala/chisel3/experimental/hierarchy/core/Definition.scala +++ b/core/src/main/scala/chisel3/experimental/hierarchy/core/Definition.scala @@ -119,5 +119,5 @@ object Definition extends SourceInfoDoc { /** Stores a [[Definition]] that is imported so that its Instances can be * compiled separately. */ -case class ImportDefinitionAnnotation[T <: BaseModule with IsInstantiable](definition: Definition[T]) +case class ImportDefinitionAnnotation[T <: BaseModule with IsInstantiable](definition: Definition[T], name : Option[String] = None) extends NoTargetAnnotation diff --git a/core/src/main/scala/chisel3/experimental/hierarchy/core/Instance.scala b/core/src/main/scala/chisel3/experimental/hierarchy/core/Instance.scala index ef7ad22e3d6..901366357b1 100644 --- a/core/src/main/scala/chisel3/experimental/hierarchy/core/Instance.scala +++ b/core/src/main/scala/chisel3/experimental/hierarchy/core/Instance.scala @@ -10,6 +10,7 @@ import chisel3.internal.sourceinfo.{InstanceTransform, SourceInfo} import chisel3.experimental.{BaseModule, ExtModule} import chisel3.internal.firrtl.{Component, DefBlackBox, DefModule, Port} import firrtl.annotations.IsModule +import chisel3.internal.throwException /** User-facing Instance type. * Represents a unique instance of type [[A]] which are marked as @instantiable @@ -118,8 +119,9 @@ object Instance extends SourceInfoDoc { if (existingMod.isEmpty) { // Add a Definition that will get emitted as an ExtModule so that FIRRTL // does not complain about a missing element + val extModName = Builder.importDefinitionMap.getOrElse(definition.proto.name,throwException("Imported Definition information not found - possibly forgot to add ImportDefinition annotation?")) class EmptyExtModule extends ExtModule { - override def desiredName: String = definition.proto.name + override def desiredName: String = extModName override def generateComponent(): Option[Component] = { require(!_closed, s"Can't generate $desiredName module more than once") _closed = true diff --git a/core/src/main/scala/chisel3/internal/Builder.scala b/core/src/main/scala/chisel3/internal/Builder.scala index 7ffd72245c2..6dd2a7c982b 100644 --- a/core/src/main/scala/chisel3/internal/Builder.scala +++ b/core/src/main/scala/chisel3/internal/Builder.scala @@ -16,6 +16,7 @@ import chisel3.experimental.dataview.{reify, reifySingleData} import chisel3.internal.Builder.Prefix import logger.LazyLogging +import scala.collection.mutable import scala.collection.mutable private[chisel3] class Namespace(keywords: Set[String]) { @@ -335,10 +336,17 @@ private[chisel3] class DynamicContext( val throwOnFirstError: Boolean) { val importDefinitionAnnos = annotationSeq.collect { case a: ImportDefinitionAnnotation[_] => a } + // Map holding the actual names of extModules + val importDefinitionMap : mutable.Map[String,String] = mutable.Map.empty + + // Pick the definition name by default in case not passed through annotation. + importDefinitionAnnos.foreach {a => importDefinitionMap += ((a.definition.proto.name,a.name.getOrElse(a.definition.proto.name)))} + // Ensure there are no repeated names for imported Definitions - val importDefinitionNames = importDefinitionAnnos.map { a => a.definition.proto.name } - if (importDefinitionNames.distinct.length < importDefinitionNames.length) { - val duplicates = importDefinitionNames.diff(importDefinitionNames.distinct).mkString(", ") + val importDistinctDefinitionNames = importDefinitionMap.keys.toSeq + val importAllDefinitionNames = importDefinitionAnnos.map { a => a.definition.proto.name } + if (importDistinctDefinitionNames.length < importAllDefinitionNames.length) { + val duplicates = importAllDefinitionNames.diff(importDistinctDefinitionNames).mkString(", ") throwException(s"Expected distinct imported Definition names but found duplicates for: $duplicates") } @@ -347,7 +355,7 @@ private[chisel3] class DynamicContext( // Ensure imported Definitions emit as ExtModules with the correct name so // that instantiations will also use the correct name and prevent any name // conflicts with Modules/Definitions in this elaboration - importDefinitionNames.foreach { importDefName => + importAllDefinitionNames.foreach { importDefName => globalNamespace.name(importDefName) } @@ -420,6 +428,7 @@ private[chisel3] object Builder extends LazyLogging { def annotations: ArrayBuffer[ChiselAnnotation] = dynamicContext.annotations def annotationSeq: AnnotationSeq = dynamicContext.annotationSeq def namingStack: NamingStack = dynamicContext.namingStack + def importDefinitionMap: mutable.Map[String,String] = dynamicContext.importDefinitionMap def unnamedViews: ArrayBuffer[Data] = dynamicContext.unnamedViews def viewNamespace: Namespace = chiselContext.get.viewNamespace From a5686dcf599d8b41704c18f84c9c5f8f64450745 Mon Sep 17 00:00:00 2001 From: Girish Pai Date: Tue, 21 Jun 2022 06:37:24 -0700 Subject: [PATCH 02/11] Splitting into ProtoName and ExtModName and adding associated checks --- .../main/scala/chisel3/internal/Builder.scala | 19 +++++++++++++------ 1 file changed, 13 insertions(+), 6 deletions(-) diff --git a/core/src/main/scala/chisel3/internal/Builder.scala b/core/src/main/scala/chisel3/internal/Builder.scala index 6dd2a7c982b..c56969e9c93 100644 --- a/core/src/main/scala/chisel3/internal/Builder.scala +++ b/core/src/main/scala/chisel3/internal/Builder.scala @@ -342,11 +342,18 @@ private[chisel3] class DynamicContext( // Pick the definition name by default in case not passed through annotation. importDefinitionAnnos.foreach {a => importDefinitionMap += ((a.definition.proto.name,a.name.getOrElse(a.definition.proto.name)))} - // Ensure there are no repeated names for imported Definitions - val importDistinctDefinitionNames = importDefinitionMap.keys.toSeq - val importAllDefinitionNames = importDefinitionAnnos.map { a => a.definition.proto.name } - if (importDistinctDefinitionNames.length < importAllDefinitionNames.length) { - val duplicates = importAllDefinitionNames.diff(importDistinctDefinitionNames).mkString(", ") + // Ensure there are no repeated names for imported Definitions - both Proto Names as well as ExtMod Names + val importAllDefinitionProtoNames = importDefinitionAnnos.map {a => a.definition.proto.name} + val importDistinctDefinitionProtoNames = importDefinitionMap.keys.toSeq + val importAllDefinitionExtModNames = importDefinitionMap.values.toSeq + val importDistinctDefinitionExtModNames = importAllDefinitionExtModNames.distinct + + if (importDistinctDefinitionProtoNames.length < importAllDefinitionProtoNames.length) { + val duplicates = importAllDefinitionProtoNames.diff(importDistinctDefinitionProtoNames).mkString(", ") + throwException(s"Expected distinct imported Definition names but found duplicates for: $duplicates") + } + if (importDistinctDefinitionExtModNames.length < importAllDefinitionExtModNames.length) { + val duplicates = importAllDefinitionExtModNames.diff(importDistinctDefinitionExtModNames).mkString(", ") throwException(s"Expected distinct imported Definition names but found duplicates for: $duplicates") } @@ -355,7 +362,7 @@ private[chisel3] class DynamicContext( // Ensure imported Definitions emit as ExtModules with the correct name so // that instantiations will also use the correct name and prevent any name // conflicts with Modules/Definitions in this elaboration - importAllDefinitionNames.foreach { importDefName => + importAllDefinitionExtModNames.foreach { importDefName => globalNamespace.name(importDefName) } From 9659dd3d9a59fbac7f30ebf688fd34c4c79a50ba Mon Sep 17 00:00:00 2001 From: Girish Pai Date: Tue, 21 Jun 2022 07:29:18 -0700 Subject: [PATCH 03/11] Fixing bug with namespaces --- core/src/main/scala/chisel3/internal/Builder.scala | 3 +++ 1 file changed, 3 insertions(+) diff --git a/core/src/main/scala/chisel3/internal/Builder.scala b/core/src/main/scala/chisel3/internal/Builder.scala index c56969e9c93..9a5b313c76d 100644 --- a/core/src/main/scala/chisel3/internal/Builder.scala +++ b/core/src/main/scala/chisel3/internal/Builder.scala @@ -362,6 +362,9 @@ private[chisel3] class DynamicContext( // Ensure imported Definitions emit as ExtModules with the correct name so // that instantiations will also use the correct name and prevent any name // conflicts with Modules/Definitions in this elaboration + importAllDefinitionProtoNames.foreach { importDefName => + globalNamespace.name(importDefName) + } importAllDefinitionExtModNames.foreach { importDefName => globalNamespace.name(importDefName) } From 8162b448e9d5fbd0f50dc271c68e890a0cbded0e Mon Sep 17 00:00:00 2001 From: Girish Pai Date: Tue, 21 Jun 2022 07:43:43 -0700 Subject: [PATCH 04/11] Properly add protoName,extModName to global namespace --- core/src/main/scala/chisel3/internal/Builder.scala | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/core/src/main/scala/chisel3/internal/Builder.scala b/core/src/main/scala/chisel3/internal/Builder.scala index 9a5b313c76d..3148da3618d 100644 --- a/core/src/main/scala/chisel3/internal/Builder.scala +++ b/core/src/main/scala/chisel3/internal/Builder.scala @@ -362,11 +362,14 @@ private[chisel3] class DynamicContext( // Ensure imported Definitions emit as ExtModules with the correct name so // that instantiations will also use the correct name and prevent any name // conflicts with Modules/Definitions in this elaboration - importAllDefinitionProtoNames.foreach { importDefName => - globalNamespace.name(importDefName) - } - importAllDefinitionExtModNames.foreach { importDefName => - globalNamespace.name(importDefName) + + importAllDefinitionProtoNames.zip(importAllDefinitionExtModNames).foreach { case ((protoName,extModName)) => + globalNamespace.name(protoName) + + // Only add the extModName to Namespace if it is different from definition proto name + if(protoName != extModName) { + globalNamespace.name(extModName) + } } val components = ArrayBuffer[Component]() From f0a485f51314f4890bc7549c8b60e5f329e04bac Mon Sep 17 00:00:00 2001 From: Girish Pai Date: Tue, 21 Jun 2022 07:46:00 -0700 Subject: [PATCH 05/11] Adding one test for extMod - vanilla case --- .../hierarchy/SeparateElaborationSpec.scala | 32 +++++++++++++++++++ 1 file changed, 32 insertions(+) diff --git a/src/test/scala/chiselTests/experimental/hierarchy/SeparateElaborationSpec.scala b/src/test/scala/chiselTests/experimental/hierarchy/SeparateElaborationSpec.scala index e67ed0b31c4..df1baa8d7ad 100644 --- a/src/test/scala/chiselTests/experimental/hierarchy/SeparateElaborationSpec.scala +++ b/src/test/scala/chiselTests/experimental/hierarchy/SeparateElaborationSpec.scala @@ -354,5 +354,37 @@ class SeparateElaborationSpec extends ChiselFunSpec with Utils { "Expected distinct imported Definition names but found duplicates for: AddOne" ) } + + describe("(4): With ExtMod Names") { + it("should pick correct ExtMod names when passed") { + val testDir = createTestDirectory(this.getClass.getSimpleName).toString + + val dutDef = getAddOneDefinition(testDir) + + class Testbench(defn: Definition[AddOne]) extends Module { + val mod = Module(new AddOne) + val inst = Instance(defn) + + // Tie inputs to a value so ChiselStage does not complain + mod.in := 0.U + inst.in := 0.U + dontTouch(mod.out) + } + (new ChiselStage).run( + Seq( + ChiselGeneratorAnnotation(() => new Testbench(dutDef)), + TargetDirAnnotation(testDir), + ImportDefinitionAnnotation(dutDef,Some("CustomPrefix_AddOne_CustomSuffix")) + ) + ) + + val tb_rtl = Source.fromFile(s"$testDir/Testbench.v").getLines.mkString + + tb_rtl should include("module AddOne_1(") + tb_rtl should include("AddOne_1 mod (") + (tb_rtl should not).include("module AddOne(") + tb_rtl should include("CustomPrefix_AddOne_CustomSuffix inst (") + } + } } From c5d603ed5d77cd7255f9a553debd65b71e941b59 Mon Sep 17 00:00:00 2001 From: Girish Pai Date: Tue, 21 Jun 2022 07:50:52 -0700 Subject: [PATCH 06/11] Ran scalafmt --- .../hierarchy/core/Definition.scala | 4 +- .../hierarchy/core/Instance.scala | 7 ++- .../main/scala/chisel3/internal/Builder.scala | 43 ++++++++++--------- .../hierarchy/SeparateElaborationSpec.scala | 6 +-- 4 files changed, 35 insertions(+), 25 deletions(-) diff --git a/core/src/main/scala/chisel3/experimental/hierarchy/core/Definition.scala b/core/src/main/scala/chisel3/experimental/hierarchy/core/Definition.scala index 05abaeeb0f8..16e5bd03159 100644 --- a/core/src/main/scala/chisel3/experimental/hierarchy/core/Definition.scala +++ b/core/src/main/scala/chisel3/experimental/hierarchy/core/Definition.scala @@ -119,5 +119,7 @@ object Definition extends SourceInfoDoc { /** Stores a [[Definition]] that is imported so that its Instances can be * compiled separately. */ -case class ImportDefinitionAnnotation[T <: BaseModule with IsInstantiable](definition: Definition[T], name : Option[String] = None) +case class ImportDefinitionAnnotation[T <: BaseModule with IsInstantiable]( + definition: Definition[T], + name: Option[String] = None) extends NoTargetAnnotation diff --git a/core/src/main/scala/chisel3/experimental/hierarchy/core/Instance.scala b/core/src/main/scala/chisel3/experimental/hierarchy/core/Instance.scala index 901366357b1..1fdf58322ae 100644 --- a/core/src/main/scala/chisel3/experimental/hierarchy/core/Instance.scala +++ b/core/src/main/scala/chisel3/experimental/hierarchy/core/Instance.scala @@ -119,7 +119,12 @@ object Instance extends SourceInfoDoc { if (existingMod.isEmpty) { // Add a Definition that will get emitted as an ExtModule so that FIRRTL // does not complain about a missing element - val extModName = Builder.importDefinitionMap.getOrElse(definition.proto.name,throwException("Imported Definition information not found - possibly forgot to add ImportDefinition annotation?")) + val extModName = Builder.importDefinitionMap.getOrElse( + definition.proto.name, + throwException( + "Imported Definition information not found - possibly forgot to add ImportDefinition annotation?" + ) + ) class EmptyExtModule extends ExtModule { override def desiredName: String = extModName override def generateComponent(): Option[Component] = { diff --git a/core/src/main/scala/chisel3/internal/Builder.scala b/core/src/main/scala/chisel3/internal/Builder.scala index 3148da3618d..68991e57240 100644 --- a/core/src/main/scala/chisel3/internal/Builder.scala +++ b/core/src/main/scala/chisel3/internal/Builder.scala @@ -336,18 +336,20 @@ private[chisel3] class DynamicContext( val throwOnFirstError: Boolean) { val importDefinitionAnnos = annotationSeq.collect { case a: ImportDefinitionAnnotation[_] => a } - // Map holding the actual names of extModules - val importDefinitionMap : mutable.Map[String,String] = mutable.Map.empty + // Map holding the actual names of extModules + val importDefinitionMap: mutable.Map[String, String] = mutable.Map.empty + + // Pick the definition name by default in case not passed through annotation. + importDefinitionAnnos.foreach { a => + importDefinitionMap += ((a.definition.proto.name, a.name.getOrElse(a.definition.proto.name))) + } - // Pick the definition name by default in case not passed through annotation. - importDefinitionAnnos.foreach {a => importDefinitionMap += ((a.definition.proto.name,a.name.getOrElse(a.definition.proto.name)))} - // Ensure there are no repeated names for imported Definitions - both Proto Names as well as ExtMod Names - val importAllDefinitionProtoNames = importDefinitionAnnos.map {a => a.definition.proto.name} + val importAllDefinitionProtoNames = importDefinitionAnnos.map { a => a.definition.proto.name } val importDistinctDefinitionProtoNames = importDefinitionMap.keys.toSeq val importAllDefinitionExtModNames = importDefinitionMap.values.toSeq val importDistinctDefinitionExtModNames = importAllDefinitionExtModNames.distinct - + if (importDistinctDefinitionProtoNames.length < importAllDefinitionProtoNames.length) { val duplicates = importAllDefinitionProtoNames.diff(importDistinctDefinitionProtoNames).mkString(", ") throwException(s"Expected distinct imported Definition names but found duplicates for: $duplicates") @@ -362,14 +364,15 @@ private[chisel3] class DynamicContext( // Ensure imported Definitions emit as ExtModules with the correct name so // that instantiations will also use the correct name and prevent any name // conflicts with Modules/Definitions in this elaboration - - importAllDefinitionProtoNames.zip(importAllDefinitionExtModNames).foreach { case ((protoName,extModName)) => - globalNamespace.name(protoName) - // Only add the extModName to Namespace if it is different from definition proto name - if(protoName != extModName) { - globalNamespace.name(extModName) - } + importAllDefinitionProtoNames.zip(importAllDefinitionExtModNames).foreach { + case ((protoName, extModName)) => + globalNamespace.name(protoName) + + // Only add the extModName to Namespace if it is different from definition proto name + if (protoName != extModName) { + globalNamespace.name(extModName) + } } val components = ArrayBuffer[Component]() @@ -436,12 +439,12 @@ private[chisel3] object Builder extends LazyLogging { def idGen: IdGen = chiselContext.get.idGen - def globalNamespace: Namespace = dynamicContext.globalNamespace - def components: ArrayBuffer[Component] = dynamicContext.components - def annotations: ArrayBuffer[ChiselAnnotation] = dynamicContext.annotations - def annotationSeq: AnnotationSeq = dynamicContext.annotationSeq - def namingStack: NamingStack = dynamicContext.namingStack - def importDefinitionMap: mutable.Map[String,String] = dynamicContext.importDefinitionMap + def globalNamespace: Namespace = dynamicContext.globalNamespace + def components: ArrayBuffer[Component] = dynamicContext.components + def annotations: ArrayBuffer[ChiselAnnotation] = dynamicContext.annotations + def annotationSeq: AnnotationSeq = dynamicContext.annotationSeq + def namingStack: NamingStack = dynamicContext.namingStack + def importDefinitionMap: mutable.Map[String, String] = dynamicContext.importDefinitionMap def unnamedViews: ArrayBuffer[Data] = dynamicContext.unnamedViews def viewNamespace: Namespace = chiselContext.get.viewNamespace diff --git a/src/test/scala/chiselTests/experimental/hierarchy/SeparateElaborationSpec.scala b/src/test/scala/chiselTests/experimental/hierarchy/SeparateElaborationSpec.scala index df1baa8d7ad..96c58ec9c13 100644 --- a/src/test/scala/chiselTests/experimental/hierarchy/SeparateElaborationSpec.scala +++ b/src/test/scala/chiselTests/experimental/hierarchy/SeparateElaborationSpec.scala @@ -354,7 +354,7 @@ class SeparateElaborationSpec extends ChiselFunSpec with Utils { "Expected distinct imported Definition names but found duplicates for: AddOne" ) } - + describe("(4): With ExtMod Names") { it("should pick correct ExtMod names when passed") { val testDir = createTestDirectory(this.getClass.getSimpleName).toString @@ -375,12 +375,12 @@ class SeparateElaborationSpec extends ChiselFunSpec with Utils { Seq( ChiselGeneratorAnnotation(() => new Testbench(dutDef)), TargetDirAnnotation(testDir), - ImportDefinitionAnnotation(dutDef,Some("CustomPrefix_AddOne_CustomSuffix")) + ImportDefinitionAnnotation(dutDef, Some("CustomPrefix_AddOne_CustomSuffix")) ) ) val tb_rtl = Source.fromFile(s"$testDir/Testbench.v").getLines.mkString - + tb_rtl should include("module AddOne_1(") tb_rtl should include("AddOne_1 mod (") (tb_rtl should not).include("module AddOne(") From d2d1daeb74d7873e0a96836dcf991dd61a2ea676 Mon Sep 17 00:00:00 2001 From: Girish Pai Date: Tue, 21 Jun 2022 11:39:07 -0700 Subject: [PATCH 07/11] Incorporate review feedback --- .../hierarchy/core/Definition.scala | 4 ++-- .../main/scala/chisel3/internal/Builder.scala | 22 +++++-------------- 2 files changed, 8 insertions(+), 18 deletions(-) diff --git a/core/src/main/scala/chisel3/experimental/hierarchy/core/Definition.scala b/core/src/main/scala/chisel3/experimental/hierarchy/core/Definition.scala index 16e5bd03159..01da4bd4600 100644 --- a/core/src/main/scala/chisel3/experimental/hierarchy/core/Definition.scala +++ b/core/src/main/scala/chisel3/experimental/hierarchy/core/Definition.scala @@ -120,6 +120,6 @@ object Definition extends SourceInfoDoc { * compiled separately. */ case class ImportDefinitionAnnotation[T <: BaseModule with IsInstantiable]( - definition: Definition[T], - name: Option[String] = None) + definition: Definition[T], + overrideDefName: Option[String] = None) extends NoTargetAnnotation diff --git a/core/src/main/scala/chisel3/internal/Builder.scala b/core/src/main/scala/chisel3/internal/Builder.scala index 68991e57240..b5fffeabb1d 100644 --- a/core/src/main/scala/chisel3/internal/Builder.scala +++ b/core/src/main/scala/chisel3/internal/Builder.scala @@ -16,7 +16,6 @@ import chisel3.experimental.dataview.{reify, reifySingleData} import chisel3.internal.Builder.Prefix import logger.LazyLogging -import scala.collection.mutable import scala.collection.mutable private[chisel3] class Namespace(keywords: Set[String]) { @@ -337,12 +336,10 @@ private[chisel3] class DynamicContext( val importDefinitionAnnos = annotationSeq.collect { case a: ImportDefinitionAnnotation[_] => a } // Map holding the actual names of extModules - val importDefinitionMap: mutable.Map[String, String] = mutable.Map.empty - // Pick the definition name by default in case not passed through annotation. - importDefinitionAnnos.foreach { a => - importDefinitionMap += ((a.definition.proto.name, a.name.getOrElse(a.definition.proto.name))) - } + val importDefinitionMap = importDefinitionAnnos + .map(a => ((a.definition.proto.name, a.overrideDefName.getOrElse(a.definition.proto.name)))) + .toMap // Ensure there are no repeated names for imported Definitions - both Proto Names as well as ExtMod Names val importAllDefinitionProtoNames = importDefinitionAnnos.map { a => a.definition.proto.name } @@ -364,15 +361,8 @@ private[chisel3] class DynamicContext( // Ensure imported Definitions emit as ExtModules with the correct name so // that instantiations will also use the correct name and prevent any name // conflicts with Modules/Definitions in this elaboration - - importAllDefinitionProtoNames.zip(importAllDefinitionExtModNames).foreach { - case ((protoName, extModName)) => - globalNamespace.name(protoName) - - // Only add the extModName to Namespace if it is different from definition proto name - if (protoName != extModName) { - globalNamespace.name(extModName) - } + (importAllDefinitionProtoNames ++ importAllDefinitionExtModNames).distinct.foreach { + globalNamespace.name(_) } val components = ArrayBuffer[Component]() @@ -444,7 +434,7 @@ private[chisel3] object Builder extends LazyLogging { def annotations: ArrayBuffer[ChiselAnnotation] = dynamicContext.annotations def annotationSeq: AnnotationSeq = dynamicContext.annotationSeq def namingStack: NamingStack = dynamicContext.namingStack - def importDefinitionMap: mutable.Map[String, String] = dynamicContext.importDefinitionMap + def importDefinitionMap: Map[String, String] = dynamicContext.importDefinitionMap def unnamedViews: ArrayBuffer[Data] = dynamicContext.unnamedViews def viewNamespace: Namespace = chiselContext.get.viewNamespace From fed0d603ae3ba3c3f948df3943d920bde3bde8a3 Mon Sep 17 00:00:00 2001 From: Girish Pai Date: Tue, 21 Jun 2022 11:58:25 -0700 Subject: [PATCH 08/11] Adding test --- .../hierarchy/SeparateElaborationSpec.scala | 55 ++++++++++++++++++- 1 file changed, 54 insertions(+), 1 deletion(-) diff --git a/src/test/scala/chiselTests/experimental/hierarchy/SeparateElaborationSpec.scala b/src/test/scala/chiselTests/experimental/hierarchy/SeparateElaborationSpec.scala index 96c58ec9c13..bc3d078c8b7 100644 --- a/src/test/scala/chiselTests/experimental/hierarchy/SeparateElaborationSpec.scala +++ b/src/test/scala/chiselTests/experimental/hierarchy/SeparateElaborationSpec.scala @@ -356,7 +356,7 @@ class SeparateElaborationSpec extends ChiselFunSpec with Utils { } describe("(4): With ExtMod Names") { - it("should pick correct ExtMod names when passed") { + it("(4.a): should pick correct ExtMod names when passed") { val testDir = createTestDirectory(this.getClass.getSimpleName).toString val dutDef = getAddOneDefinition(testDir) @@ -387,4 +387,57 @@ class SeparateElaborationSpec extends ChiselFunSpec with Utils { tb_rtl should include("CustomPrefix_AddOne_CustomSuffix inst (") } } + + it( + "(4.b): should work if a list of imported Definitions is passed between Stages with ExtModName." + ) { + val testDir = createTestDirectory(this.getClass.getSimpleName).toString + + val dutAnnos0 = (new ChiselStage).run( + Seq( + ChiselGeneratorAnnotation(() => new AddOneParameterized(4)), + TargetDirAnnotation(s"$testDir/dutDef0") + ) + ) + val dutDef0 = getDesignAnnotation(dutAnnos0).design.asInstanceOf[AddOneParameterized].toDefinition + + val dutAnnos1 = (new ChiselStage).run( + Seq( + ChiselGeneratorAnnotation(() => new AddOneParameterized(8)), + TargetDirAnnotation(s"$testDir/dutDef1"), + // pass in previously elaborated Definitions + ImportDefinitionAnnotation(dutDef0) + ) + ) + val dutDef1 = getDesignAnnotation(dutAnnos1).design.asInstanceOf[AddOneParameterized].toDefinition + + class Testbench(defn0: Definition[AddOneParameterized], defn1: Definition[AddOneParameterized]) extends Module { + val inst0 = Instance(defn0) + val inst1 = Instance(defn1) + + // Tie inputs to a value so ChiselStage does not complain + inst0.in := 0.U + inst1.in := 0.U + } + + (new ChiselStage).run( + Seq( + ChiselGeneratorAnnotation(() => new Testbench(dutDef0, dutDef1)), + TargetDirAnnotation(testDir), + ImportDefinitionAnnotation(dutDef0,Some("Inst1_Prefix_AddOnePramaterized_Inst1_Suffix")), + ImportDefinitionAnnotation(dutDef1,Some("Inst2_Prefix_AddOnePrameterized_1_Inst2_Suffix")) + ) + ) + + val dutDef0_rtl = Source.fromFile(s"$testDir/dutDef0/AddOneParameterized.v").getLines.mkString + dutDef0_rtl should include("module AddOneParameterized(") + val dutDef1_rtl = Source.fromFile(s"$testDir/dutDef1/AddOneParameterized_1.v").getLines.mkString + dutDef1_rtl should include("module AddOneParameterized_1(") + + val tb_rtl = Source.fromFile(s"$testDir/Testbench.v").getLines.mkString + tb_rtl should include("Inst1_Prefix_AddOnePramaterized_Inst1_Suffix inst0 (") + tb_rtl should include("Inst2_Prefix_AddOnePrameterized_1_Inst2_Suffix inst1 (") + (tb_rtl should not).include("module AddOneParameterized(") + (tb_rtl should not).include("module AddOneParameterized_1(") + } } From f2371da6403622d767bd0a0a87f20603e5c45281 Mon Sep 17 00:00:00 2001 From: Girish Pai Date: Tue, 21 Jun 2022 13:43:45 -0700 Subject: [PATCH 09/11] Adding negative test;fixing bugs found --- .../main/scala/chisel3/internal/Builder.scala | 4 +- .../hierarchy/SeparateElaborationSpec.scala | 128 +++++++++++++----- 2 files changed, 93 insertions(+), 39 deletions(-) diff --git a/core/src/main/scala/chisel3/internal/Builder.scala b/core/src/main/scala/chisel3/internal/Builder.scala index b5fffeabb1d..60f1775d5f5 100644 --- a/core/src/main/scala/chisel3/internal/Builder.scala +++ b/core/src/main/scala/chisel3/internal/Builder.scala @@ -344,7 +344,7 @@ private[chisel3] class DynamicContext( // Ensure there are no repeated names for imported Definitions - both Proto Names as well as ExtMod Names val importAllDefinitionProtoNames = importDefinitionAnnos.map { a => a.definition.proto.name } val importDistinctDefinitionProtoNames = importDefinitionMap.keys.toSeq - val importAllDefinitionExtModNames = importDefinitionMap.values.toSeq + val importAllDefinitionExtModNames = importDefinitionMap.toSeq.map(_._2) val importDistinctDefinitionExtModNames = importAllDefinitionExtModNames.distinct if (importDistinctDefinitionProtoNames.length < importAllDefinitionProtoNames.length) { @@ -353,7 +353,7 @@ private[chisel3] class DynamicContext( } if (importDistinctDefinitionExtModNames.length < importAllDefinitionExtModNames.length) { val duplicates = importAllDefinitionExtModNames.diff(importDistinctDefinitionExtModNames).mkString(", ") - throwException(s"Expected distinct imported Definition names but found duplicates for: $duplicates") + throwException(s"Expected distinct overrideDef names but found duplicates for: $duplicates") } val globalNamespace = Namespace.empty diff --git a/src/test/scala/chiselTests/experimental/hierarchy/SeparateElaborationSpec.scala b/src/test/scala/chiselTests/experimental/hierarchy/SeparateElaborationSpec.scala index bc3d078c8b7..2bf2517ecec 100644 --- a/src/test/scala/chiselTests/experimental/hierarchy/SeparateElaborationSpec.scala +++ b/src/test/scala/chiselTests/experimental/hierarchy/SeparateElaborationSpec.scala @@ -389,55 +389,109 @@ class SeparateElaborationSpec extends ChiselFunSpec with Utils { } it( - "(4.b): should work if a list of imported Definitions is passed between Stages with ExtModName." - ) { - val testDir = createTestDirectory(this.getClass.getSimpleName).toString + "(4.b): should work if a list of imported Definitions is passed between Stages with ExtModName." + ) { + val testDir = createTestDirectory(this.getClass.getSimpleName).toString - val dutAnnos0 = (new ChiselStage).run( - Seq( - ChiselGeneratorAnnotation(() => new AddOneParameterized(4)), - TargetDirAnnotation(s"$testDir/dutDef0") - ) + val dutAnnos0 = (new ChiselStage).run( + Seq( + ChiselGeneratorAnnotation(() => new AddOneParameterized(4)), + TargetDirAnnotation(s"$testDir/dutDef0") ) - val dutDef0 = getDesignAnnotation(dutAnnos0).design.asInstanceOf[AddOneParameterized].toDefinition + ) + val dutDef0 = getDesignAnnotation(dutAnnos0).design.asInstanceOf[AddOneParameterized].toDefinition - val dutAnnos1 = (new ChiselStage).run( - Seq( - ChiselGeneratorAnnotation(() => new AddOneParameterized(8)), - TargetDirAnnotation(s"$testDir/dutDef1"), - // pass in previously elaborated Definitions - ImportDefinitionAnnotation(dutDef0) - ) + val dutAnnos1 = (new ChiselStage).run( + Seq( + ChiselGeneratorAnnotation(() => new AddOneParameterized(8)), + TargetDirAnnotation(s"$testDir/dutDef1"), + // pass in previously elaborated Definitions + ImportDefinitionAnnotation(dutDef0) ) - val dutDef1 = getDesignAnnotation(dutAnnos1).design.asInstanceOf[AddOneParameterized].toDefinition + ) + val dutDef1 = getDesignAnnotation(dutAnnos1).design.asInstanceOf[AddOneParameterized].toDefinition - class Testbench(defn0: Definition[AddOneParameterized], defn1: Definition[AddOneParameterized]) extends Module { - val inst0 = Instance(defn0) - val inst1 = Instance(defn1) + class Testbench(defn0: Definition[AddOneParameterized], defn1: Definition[AddOneParameterized]) extends Module { + val inst0 = Instance(defn0) + val inst1 = Instance(defn1) - // Tie inputs to a value so ChiselStage does not complain - inst0.in := 0.U - inst1.in := 0.U - } + // Tie inputs to a value so ChiselStage does not complain + inst0.in := 0.U + inst1.in := 0.U + } + + (new ChiselStage).run( + Seq( + ChiselGeneratorAnnotation(() => new Testbench(dutDef0, dutDef1)), + TargetDirAnnotation(testDir), + ImportDefinitionAnnotation(dutDef0, Some("Inst1_Prefix_AddOnePramaterized_Inst1_Suffix")), + ImportDefinitionAnnotation(dutDef1, Some("Inst2_Prefix_AddOnePrameterized_1_Inst2_Suffix")) + ) + ) + val dutDef0_rtl = Source.fromFile(s"$testDir/dutDef0/AddOneParameterized.v").getLines.mkString + dutDef0_rtl should include("module AddOneParameterized(") + val dutDef1_rtl = Source.fromFile(s"$testDir/dutDef1/AddOneParameterized_1.v").getLines.mkString + dutDef1_rtl should include("module AddOneParameterized_1(") + + val tb_rtl = Source.fromFile(s"$testDir/Testbench.v").getLines.mkString + tb_rtl should include("Inst1_Prefix_AddOnePramaterized_Inst1_Suffix inst0 (") + tb_rtl should include("Inst2_Prefix_AddOnePrameterized_1_Inst2_Suffix inst1 (") + (tb_rtl should not).include("module AddOneParameterized(") + (tb_rtl should not).include("module AddOneParameterized_1(") + } + + it( + "(4.c): should throw an exception if a list of imported Definitions is passed between Stages with same ExtModName." + ) { + val testDir = createTestDirectory(this.getClass.getSimpleName).toString + + val dutAnnos0 = (new ChiselStage).run( + Seq( + ChiselGeneratorAnnotation(() => new AddOneParameterized(4)), + TargetDirAnnotation(s"$testDir/dutDef0") + ) + ) + val importDefinitionAnnos0 = allModulesToImportedDefs(dutAnnos0) + val dutDef0 = getDesignAnnotation(dutAnnos0).design.asInstanceOf[AddOneParameterized].toDefinition + + val dutAnnos1 = (new ChiselStage).run( + Seq( + ChiselGeneratorAnnotation(() => new AddOneParameterized(8)), + TargetDirAnnotation(s"$testDir/dutDef1"), + // pass in previously elaborated Definitions + ImportDefinitionAnnotation(dutDef0) + ) + ) + val importDefinitionAnnos1 = allModulesToImportedDefs(dutAnnos1) + val dutDef1 = getDesignAnnotation(dutAnnos1).design.asInstanceOf[AddOneParameterized].toDefinition + + class Testbench(defn0: Definition[AddOneParameterized], defn1: Definition[AddOneParameterized]) extends Module { + val inst0 = Instance(defn0) + val inst1 = Instance(defn1) + + // Tie inputs to a value so ChiselStage does not complain + inst0.in := 0.U + inst1.in := 0.U + } + + val dutDef0_rtl = Source.fromFile(s"$testDir/dutDef0/AddOneParameterized.v").getLines.mkString + dutDef0_rtl should include("module AddOneParameterized(") + val dutDef1_rtl = Source.fromFile(s"$testDir/dutDef1/AddOneParameterized_1.v").getLines.mkString + dutDef1_rtl should include("module AddOneParameterized_1(") + + val errMsg = intercept[ChiselException] { (new ChiselStage).run( Seq( ChiselGeneratorAnnotation(() => new Testbench(dutDef0, dutDef1)), TargetDirAnnotation(testDir), - ImportDefinitionAnnotation(dutDef0,Some("Inst1_Prefix_AddOnePramaterized_Inst1_Suffix")), - ImportDefinitionAnnotation(dutDef1,Some("Inst2_Prefix_AddOnePrameterized_1_Inst2_Suffix")) + ImportDefinitionAnnotation(dutDef0, Some("Inst1_Prefix_AddOnePrameterized_Inst1_Suffix")), + ImportDefinitionAnnotation(dutDef1, Some("Inst1_Prefix_AddOnePrameterized_Inst1_Suffix")) ) ) - - val dutDef0_rtl = Source.fromFile(s"$testDir/dutDef0/AddOneParameterized.v").getLines.mkString - dutDef0_rtl should include("module AddOneParameterized(") - val dutDef1_rtl = Source.fromFile(s"$testDir/dutDef1/AddOneParameterized_1.v").getLines.mkString - dutDef1_rtl should include("module AddOneParameterized_1(") - - val tb_rtl = Source.fromFile(s"$testDir/Testbench.v").getLines.mkString - tb_rtl should include("Inst1_Prefix_AddOnePramaterized_Inst1_Suffix inst0 (") - tb_rtl should include("Inst2_Prefix_AddOnePrameterized_1_Inst2_Suffix inst1 (") - (tb_rtl should not).include("module AddOneParameterized(") - (tb_rtl should not).include("module AddOneParameterized_1(") } + errMsg.getMessage should include( + "Expected distinct overrideDef names but found duplicates for: Inst1_Prefix_AddOnePrameterized_Inst1_Suffix" + ) + } } From 019d33f31c9136a49c90f79a82ab7b4588fa13fb Mon Sep 17 00:00:00 2001 From: Girish Pai Date: Tue, 21 Jun 2022 14:53:11 -0700 Subject: [PATCH 10/11] Move public vals to def --- .../main/scala/chisel3/internal/Builder.scala | 33 +++++++++++-------- 1 file changed, 19 insertions(+), 14 deletions(-) diff --git a/core/src/main/scala/chisel3/internal/Builder.scala b/core/src/main/scala/chisel3/internal/Builder.scala index 60f1775d5f5..0fdab756c1b 100644 --- a/core/src/main/scala/chisel3/internal/Builder.scala +++ b/core/src/main/scala/chisel3/internal/Builder.scala @@ -341,19 +341,24 @@ private[chisel3] class DynamicContext( .map(a => ((a.definition.proto.name, a.overrideDefName.getOrElse(a.definition.proto.name)))) .toMap - // Ensure there are no repeated names for imported Definitions - both Proto Names as well as ExtMod Names - val importAllDefinitionProtoNames = importDefinitionAnnos.map { a => a.definition.proto.name } - val importDistinctDefinitionProtoNames = importDefinitionMap.keys.toSeq - val importAllDefinitionExtModNames = importDefinitionMap.toSeq.map(_._2) - val importDistinctDefinitionExtModNames = importAllDefinitionExtModNames.distinct - - if (importDistinctDefinitionProtoNames.length < importAllDefinitionProtoNames.length) { - val duplicates = importAllDefinitionProtoNames.diff(importDistinctDefinitionProtoNames).mkString(", ") - throwException(s"Expected distinct imported Definition names but found duplicates for: $duplicates") - } - if (importDistinctDefinitionExtModNames.length < importAllDefinitionExtModNames.length) { - val duplicates = importAllDefinitionExtModNames.diff(importDistinctDefinitionExtModNames).mkString(", ") - throwException(s"Expected distinct overrideDef names but found duplicates for: $duplicates") + // Helper function which does 2 things + // 1. Ensure there are no repeated names for imported Definitions - both Proto Names as well as ExtMod Names + // 2. Return the distinct definition / extMod names + private def checkAndGeDistinctProtoExtModNames() = { + val importAllDefinitionProtoNames = importDefinitionAnnos.map { a => a.definition.proto.name } + val importDistinctDefinitionProtoNames = importDefinitionMap.keys.toSeq + val importAllDefinitionExtModNames = importDefinitionMap.toSeq.map(_._2) + val importDistinctDefinitionExtModNames = importAllDefinitionExtModNames.distinct + + if (importDistinctDefinitionProtoNames.length < importAllDefinitionProtoNames.length) { + val duplicates = importAllDefinitionProtoNames.diff(importDistinctDefinitionProtoNames).mkString(", ") + throwException(s"Expected distinct imported Definition names but found duplicates for: $duplicates") + } + if (importDistinctDefinitionExtModNames.length < importAllDefinitionExtModNames.length) { + val duplicates = importAllDefinitionExtModNames.diff(importDistinctDefinitionExtModNames).mkString(", ") + throwException(s"Expected distinct overrideDef names but found duplicates for: $duplicates") + } + (importAllDefinitionProtoNames ++ importAllDefinitionExtModNames).distinct } val globalNamespace = Namespace.empty @@ -361,7 +366,7 @@ private[chisel3] class DynamicContext( // Ensure imported Definitions emit as ExtModules with the correct name so // that instantiations will also use the correct name and prevent any name // conflicts with Modules/Definitions in this elaboration - (importAllDefinitionProtoNames ++ importAllDefinitionExtModNames).distinct.foreach { + checkAndGeDistinctProtoExtModNames().foreach { globalNamespace.name(_) } From a6643a16c749d414e00ec5aff6b4fd0d8978db97 Mon Sep 17 00:00:00 2001 From: Girish Pai Date: Tue, 21 Jun 2022 17:19:20 -0700 Subject: [PATCH 11/11] Incorporate Jack's feedback --- core/src/main/scala/chisel3/internal/Builder.scala | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/core/src/main/scala/chisel3/internal/Builder.scala b/core/src/main/scala/chisel3/internal/Builder.scala index 0fdab756c1b..f08c5d92e6a 100644 --- a/core/src/main/scala/chisel3/internal/Builder.scala +++ b/core/src/main/scala/chisel3/internal/Builder.scala @@ -338,7 +338,7 @@ private[chisel3] class DynamicContext( // Map holding the actual names of extModules // Pick the definition name by default in case not passed through annotation. val importDefinitionMap = importDefinitionAnnos - .map(a => ((a.definition.proto.name, a.overrideDefName.getOrElse(a.definition.proto.name)))) + .map(a => a.definition.proto.name -> a.overrideDefName.getOrElse(a.definition.proto.name)) .toMap // Helper function which does 2 things