Skip to content

Cache all lookups, added tests #2419

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
azidar wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Cache all lookups, added tests #2419

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
d9d1c19
cache all lookups, added tests
azidar Feb 16, 2022
30e2f84
clarified code comment
azidar Feb 16, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion core/src/main/scala/chisel3/experimental/hierarchy/Definition.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 */
Expand Down
7 changes: 5 additions & 2 deletions core/src/main/scala/chisel3/experimental/hierarchy/Hierarchy.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,8 +21,11 @@ sealed trait Hierarchy[+A] {
case Clone(i: IsClone[A]) => i.getProto
}

/** Updated by calls to [[_lookup]], to avoid recloning returned Data's */
private[chisel3] val cache = HashMap[Data, Data]()
/** Updated by calls to [[_lookup]], to avoid recloning returned values */
private val cache = HashMap[Any, Any]()
Copy link
Contributor

@jackkoenig jackkoenig Feb 16, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure if I can construct the case where it will matter, but my intuition is still that this should be an IdentityHashMap

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.
Expand Down
2 changes: 1 addition & 1 deletion core/src/main/scala/chisel3/experimental/hierarchy/Instance.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 */
Expand Down
17 changes: 6 additions & 11 deletions core/src/main/scala/chisel3/experimental/hierarchy/Lookupable.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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]
)(
Expand All @@ -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)
Expand Down Expand Up @@ -157,15 +153,14 @@ 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]
)(
implicit sourceInfo: SourceInfo,
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

Expand Down Expand Up @@ -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 = {
Expand All @@ -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)
}

}
Expand Down
16 changes: 15 additions & 1 deletion src/test/scala/chiselTests/experimental/hierarchy/InstanceSpec.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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") {
Expand Down