From f2ef3a8ee378a307661bd598cd44d4b895b9352e Mon Sep 17 00:00:00 2001 From: mergify[bot] Date: Tue, 8 Nov 2022 07:05:53 +0000 Subject: Improve Record.bind and Detect Records with unstable elements (backport #2829) (#2831) * Add Aggregate.elementsIterator and micro-optimize elementsIterator provides a more efficient API for iterating on the elements of Aggregates. It is especially useful for Records where getElements returns a Seq and thus eagerly constructs a new datastructure which may then just be iterated on anyway. This new elementsIterator API is then used throughout the codebase where it makes sense. Also change Vec.getElements to just return the underlying self instead of constructing a new Seq. (cherry picked from commit defa440b349031475daeff4024fad04925cccee6) # Conflicts: # core/src/main/scala/chisel3/Aggregate.scala # core/src/main/scala/chisel3/Module.scala # core/src/main/scala/chisel3/experimental/Trace.scala * Move Aggregate.bind inline into Record.bind Vec overrides bind and does not call the version in Aggregate so the version in Aggregate is misleading in that its only ever used by Records. Now there is no version in Aggregate and the actual functionality and use is more clear. (cherry picked from commit b054c30ba47026cb2a9b28c696a0a0a58b1e2ee7) # Conflicts: # core/src/main/scala/chisel3/Aggregate.scala * Extract and optimize duplicate checking Record.bind This replaces an immutable.Map with a single mutable.HashSet and saves the allocation of # elements Seqs. (cherry picked from commit 832ea52bc23424bb75b9654422b725a9cafaef40) # Conflicts: # core/src/main/scala/chisel3/Aggregate.scala * Add check for Records that define def elements (cherry picked from commit a4f223415de19e2a732e0b6a8fe681f706a19a56) * Resolve backport conflicts * Make elementsIterator final and package private * Waive false MiMa failure Co-authored-by: Jack Koenig --- core/src/main/scala/chisel3/Aggregate.scala | 123 ++++++++++++--------- core/src/main/scala/chisel3/Data.scala | 16 +-- core/src/main/scala/chisel3/Module.scala | 4 +- .../main/scala/chisel3/experimental/Trace.scala | 2 +- 4 files changed, 81 insertions(+), 64 deletions(-) (limited to 'core') diff --git a/core/src/main/scala/chisel3/Aggregate.scala b/core/src/main/scala/chisel3/Aggregate.scala index 4fc9b20f..16611277 100644 --- a/core/src/main/scala/chisel3/Aggregate.scala +++ b/core/src/main/scala/chisel3/Aggregate.scala @@ -23,52 +23,6 @@ class AliasedAggregateFieldException(message: String) extends ChiselException(me * of) other Data objects. */ sealed abstract class Aggregate extends Data { - private[chisel3] override def bind(target: Binding, parentDirection: SpecifiedDirection): Unit = { - _parent.foreach(_.addId(this)) - binding = target - - val resolvedDirection = SpecifiedDirection.fromParent(parentDirection, specifiedDirection) - val duplicates = getElements.groupBy(identity).collect { case (x, elts) if elts.size > 1 => x } - if (!duplicates.isEmpty) { - this match { - case b: Record => - // show groups of names of fields with duplicate id's - // The sorts make the displayed order of fields deterministic and matching the order of occurrence in the Bundle. - // It's a bit convoluted but happens rarely and makes the error message easier to understand - val dupNames = duplicates.toSeq - .sortBy(_._id) - .map { duplicate => - b.elements.collect { case x if x._2._id == duplicate._id => x }.toSeq - .sortBy(_._2._id) - .map(_._1) - .reverse - .mkString("(", ",", ")") - } - .mkString(",") - throw new AliasedAggregateFieldException( - s"${b.className} contains aliased fields named ${dupNames}" - ) - case _ => - throw new AliasedAggregateFieldException( - s"Aggregate ${this.getClass} contains aliased fields $duplicates ${duplicates.mkString(",")}" - ) - } - } - for (child <- getElements) { - child.bind(ChildBinding(this), resolvedDirection) - } - - // Check that children obey the directionality rules. - val childDirections = getElements.map(_.direction).toSet - ActualDirection.Empty - direction = ActualDirection.fromChildren(childDirections, resolvedDirection) match { - case Some(dir) => dir - case None => - val childWithDirections = getElements.zip(getElements.map(_.direction)) - throw MixedDirectionAggregateException( - s"Aggregate '$this' can't have elements that are both directioned and undirectioned: $childWithDirections" - ) - } - } /** Return an Aggregate's literal value if it is a literal, None otherwise. * If any element of the aggregate is not a literal with a defined width, the result isn't a literal. @@ -100,7 +54,10 @@ sealed abstract class Aggregate extends Data { */ def getElements: Seq[Data] - private[chisel3] def width: Width = getElements.map(_.width).foldLeft(0.W)(_ + _) + /** Similar to [[getElements]] but allows for more optimized use */ + private[chisel3] def elementsIterator: Iterator[Data] + + private[chisel3] def width: Width = elementsIterator.map(_.width).foldLeft(0.W)(_ + _) private[chisel3] def legacyConnect(that: Data)(implicit sourceInfo: SourceInfo): Unit = { // If the source is a DontCare, generate a DefInvalid for the sink, @@ -221,7 +178,7 @@ sealed class Vec[T <: Data] private[chisel3] (gen: => T, val length: Int) extend val resolvedDirection = SpecifiedDirection.fromParent(parentDirection, specifiedDirection) sample_element.bind(SampleElementBinding(this), resolvedDirection) - for (child <- getElements) { // assume that all children are the same + for (child <- elementsIterator) { // assume that all children are the same child.bind(ChildBinding(this), resolvedDirection) } @@ -342,8 +299,9 @@ sealed class Vec[T <: Data] private[chisel3] (gen: => T, val length: Int) extend new Vec(gen.cloneTypeFull, length).asInstanceOf[this.type] } - override def getElements: Seq[Data] = - (0 until length).map(apply(_)) + override def getElements: Seq[Data] = self + + final override private[chisel3] def elementsIterator: Iterator[Data] = self.iterator /** Default "pretty-print" implementation * Analogous to printing a Seq @@ -953,9 +911,66 @@ abstract class Record(private[chisel3] implicit val compileOptions: CompileOptio } } + /** Checks that there are no duplicate elements (aka aliased fields) in the Record */ + private def checkForAndReportDuplicates(): Unit = { + // Using List to avoid allocation in the common case of no duplicates + var duplicates: List[Data] = Nil + // Is there a more optimized datastructure we could use with the Int identities? BitSet? Requires benchmarking. + val seen = mutable.HashSet.empty[Data] + this.elementsIterator.foreach { e => + if (seen(e)) { + duplicates = e :: duplicates + } + seen += e + } + if (!duplicates.isEmpty) { + // show groups of names of fields with duplicate id's + // The sorts make the displayed order of fields deterministic and matching the order of occurrence in the Bundle. + // It's a bit convoluted but happens rarely and makes the error message easier to understand + val dupNames = duplicates.toSeq + .sortBy(_._id) + .map { duplicate => + this.elements.collect { case x if x._2._id == duplicate._id => x }.toSeq + .sortBy(_._2._id) + .map(_._1) + .reverse + .mkString("(", ",", ")") + } + .mkString(",") + throw new AliasedAggregateFieldException( + s"${this.className} contains aliased fields named ${dupNames}" + ) + } + } + private[chisel3] override def bind(target: Binding, parentDirection: SpecifiedDirection): Unit = { try { - super.bind(target, parentDirection) + _parent.foreach(_.addId(this)) + binding = target + + val resolvedDirection = SpecifiedDirection.fromParent(parentDirection, specifiedDirection) + + checkForAndReportDuplicates() + + for ((child, sameChild) <- this.elementsIterator.zip(this.elementsIterator)) { + if (child != sameChild) { + throwException( + s"${this.className} does not return the same objects when calling .elements multiple times. Did you make it a def by mistake?" + ) + } + child.bind(ChildBinding(this), resolvedDirection) + } + + // Check that children obey the directionality rules. + val childDirections = elementsIterator.map(_.direction).toSet - ActualDirection.Empty + direction = ActualDirection.fromChildren(childDirections, resolvedDirection) match { + case Some(dir) => dir + case None => + val childWithDirections = getElements.zip(getElements.map(_.direction)) + throw MixedDirectionAggregateException( + s"Aggregate '$this' can't have elements that are both directioned and undirectioned: $childWithDirections" + ) + } } catch { // nasty compatibility mode shim, where anything flies case e: MixedDirectionAggregateException if !compileOptions.dontAssumeDirectionality => val resolvedDirection = SpecifiedDirection.fromParent(parentDirection, specifiedDirection) @@ -1142,9 +1157,11 @@ abstract class Record(private[chisel3] implicit val compileOptions: CompileOptio } } - private[chisel3] final def allElements: Seq[Element] = elements.toIndexedSeq.flatMap(_._2.allElements) + private[chisel3] final def allElements: Seq[Element] = elementsIterator.flatMap(_.allElements).toIndexedSeq + + override def getElements: Seq[Data] = elementsIterator.toIndexedSeq - override def getElements: Seq[Data] = elements.toIndexedSeq.map(_._2) + final override private[chisel3] def elementsIterator: Iterator[Data] = elements.iterator.map(_._2) // Helper because Bundle elements are reversed before printing private[chisel3] def toPrintableHelper(elts: Seq[(String, Data)]): Printable = { diff --git a/core/src/main/scala/chisel3/Data.scala b/core/src/main/scala/chisel3/Data.scala index 3af5ade1..50093333 100644 --- a/core/src/main/scala/chisel3/Data.scala +++ b/core/src/main/scala/chisel3/Data.scala @@ -361,7 +361,7 @@ private[chisel3] object getRecursiveFields { _ ++ _ } case data: Vec[_] => - data.getElements.zipWithIndex.map { + data.elementsIterator.zipWithIndex.map { case (fieldData, fieldIndex) => getRecursiveFields(fieldData, path = s"$path($fieldIndex)") }.fold(Seq(data -> path)) { @@ -379,7 +379,7 @@ private[chisel3] object getRecursiveFields { } case data: Vec[_] => LazyList(data -> path) ++ - data.getElements.view.zipWithIndex.flatMap { + data.elementsIterator.zipWithIndex.flatMap { case (fieldData, fieldIndex) => getRecursiveFields(fieldData, path = s"$path($fieldIndex)") } @@ -406,8 +406,8 @@ private[chisel3] object getMatchedFields { _ ++ _ } case (x: Vec[_], y: Vec[_]) => - (x.getElements - .zip(y.getElements)) + (x.elementsIterator + .zip(y.elementsIterator)) .map { case (xElt, yElt) => getMatchedFields(xElt, yElt) @@ -464,7 +464,7 @@ abstract class Data extends HasId with NamedComponent with SourceInfoDoc { @deprecated("pending removal once all instances replaced", "chisel3") private[chisel3] def flatten: IndexedSeq[Element] = { this match { - case elt: Aggregate => elt.getElements.toIndexedSeq.flatMap { _.flatten } + case elt: Aggregate => elt.elementsIterator.toIndexedSeq.flatMap { _.flatten } case elt: Element => IndexedSeq(elt) case elt => throwException(s"Cannot flatten type ${elt.getClass}") } @@ -749,7 +749,7 @@ abstract class Data extends HasId with NamedComponent with SourceInfoDoc { data match { case _: Element => case agg: Aggregate => - agg.getElements.foreach(rec) + agg.elementsIterator.foreach(rec) } } rec(this) @@ -931,8 +931,8 @@ object Data { if (thiz.length != that.length) { throwException(s"Cannot compare Vecs $thiz and $that: Vec sizes differ") } else { - thiz.getElements - .zip(that.getElements) + thiz.elementsIterator + .zip(that.elementsIterator) .map { case (thisData, thatData) => thisData === thatData } .reduce(_ && _) } diff --git a/core/src/main/scala/chisel3/Module.scala b/core/src/main/scala/chisel3/Module.scala index ba2d2e32..9315a44b 100644 --- a/core/src/main/scala/chisel3/Module.scala +++ b/core/src/main/scala/chisel3/Module.scala @@ -709,9 +709,9 @@ package experimental { data match { case record: Record => val compatRecord = !record.compileOptions.dontAssumeDirectionality - record.getElements.foreach(assignCompatDir(_, compatRecord)) + record.elementsIterator.foreach(assignCompatDir(_, compatRecord)) case vec: Vec[_] => - vec.getElements.foreach(assignCompatDir(_, insideCompat)) + vec.elementsIterator.foreach(assignCompatDir(_, insideCompat)) } case SpecifiedDirection.Input | SpecifiedDirection.Output => // forced assign, nothing to do } diff --git a/core/src/main/scala/chisel3/experimental/Trace.scala b/core/src/main/scala/chisel3/experimental/Trace.scala index 33d18147..eb2ed46a 100644 --- a/core/src/main/scala/chisel3/experimental/Trace.scala +++ b/core/src/main/scala/chisel3/experimental/Trace.scala @@ -66,7 +66,7 @@ object Trace { annotate(new ChiselAnnotation { def toFirrtl: Annotation = TraceAnnotation(aggregate.toAbsoluteTarget, aggregate.toAbsoluteTarget) }) - aggregate.getElements.foreach(traceNameV2) + aggregate.elementsIterator.foreach(traceNameV2) case element: Element => annotate(new ChiselAnnotation { def toFirrtl: Annotation = TraceAnnotation(element.toAbsoluteTarget, element.toAbsoluteTarget) -- cgit v1.2.3