From d9d1c19437adf8b0efe9b807d9a0828739e489b6 Mon Sep 17 00:00:00 2001 From: Adam Izraelevitz Date: Wed, 16 Feb 2022 07:53:15 -0800 Subject: [PATCH 1/2] cache all lookups, added tests --- .../experimental/hierarchy/Definition.scala | 2 +- .../experimental/hierarchy/Hierarchy.scala | 5 ++++- .../experimental/hierarchy/Instance.scala | 2 +- .../experimental/hierarchy/Lookupable.scala | 17 ++++++----------- .../experimental/hierarchy/InstanceSpec.scala | 16 +++++++++++++++- 5 files changed, 27 insertions(+), 15 deletions(-) diff --git a/core/src/main/scala/chisel3/experimental/hierarchy/Definition.scala b/core/src/main/scala/chisel3/experimental/hierarchy/Definition.scala index 59b4c692954..a00c7658187 100644 --- a/core/src/main/scala/chisel3/experimental/hierarchy/Definition.scala +++ b/core/src/main/scala/chisel3/experimental/hierarchy/Definition.scala @@ -44,7 +44,7 @@ final case class Definition[+A] private[chisel3] (private[chisel3] underlying: U implicit lookup: Lookupable[B], macroGenerated: chisel3.internal.MacroGenerated ): lookup.C = { - lookup.definitionLookup(that, this) + returnValue(that(proto), lookup.definitionLookup(that, this)) } /** @return the context of any Data's return from inside the instance */ diff --git a/core/src/main/scala/chisel3/experimental/hierarchy/Hierarchy.scala b/core/src/main/scala/chisel3/experimental/hierarchy/Hierarchy.scala index 2016bb542ac..5e6d6c23ed6 100644 --- a/core/src/main/scala/chisel3/experimental/hierarchy/Hierarchy.scala +++ b/core/src/main/scala/chisel3/experimental/hierarchy/Hierarchy.scala @@ -22,7 +22,10 @@ sealed trait Hierarchy[+A] { } /** Updated by calls to [[_lookup]], to avoid recloning returned Data's */ - private[chisel3] val cache = HashMap[Data, Data]() + private val cache = HashMap[Any, Any]() + private[chisel3] def returnValue[T](protoValue: Any, contextValue: => T): T = { + cache.getOrElseUpdate(protoValue, contextValue).asInstanceOf[T] + } private[chisel3] def getInnerDataContext: Option[BaseModule] /** Determine whether underlying proto is of type provided. diff --git a/core/src/main/scala/chisel3/experimental/hierarchy/Instance.scala b/core/src/main/scala/chisel3/experimental/hierarchy/Instance.scala index cc926771499..4f043277400 100644 --- a/core/src/main/scala/chisel3/experimental/hierarchy/Instance.scala +++ b/core/src/main/scala/chisel3/experimental/hierarchy/Instance.scala @@ -57,7 +57,7 @@ final case class Instance[+A] private[chisel3] (private[chisel3] underlying: Und implicit lookup: Lookupable[B], macroGenerated: chisel3.internal.MacroGenerated ): lookup.C = { - lookup.instanceLookup(that, this) + returnValue(that(proto), lookup.instanceLookup(that, this)) } /** Returns the definition of this Instance */ diff --git a/core/src/main/scala/chisel3/experimental/hierarchy/Lookupable.scala b/core/src/main/scala/chisel3/experimental/hierarchy/Lookupable.scala index bc94f95dbfe..a79a4ef1b7d 100644 --- a/core/src/main/scala/chisel3/experimental/hierarchy/Lookupable.scala +++ b/core/src/main/scala/chisel3/experimental/hierarchy/Lookupable.scala @@ -74,10 +74,9 @@ object Lookupable { } } // The business logic of lookupData - // Also called by cloneViewToContext which potentially needs to lookup stuff from ioMap or the cache + // Also called by cloneViewToContext which potentially needs to lookup stuff from ioMap private[chisel3] def doLookupData[A, B <: Data]( data: B, - cache: HashMap[Data, Data], ioMap: Option[Map[Data, Data]], context: Option[BaseModule] )( @@ -86,12 +85,9 @@ object Lookupable { ): B = { def impl[C <: Data](d: C): C = d match { case x: Data if ioMap.nonEmpty && ioMap.get.contains(x) => ioMap.get(x).asInstanceOf[C] - case x: Data if cache.contains(x) => cache(x).asInstanceOf[C] case _ => assert(context.nonEmpty) // TODO is this even possible? Better error message here - val ret = cloneDataToContext(d, context.get) - cache(d) = ret - ret + cloneDataToContext(d, context.get) } data.binding match { case Some(_: ChildBinding) => mapRootAndExtractSubField(data, impl) @@ -157,7 +153,6 @@ object Lookupable { // TODO Describe what this is doing at a high level private[chisel3] def cloneViewToContext[A, B <: Data]( data: B, - cache: HashMap[Data, Data], ioMap: Option[Map[Data, Data]], context: Option[BaseModule] )( @@ -165,7 +160,7 @@ object Lookupable { compileOptions: CompileOptions ): B = { // alias to shorten lookups - def lookupData[C <: Data](d: C) = doLookupData(d, cache, ioMap, context) + def lookupData[C <: Data](d: C) = doLookupData(d, ioMap, context) val result = data.cloneTypeFull @@ -329,7 +324,7 @@ object Lookupable { if (isView(ret)) { ??? // TODO!!!!!! cloneViewToContext(ret, instance, ioMap, instance.getInnerDataContext) } else { - doLookupData(ret, definition.cache, None, definition.getInnerDataContext) + doLookupData(ret, None, definition.getInnerDataContext) } } def instanceLookup[A](that: A => B, instance: Instance[A]): C = { @@ -340,9 +335,9 @@ object Lookupable { case _ => None } if (isView(ret)) { - cloneViewToContext(ret, instance.cache, ioMap, instance.getInnerDataContext) + cloneViewToContext(ret, ioMap, instance.getInnerDataContext) } else { - doLookupData(ret, instance.cache, ioMap, instance.getInnerDataContext) + doLookupData(ret, ioMap, instance.getInnerDataContext) } } diff --git a/src/test/scala/chiselTests/experimental/hierarchy/InstanceSpec.scala b/src/test/scala/chiselTests/experimental/hierarchy/InstanceSpec.scala index 407a7237311..929e08feaba 100644 --- a/src/test/scala/chiselTests/experimental/hierarchy/InstanceSpec.scala +++ b/src/test/scala/chiselTests/experimental/hierarchy/InstanceSpec.scala @@ -338,10 +338,24 @@ class InstanceSpec extends ChiselFunSpec with Utils { mark(i.syncReadMem, "SyncReadMem") } val (_, annos) = getFirrtlAndAnnos(new Top) - annos.foreach { x => println(x.serialize) } annos should contain(MarkAnnotation("~Top|Top/i:HasMems>mem".rt, "Mem")) annos should contain(MarkAnnotation("~Top|Top/i:HasMems>syncReadMem".rt, "SyncReadMem")) } + it("3.14: equality should work with repeated lookups") { + class Top() extends Module { + val d = Definition(new AddFour()) + val i = Instance(d) + i.i0 should be (i.i0) + i.i0.in should be (i.i0.in) + i.i0.i0 should be (i.i0.i0) + i.i0.i0.in should be (i.i0.i0.in) + d.i0 shouldNot be (i.i0) + d.i0.in shouldNot be (i.i0.in) + d.i0.i0 shouldNot be (i.i0.i0) + d.i0.i0.in shouldNot be (i.i0.i0.in) + } + val (_, annos) = getFirrtlAndAnnos(new Top) + } } describe("4: toInstance") { it("4.0: should work on modules") { From 30e2f8412c24d78bb77ab5d4e65655330b0b689c Mon Sep 17 00:00:00 2001 From: Adam Izraelevitz Date: Wed, 16 Feb 2022 07:56:30 -0800 Subject: [PATCH 2/2] clarified code comment --- .../main/scala/chisel3/experimental/hierarchy/Hierarchy.scala | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/core/src/main/scala/chisel3/experimental/hierarchy/Hierarchy.scala b/core/src/main/scala/chisel3/experimental/hierarchy/Hierarchy.scala index 5e6d6c23ed6..26c6bb851c1 100644 --- a/core/src/main/scala/chisel3/experimental/hierarchy/Hierarchy.scala +++ b/core/src/main/scala/chisel3/experimental/hierarchy/Hierarchy.scala @@ -21,7 +21,7 @@ sealed trait Hierarchy[+A] { case Clone(i: IsClone[A]) => i.getProto } - /** Updated by calls to [[_lookup]], to avoid recloning returned Data's */ + /** Updated by calls to [[_lookup]], to avoid recloning returned values */ private val cache = HashMap[Any, Any]() private[chisel3] def returnValue[T](protoValue: Any, contextValue: => T): T = { cache.getOrElseUpdate(protoValue, contextValue).asInstanceOf[T]