diff options
| -rw-r--r-- | build.sbt | 2 | ||||
| -rw-r--r-- | core/src/main/scala/chisel3/Aggregate.scala | 123 | ||||
| -rw-r--r-- | core/src/main/scala/chisel3/Data.scala | 16 | ||||
| -rw-r--r-- | core/src/main/scala/chisel3/Module.scala | 4 | ||||
| -rw-r--r-- | core/src/main/scala/chisel3/experimental/Trace.scala | 2 | ||||
| -rw-r--r-- | src/main/scala/chisel3/aop/Select.scala | 4 | ||||
| -rw-r--r-- | src/test/scala/chiselTests/RecordSpec.scala | 13 |
7 files changed, 98 insertions, 66 deletions
@@ -193,6 +193,8 @@ lazy val core = (project in file("core")). settings( mimaPreviousArtifacts := Set("edu.berkeley.cs" %% "chisel3-core" % "3.5.4"), mimaBinaryIssueFilters ++= Seq( + // This is not a problem because the relevant method is implemented (and final) in Vec and Record + ProblemFilters.exclude[ReversedMissingMethodProblem]("chisel3.Aggregate.elementsIterator"), // Modified package private methods (https://github.com/lightbend/mima/issues/53) ProblemFilters.exclude[DirectMissingMethodProblem]("chisel3.Data._computeName"), ProblemFilters.exclude[DirectMissingMethodProblem]("chisel3.Data.forceName"), 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) diff --git a/src/main/scala/chisel3/aop/Select.scala b/src/main/scala/chisel3/aop/Select.scala index 3a2a8931..738d6f31 100644 --- a/src/main/scala/chisel3/aop/Select.scala +++ b/src/main/scala/chisel3/aop/Select.scala @@ -26,7 +26,7 @@ object Select { * @param d Component to find leafs if aggregate typed. Intermediate fields/indicies are not included */ def getLeafs(d: Data): Seq[Data] = d match { - case r: Record => r.getElements.flatMap(getLeafs) + case r: Record => r.elementsIterator.flatMap(getLeafs).toSeq case v: Vec[_] => v.getElements.flatMap(getLeafs) case other => Seq(other) } @@ -36,7 +36,7 @@ object Select { * @param d Component to find leafs if aggregate typed. Intermediate fields/indicies ARE included */ def getIntermediateAndLeafs(d: Data): Seq[Data] = d match { - case r: Record => r +: r.getElements.flatMap(getIntermediateAndLeafs) + case r: Record => r +: r.elementsIterator.flatMap(getIntermediateAndLeafs).toSeq case v: Vec[_] => v +: v.getElements.flatMap(getIntermediateAndLeafs) case other => Seq(other) } diff --git a/src/test/scala/chiselTests/RecordSpec.scala b/src/test/scala/chiselTests/RecordSpec.scala index cde18da7..509edbbc 100644 --- a/src/test/scala/chiselTests/RecordSpec.scala +++ b/src/test/scala/chiselTests/RecordSpec.scala @@ -325,4 +325,17 @@ class RecordSpec extends ChiselFlatSpec with RecordSpecUtils with Utils { "CustomBundle" should "check the types" in { ChiselStage.elaborate { new RecordTypeTester } } + + "Record with unstable elements" should "error" in { + class MyRecord extends Record { + def elements = SeqMap("a" -> UInt(8.W)) + override def cloneType: this.type = (new MyRecord).asInstanceOf[this.type] + } + val e = the[ChiselException] thrownBy { + ChiselStage.elaborate(new Module { + val io = IO(Input(new MyRecord)) + }) + } + e.getMessage should include("does not return the same objects when calling .elements multiple times") + } } |