Skip to content

Commit

Permalink
Optimize Record._elements to not duplicate VectorMap if possible (#4254
Browse files Browse the repository at this point in the history
…) (#4265)

(cherry picked from commit 0df3ec9)

Co-authored-by: Jack Koenig <[email protected]>
Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
  • Loading branch information
mergify[bot] and jackkoenig authored Jul 10, 2024
1 parent 5506170 commit fd6163d
Show file tree
Hide file tree
Showing 2 changed files with 44 additions and 13 deletions.
45 changes: 33 additions & 12 deletions core/src/main/scala/chisel3/Aggregate.scala
Original file line number Diff line number Diff line change
Expand Up @@ -1259,25 +1259,46 @@ abstract class Record extends Aggregate {
// without having to recurse over all elements after the Record is
// constructed. Laziness of _elements means that this check will
// occur (only) at the first instance _elements is referenced.
// Also used to sanitize names and convert to more optimized VectorMap datastructure
// Also used to sanitize names and convert to more optimized VectorMap datastructure (if necessary)
// TODO We should figure out how to combine this with elements
// The main trick there is storing the sanitized names and making sure every Chisel API uses them
// Strawman proposal is to put sanitized name in the Slot refs of the children, and then replace
// _elements and elements with a single Array and deprecate elements as a public API
// This would also give an opportunity to stop storing elements in reverse order
private[chisel3] lazy val _elements: VectorMap[String, Data] = {
// Since elements is a map, 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
// Note that OpaqueTypes cannot have sanitization (the name of the element needs to stay empty)
// Use an empty Namespace to indicate OpaqueType
val namespace = Option.when(!this._isOpaqueType)(Namespace.empty)
elements.view.map {
case (name, field) =>
if (field.binding.isDefined) {
throw RebindingException(
s"Cannot create Record ${this.className}; element ${field} of Record must be a Chisel type, not hardware."
)
}
// namespace.name also sanitizes for firrtl, leave name alone for OpaqueTypes
val sanitizedName = namespace.map(_.name(name, leadingDigitOk = true)).getOrElse(name)
sanitizedName -> field
}.to(VectorMap) // VectorMap has O(1) lookup whereas ListMap is O(n)
val originalElements = elements
// Don't create a new map unless necessary, this is much more memory efficient in common case
// This is true if elements is not a VectorMap or if any names need sanitization
var needNewMap = !originalElements.isInstanceOf[VectorMap[_, _]]
val newNames =
elements.view.map {
case (name, field) =>
if (field.binding.isDefined) {
throw RebindingException(
s"Cannot create Record ${this.className}; element ${field} of Record must be a Chisel type, not hardware."
)
}
// namespace.name also sanitizes for firrtl, leave name alone for OpaqueTypes
val sanitizedName = namespace.map(_.name(name, leadingDigitOk = true)).getOrElse(name)
if (sanitizedName != name) {
needNewMap = true
}
sanitizedName
}.toArray // It's very important we eagerly evaluate this sequence.
if (needNewMap) {
newNames.view
.zip(originalElements)
.map { case (name, (_, data)) => name -> data }
.to(VectorMap) // VectorMap has O(1) lookup whereas ListMap is O(n)
} else {
originalElements.asInstanceOf[VectorMap[String, Data]]
}
}

/** Name for Pretty Printing */
Expand Down
12 changes: 11 additions & 1 deletion src/test/scala/chiselTests/RecordSpec.scala
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ import chisel3.testers.BasicTester
import chisel3.util.{Counter, Queue}
import circt.stage.ChiselStage

import scala.collection.immutable.{ListMap, SeqMap}
import scala.collection.immutable.{ListMap, SeqMap, VectorMap}

object RecordSpec {
class MyBundle extends Bundle {
Expand Down Expand Up @@ -164,6 +164,16 @@ class RecordSpec extends ChiselFlatSpec with Utils {
assertTesterPasses { new RecordDigitTester }
}

they should "sanitize the user-provided names" in {
class MyRecord extends Record {
lazy val elements = VectorMap("sanitize me" -> UInt(8.W))
}
val chirrtl = ChiselStage.emitCHIRRTL(new RawModule {
val out = IO(Output(new MyRecord))
})
chirrtl should include("output out : { sanitizeme : UInt<8>}")
}

"Bulk connect on Record" should "check that the fields match" in {
(the[ChiselException] thrownBy extractCause[ChiselException] {
ChiselStage.emitCHIRRTL { new MyModule(fooBarType, new CustomBundle("bar" -> UInt(32.W))) }
Expand Down

0 comments on commit fd6163d

Please sign in to comment.