From 23e39ffe1b73202e84b9e8d65834f5ba976035e8 Mon Sep 17 00:00:00 2001 From: Jack Koenig Date: Fri, 14 Jun 2024 15:39:51 -0700 Subject: [PATCH] Distinguish identity from single-target views This is a minor overhaul of DataViews internals that make the various use cases of what were previously referred to as "1-1 views" more clear. This incidentally fixes an issue with bulk connections and views. --- core/src/main/scala/chisel3/Aggregate.scala | 12 +- core/src/main/scala/chisel3/Data.scala | 8 +- core/src/main/scala/chisel3/Element.scala | 7 +- .../experimental/dataview/package.scala | 100 ++++-- .../hierarchy/core/Lookupable.scala | 47 +-- .../scala/chisel3/internal/BiConnect.scala | 12 +- .../main/scala/chisel3/internal/Builder.scala | 45 ++- .../scala/chisel3/internal/MonoConnect.scala | 16 +- .../scala/chisel3/simulator/package.scala | 6 +- .../experimental/dataview/ReifySpec.scala | 317 ++++++++++++++++++ .../scala/chiselTests/BulkConnectSpec.scala | 20 ++ 11 files changed, 482 insertions(+), 108 deletions(-) create mode 100644 src/test/scala/chisel3/experimental/dataview/ReifySpec.scala diff --git a/core/src/main/scala/chisel3/Aggregate.scala b/core/src/main/scala/chisel3/Aggregate.scala index 4e818912dec..7486c296005 100644 --- a/core/src/main/scala/chisel3/Aggregate.scala +++ b/core/src/main/scala/chisel3/Aggregate.scala @@ -3,7 +3,7 @@ package chisel3 import chisel3.experimental.VecLiterals.AddVecLiteralConstructor -import chisel3.experimental.dataview.{isView, reify, reifySingleData, InvalidViewException} +import chisel3.experimental.dataview.{isView, reify, reifyIdentityView, InvalidViewException} import scala.collection.immutable.{SeqMap, VectorMap} import scala.collection.mutable.{HashSet, LinkedHashMap} @@ -47,12 +47,10 @@ sealed abstract class Aggregate extends Data { topBindingOpt match { // Don't accidentally invent a literal value for a view that is empty case Some(_: AggregateViewBinding) if this.getElements.isEmpty => - reifySingleData(this) match { + reifyIdentityView(this) match { case Some(target: Aggregate) => target.checkingLitOption(checkForDontCares) - case _ => - val msg = - s"It should not be possible to have an empty Aggregate view that doesn't reify to a single target, but got $this" - Builder.exception(msg)(UnlocatableSourceInfo) + // This can occur with empty Vecs or Bundles + case _ => None } case Some(_: BundleLitBinding | _: VecLitBinding | _: AggregateViewBinding) => // Records store elements in reverse order and higher indices are more significant in Vecs @@ -353,7 +351,7 @@ sealed class Vec[T <: Data] private[chisel3] (gen: => T, val length: Int) extend // Special handling for views if (isView(this)) { - reifySingleData(this) match { + reifyIdentityView(this) match { // Views complicate things a bit, but views that correspond exactly to an identical Vec can just forward the // dynamic indexing to the target Vec // In theory, we could still do this forwarding if the sample element were different by deriving a DataView diff --git a/core/src/main/scala/chisel3/Data.scala b/core/src/main/scala/chisel3/Data.scala index b39faa5cf80..878d25a4b63 100644 --- a/core/src/main/scala/chisel3/Data.scala +++ b/core/src/main/scala/chisel3/Data.scala @@ -7,7 +7,7 @@ import chisel3.experimental.dataview.reify import scala.language.experimental.macros import chisel3.experimental.{requireIsChiselType, requireIsHardware, Analog, BaseModule} import chisel3.experimental.{prefix, SourceInfo, UnlocatableSourceInfo} -import chisel3.experimental.dataview.reifySingleData +import chisel3.experimental.dataview.{reifyIdentityView, reifySingleTarget} import chisel3.internal.Builder.pushCommand import chisel3.internal._ import chisel3.internal.binding._ @@ -427,7 +427,7 @@ abstract class Data extends HasId with NamedComponent with SourceInfoDoc { // Trace views to give better error messages // Reifying involves checking against ViewParent which requires being in a Builder context // Since we're just printing a String, suppress such errors and use this object - val thiz = Try(reifySingleData(this)).toOption.flatten.getOrElse(this) + val thiz = Try(reifySingleTarget(this)).toOption.flatten.getOrElse(this) thiz.topBindingOpt match { case None => chiselType // Handle DontCares specially as they are "literal-like" but not actually literals @@ -679,9 +679,9 @@ abstract class Data extends HasId with NamedComponent with SourceInfoDoc { // DataView case Some(ViewBinding(target)) => reify(target).ref case Some(_: AggregateViewBinding) => - reifySingleData(this) match { + reifyIdentityView(this) match { // If this is an identity view (a view of something of the same type), return ref of target - case Some(target) if this.typeEquivalent(target) => target.ref + case Some(target) => target.ref // Otherwise, we need to materialize hardware of the correct type case _ => materializeWire() } diff --git a/core/src/main/scala/chisel3/Element.scala b/core/src/main/scala/chisel3/Element.scala index ea31f53a45f..54c546e6721 100644 --- a/core/src/main/scala/chisel3/Element.scala +++ b/core/src/main/scala/chisel3/Element.scala @@ -42,11 +42,8 @@ abstract class Element extends Data { case Some(b @ AggregateViewBinding(viewMap)) => viewMap.get(this) match { case Some(elt: Element) => Some(ViewBinding(elt)) - // TODO We could generate a reduced AggregateViewBinding, but is there a point? - // Generating the new object would be somewhat slow, it's not clear if we should do this - // matching anyway - case Some(data: Aggregate) => Some(b) - case _ => throwException(s"Internal Error! $this missing from topBinding $b") + // Children of Probes won't be in viewMap, just return the binding + case _ => Some(b) } case topBindingOpt => topBindingOpt } diff --git a/core/src/main/scala/chisel3/experimental/dataview/package.scala b/core/src/main/scala/chisel3/experimental/dataview/package.scala index 487e0153986..cd38a3bd653 100644 --- a/core/src/main/scala/chisel3/experimental/dataview/package.scala +++ b/core/src/main/scala/chisel3/experimental/dataview/package.scala @@ -9,6 +9,8 @@ import chisel3.properties.Property import scala.annotation.{implicitNotFound, tailrec} import scala.collection.mutable +import chisel3.reflect.DataMirror +import chisel3.experimental.ClonePorts package object dataview { case class InvalidViewException(message: String) extends chisel3.ChiselException(message) @@ -203,23 +205,22 @@ package object dataview { } view match { - case elt: Element => view.bind(ViewBinding(elementResult(elt))) + case elt: Element => view.bind(ViewBinding(elementResult(elt))) case agg: Aggregate => - // Don't forget the potential mapping of the view to the target! - target match { - case d: Data if total => - aggregateMappings += (agg -> d) - case _ => - } - val fullResult = elementResult ++ aggregateMappings - // We need to record any Aggregates that don't have a 1-1 mapping (including the view - // itself) - getRecursiveFields.lazily(view, "_").foreach { - case (unnamed: Aggregate, _) if !fullResult.contains(unnamed) => - Builder.unnamedViews += unnamed - case _ => // Do nothing + // We need to record any Aggregates that don't have a single-target mapping. + // Technically, this adds any Aggregate that is not an identity mapping, + // but we don't have a cheap way to check for single-target. + // The Builder context check is a hack to allow calling .viewAs outside of elaboration + // on views that are not identity but still single-target. + // FIXME we really just need to get rid of the need for unnammed view renaming. + if (Builder.inContext) { + getRecursiveFields.lazily(view, "_").foreach { + case (unnamed: Aggregate, _) if !fullResult.contains(unnamed) => + Builder.unnamedViews += unnamed + case _ => // Do nothing + } } agg.bind(AggregateViewBinding(fullResult)) } @@ -260,14 +261,15 @@ package object dataview { case _ => elt } - /** Determine the target of a View if it is a single Target + /** Determine the target of a View if the view is an identity mapping. * - * @note An Aggregate may be a view of unrelated [[Data]] (eg. like a Seq or tuple) and thus this - * there is no single Data representing the Target and this function will return None - * @return The single Data target of this view or None if a single Data doesn't exist - * @note Returns Some(_) of the argument if it is not a view + * This is only true if the target of the view is of the same type and fields correspond 1:1. + * For example, it would *not* be an identity view to view a Vec as a Vec with its elements in reverse order. + * + * @return The identity target of this view or None if not an identity view. + * @note Returns Some(_) of the argument if it is not a view. */ - private[chisel3] def reifySingleData(data: Data): Option[Data] = { + private[chisel3] def reifyIdentityView[T <: Data](data: T): Option[T] = { val candidate: Option[Data] = data.topBindingOpt match { case None => None @@ -276,19 +278,61 @@ package object dataview { case Some(_) => Some(data) } candidate.flatMap { d => + // This cast is safe by construction, we only put Data in the view mapping if it is an identity mapping + val cast = d.asInstanceOf[T] // Candidate may itself be a view, keep tracing in those cases - if (isView(d)) reifySingleData(d) else Some(d) + if (isView(d)) reifyIdentityView(cast) else Some(cast) } } - /** Determine the target of a View if it is a single Target + // Return all parents of a Data, including itself + private def allParents(d: Data): List[Data] = d.binding match { + case Some(ChildBinding(parent)) => d :: allParents(parent) + case _ => List(d) + } + + /** Determine the target of a View if the view maps to a single `Data`. * - * @note An Aggregate may be a view of unrelated [[Data]] (eg. like a Seq or tuple) and thus this - * there is no single Data representing the Target and this function will return None - * @return The single Data target of this view or None if a single Data doesn't exist + * An Aggregate may be a view of `non-Data` (like a `Seq` or tuple) and thus + * there is no single Data representing the Target and this function will return None. + * + * @return The single Data target of this view or None if a single Data doesn't exist. + * @note Returns Some(_) of the argument if it is not a view. */ - private[chisel3] def reifyToAggregate(data: Data): Option[Aggregate] = reifySingleData(data) match { - case Some(a: Aggregate) => Some(a) - case other => None + private[chisel3] def reifySingleTarget(data: Data): Option[Data] = { + def err(msg: String) = throwException(s"Internal Error! $msg reifySingleTarget($data)") + // Identity views are obviously single targets. + reifyIdentityView(data).orElse { + // Otherwise, all children of data need to map to all of the children of another Data. + // This is really expensive, is there a better way? + data.topBindingOpt.flatMap { + case AggregateViewBinding(mapping) => + // Take every single leaf and map to its target + val leaves = DataMirror.collectLeafMembers(data) + val targets = leaves.map { l => + // All leaves are stored in the mapping. + val oneLevel = mapping(l) + // This .get is safe because collectLeafMembers returns leaves. + // It is of type Data (not Element) because of Probes, but Probes are atomic. + reifySingleTarget(oneLevel).get + } + // Now, if there are any targets, check if all of the targets share a common parent, + // and if that parent is exclusively composed of these targets. + val tset = targets.toSet + targets.headOption.flatMap { head => + allParents(head).find { + // This is kind of a hack but ClonePorts is itself a hack. + // We must ignore ClonePorts because it isn't a real Data, so it cannot be the "Single Target" to which we map. + case _: ClonePorts => false + case p => + val pset = DataMirror.collectLeafMembers(p).toSet + pset == tset + } + } + // Anything else should've been handled by reifyIdentityView + case bad => err(s"This should not be reachable. Got binding = $bad in") + + } + } } } diff --git a/core/src/main/scala/chisel3/experimental/hierarchy/core/Lookupable.scala b/core/src/main/scala/chisel3/experimental/hierarchy/core/Lookupable.scala index adc642b0fb9..c8248bc72f3 100644 --- a/core/src/main/scala/chisel3/experimental/hierarchy/core/Lookupable.scala +++ b/core/src/main/scala/chisel3/experimental/hierarchy/core/Lookupable.scala @@ -8,7 +8,7 @@ import chisel3.experimental.hierarchy.{InstanceClone, InstantiableClone, ModuleC import scala.annotation.implicitNotFound import scala.collection.mutable.HashMap import chisel3._ -import chisel3.experimental.dataview.{isView, reify, reifySingleData} +import chisel3.experimental.dataview.{isView, reify, reifyIdentityView} import chisel3.internal.firrtl.ir.{Arg, ILit, Index, ModuleIO, Slot, ULit} import chisel3.internal.{throwException, Builder, ViewParent} import chisel3.internal.binding.{AggregateViewBinding, ChildBinding, CrossModuleBinding, ViewBinding} @@ -104,11 +104,11 @@ object Lookupable { } } - // Helper for co-iterating on Elements of aggregates, they must be the same type but that is unchecked - private def coiterate(a: Data, b: Data): Iterable[(Element, Element)] = { + // Helper for co-iterating on all Elements of aggregates, they must be the same type but that is unchecked + private def coiterate(a: Data, b: Data): Iterable[(Data, Data)] = { val as = getRecursiveFields.lazily(a, "_") val bs = getRecursiveFields.lazily(b, "_") - as.zip(bs).collect { case ((ae: Element, _), (be: Element, _)) => (ae, be) } + as.zip(bs).collect { case ((ae: Data, _), (be: Data, _)) => (ae, be) } } /** Given a Data, find the root of its binding, apply a function to the root to get a "new root", @@ -181,43 +181,28 @@ object Lookupable { data match { case e: Element => ViewBinding(lookupData(reify(avb.lookup(e).get))) case _: Aggregate => - // Provide a 1:1 mapping if possible - val singleTargetOpt = map.get(data).filter(_ => avb == data.binding.get).flatMap(reifySingleData) - singleTargetOpt match { - case Some(singleTarget) => // It is 1:1! - // This is a little tricky because the values in newMap need to point to Elements of newTarget - val newTarget = lookupData(singleTarget) - val newMap = coiterate(result, data).map { - case (res, from) => - (res: Data) -> mapRootAndExtractSubField(map(from), _ => newTarget) - }.toMap - AggregateViewBinding(newMap + (result -> newTarget)) - - case None => // No 1:1 mapping so we have to do a flat binding - // Just remap each Element of this aggregate - val newMap = coiterate(result, data).map { - // Upcast res to Data since Maps are invariant in the Key type parameter - case (res, from) => (res: Data) -> lookupData(reify(avb.lookup(from).get)) - }.toMap - AggregateViewBinding(newMap) - } + // We could just call reifyIdentityView, but since we already have avb, it is + // faster to just use it but then call reifyIdentityView in case the target is itself a view + def reifyOpt(data: Data): Option[Data] = map.get(data).flatMap(reifyIdentityView(_)) + // Just remap each Data present in the map + val newMap = coiterate(result, data).flatMap { + case (res, from) => reifyOpt(from).map(res -> lookupData(_)) + }.toMap + AggregateViewBinding(newMap) } case _ => throw new InternalErrorException("Match error: data.topBinding=${data.topBinding}") } // TODO Unify the following with `.viewAs` - // We must also mark non-1:1 and child Aggregates in the view for renaming + // We must also mark any non-identity Aggregates as unnammed newBinding match { case _: ViewBinding => // Do nothing case AggregateViewBinding(childMap) => - if (!childMap.contains(result)) { - Builder.unnamedViews += result - } - // Binding does not capture 1:1 for child aggregates views + // TODO we could do reifySingleTarget instead of just marking non-identity mappings getRecursiveFields.lazily(result, "_").foreach { - case (agg: Aggregate, _) if agg != result => + case (agg: Aggregate, _) if !childMap.contains(agg) => Builder.unnamedViews += agg - case _ => // Do nothing + case _ => () } case _ => throw new InternalErrorException("Match error: newBinding=$newBinding") } diff --git a/core/src/main/scala/chisel3/internal/BiConnect.scala b/core/src/main/scala/chisel3/internal/BiConnect.scala index 30e33e09b85..312adf6d710 100644 --- a/core/src/main/scala/chisel3/internal/BiConnect.scala +++ b/core/src/main/scala/chisel3/internal/BiConnect.scala @@ -3,7 +3,7 @@ package chisel3.internal import chisel3._ -import chisel3.experimental.dataview.{isView, reify, reifyToAggregate} +import chisel3.experimental.dataview.{isView, reify, reifyIdentityView} import chisel3.experimental.{attach, Analog, BaseModule, SourceInfo} import chisel3.properties.Property import chisel3.internal.binding._ @@ -121,8 +121,10 @@ private[chisel3] object BiConnect { throw MismatchedVecException } - val leftReified: Option[Aggregate] = if (isView(left_v)) reifyToAggregate(left_v) else Some(left_v) - val rightReified: Option[Aggregate] = if (isView(right_v)) reifyToAggregate(right_v) else Some(right_v) + val leftReified: Option[Vec[Data @unchecked]] = + if (isView(left_v)) reifyIdentityView(left_v) else Some(left_v) + val rightReified: Option[Vec[Data @unchecked]] = + if (isView(right_v)) reifyIdentityView(right_v) else Some(right_v) if ( leftReified.nonEmpty && rightReified.nonEmpty && canFirrtlConnectData( @@ -171,8 +173,8 @@ private[chisel3] object BiConnect { !MonoConnect.canBeSink(left_r, context_mod) || !MonoConnect.canBeSource(right_r, context_mod) val (newLeft, newRight) = if (flipConnection) (right_r, left_r) else (left_r, right_r) - val leftReified: Option[Aggregate] = if (isView(newLeft)) reifyToAggregate(newLeft) else Some(newLeft) - val rightReified: Option[Aggregate] = if (isView(newRight)) reifyToAggregate(newRight) else Some(newRight) + val leftReified: Option[Record] = if (isView(newLeft)) reifyIdentityView(newLeft) else Some(newLeft) + val rightReified: Option[Record] = if (isView(newRight)) reifyIdentityView(newRight) else Some(newRight) if ( leftReified.nonEmpty && rightReified.nonEmpty && canFirrtlConnectData( diff --git a/core/src/main/scala/chisel3/internal/Builder.scala b/core/src/main/scala/chisel3/internal/Builder.scala index db9ee37719a..9232e1e59a5 100644 --- a/core/src/main/scala/chisel3/internal/Builder.scala +++ b/core/src/main/scala/chisel3/internal/Builder.scala @@ -18,7 +18,7 @@ import _root_.firrtl.AnnotationSeq import _root_.firrtl.renamemap.MutableRenameMap import _root_.firrtl.util.BackendCompilationUtilities._ import _root_.firrtl.{ir => fir} -import chisel3.experimental.dataview.{reify, reifySingleData} +import chisel3.experimental.dataview.{reify, reifySingleTarget} import chisel3.internal.Builder.Prefix import logger.{LazyLogging, LoggerOption} @@ -303,7 +303,7 @@ private[chisel3] trait HasId extends chisel3.InstanceId { // Helper for reifying views if they map to a single Target private[chisel3] def reifyTarget: Option[Data] = this match { - case d: Data => reifySingleData(d) // Only Data can be views + case d: Data => reifySingleTarget(d) // Only Data can be views case bad => throwException(s"This shouldn't be possible - got $bad with ${_parent}") } @@ -953,23 +953,32 @@ private[chisel3] object Builder extends LazyLogging { // RenameMap can split into the constituent parts private[chisel3] def makeViewRenameMap: MutableRenameMap = { val renames = MutableRenameMap() - for (view <- unnamedViews) { - val localTarget = view.toTarget - val absTarget = view.toAbsoluteTarget - val elts = getRecursiveFields.lazily(view, "").collect { case (elt: Element, _) => elt } - for (elt <- elts if !elt.isLit) { - // This is a hack to not crash when .viewAs is called on non-hardware - // It can be removed in Chisel 6.0.0 when it becomes illegal to call .viewAs on non-hardware - val targetOfViewOpt = - try { - Some(reify(elt)) - } catch { - case _: NoSuchElementException => None + for (view <- unnamedViews if !view.isLit) { // Aggregates can be literals too! + reifySingleTarget(view) match { + // If the target reifies to a single target, we don't need to rename. + // This is ludicrously expensive though, find a way to make this code unnecessary. + case Some(_) => () + case None => + val localTarget = view.toTarget + val absTarget = view.toAbsoluteTarget + val elts = getRecursiveFields.lazily(view, "").collect { case (elt: Element, _) => elt } + for (elt <- elts if !elt.isLit) { + // This is a hack to not crash when .viewAs is called on non-hardware + // It can be removed in Chisel 6.0.0 when it becomes illegal to call .viewAs on non-hardware + val targetOfViewOpt = + try { + Some(reify(elt)) + } catch { + case _: NoSuchElementException => None + } + // It is legal to target DontCare, but we obviously cannot rename to DontCare + targetOfViewOpt + .filterNot(_.binding.contains(DontCareBinding())) + .foreach { targetOfView => + renames.record(localTarget, targetOfView.toTarget) + renames.record(absTarget, targetOfView.toAbsoluteTarget) + } } - targetOfViewOpt.foreach { targetOfView => - renames.record(localTarget, targetOfView.toTarget) - renames.record(absTarget, targetOfView.toAbsoluteTarget) - } } } renames diff --git a/core/src/main/scala/chisel3/internal/MonoConnect.scala b/core/src/main/scala/chisel3/internal/MonoConnect.scala index 0995afdc285..673c5d486f9 100644 --- a/core/src/main/scala/chisel3/internal/MonoConnect.scala +++ b/core/src/main/scala/chisel3/internal/MonoConnect.scala @@ -9,7 +9,7 @@ import chisel3.internal.binding._ import chisel3.internal.Builder.pushCommand import chisel3.internal.firrtl.ir.{Connect, DefInvalid, ProbeDefine, PropAssign} import chisel3.internal.firrtl.Converter -import chisel3.experimental.dataview.{isView, reify, reifySingleData, reifyToAggregate} +import chisel3.experimental.dataview.{isView, reify, reifyIdentityView} import chisel3.properties.{Class, Property} import chisel3.reflect.DataMirror @@ -156,8 +156,10 @@ private[chisel3] object MonoConnect { case (sink_v: Vec[Data @unchecked], source_v: Vec[Data @unchecked]) => if (sink_v.length != source_v.length) { throw MismatchedVecException } - val sinkReified: Option[Aggregate] = if (isView(sink_v)) reifyToAggregate(sink_v) else Some(sink_v) - val sourceReified: Option[Aggregate] = if (isView(source_v)) reifyToAggregate(source_v) else Some(source_v) + val sinkReified: Option[Vec[Data @unchecked]] = + if (isView(sink_v)) reifyIdentityView(sink_v) else Some(sink_v) + val sourceReified: Option[Vec[Data @unchecked]] = + if (isView(source_v)) reifyIdentityView(source_v) else Some(source_v) if ( sinkReified.nonEmpty && sourceReified.nonEmpty && canFirrtlConnectData( @@ -189,8 +191,8 @@ private[chisel3] object MonoConnect { // Handle Record case case (sink_r: Record, source_r: Record) => - val sinkReified: Option[Aggregate] = if (isView(sink_r)) reifyToAggregate(sink_r) else Some(sink_r) - val sourceReified: Option[Aggregate] = if (isView(source_r)) reifyToAggregate(source_r) else Some(source_r) + val sinkReified: Option[Record] = if (isView(sink_r)) reifyIdentityView(sink_r) else Some(sink_r) + val sourceReified: Option[Record] = if (isView(source_r)) reifyIdentityView(source_r) else Some(source_r) if ( sinkReified.nonEmpty && sourceReified.nonEmpty && canFirrtlConnectData( @@ -445,12 +447,12 @@ private[chisel3] object MonoConnect { context: BaseModule ): Unit = { - val sink = reifySingleData(sinkProbe).getOrElse( + val sink = reifyIdentityView(sinkProbe).getOrElse( throwException( s"If a DataView contains a Probe, it must resolve to one Data. $sinkProbe does not meet this criteria." ) ) - val source = reifySingleData(sourceProbe).getOrElse( + val source = reifyIdentityView(sourceProbe).getOrElse( throwException( s"If a DataView contains a Probe, it must resolve to one Data. $sourceProbe does not meet this criteria." ) diff --git a/src/main/scala/chisel3/simulator/package.scala b/src/main/scala/chisel3/simulator/package.scala index a62a8d005b1..14269602165 100644 --- a/src/main/scala/chisel3/simulator/package.scala +++ b/src/main/scala/chisel3/simulator/package.scala @@ -2,7 +2,7 @@ package chisel3 import svsim._ import chisel3.reflect.DataMirror -import chisel3.experimental.dataview.reifySingleData +import chisel3.experimental.dataview.reifyIdentityView import scala.collection.mutable import java.nio.file.{Files, Path, Paths} @@ -34,9 +34,9 @@ package object simulator { case (data, port) => data -> controller.port(port.name) }.toMap def port(data: Data): Simulation.Port = { - // TODO, we can support non 1-1 views, but it will require changing this API to return a Seq[Port] + // TODO, we can support non identity views, but it will require changing this API to return a Seq[Port] // and packing/unpacking the BigInt literal representation. - val reified = reifySingleData(data).getOrElse { + val reified = reifyIdentityView(data).getOrElse { val url = "https://github.com/chipsalliance/chisel/issues/new/choose" throw new Exception( s"Cannot poke $data as is a view that does not map to a single Data. " + diff --git a/src/test/scala/chisel3/experimental/dataview/ReifySpec.scala b/src/test/scala/chisel3/experimental/dataview/ReifySpec.scala new file mode 100644 index 00000000000..006a5fc6d43 --- /dev/null +++ b/src/test/scala/chisel3/experimental/dataview/ReifySpec.scala @@ -0,0 +1,317 @@ +// SPDX-License-Identifier: Apache-2.0 + +package chisel3.experimental.dataview + +import circt.stage.ChiselStage +import chisel3._ +import chisel3.probe.Probe +import chisel3.properties.Property +import chisel3.experimental.Analog +import chisel3.experimental.dataview._ + +import org.scalatest.funspec.AnyFunSpec +import org.scalatest.matchers.should.Matchers._ +import chisel3.reflect.DataMirror +import chisel3.experimental.hierarchy.{instantiable, public, Instantiate} + +object ReifySpec { + // Views can be single target of the same type, but still not identity. + type ReversedVec[T <: Data] = Vec[T] + implicit def reversedVecView[T <: Data]: DataView[Vec[T], ReversedVec[T]] = + DataView.mapping[Vec[T], ReversedVec[T]](v => v.cloneType, { case (a, b) => a.reverse.zip(b) }) + + class AllElementsBundle extends Bundle { + val u = UInt(8.W) + val s = SInt(8.W) + val b = Bool() + val r = Reset() + val d = AsyncReset() + val c = Clock() + val a = Analog(8.W) + val p = Probe(UInt(8.W)) + val prop = Property[String]() + } + implicit val allElementsView: DataView[Seq[Data], AllElementsBundle] = + DataView.mapping[Seq[Data], AllElementsBundle]( + _ => new AllElementsBundle, + { case (a, b) => a.zip(b.getElements) } + ) + + class SimpleBundle extends Bundle { + val value = UInt(8.W) + } + class NestedBundle extends Bundle { + val child = new SimpleBundle + } + class TargetBundle extends Bundle { + val fizz = new NestedBundle + val buzz = new SimpleBundle + val vec = Vec(2, UInt(8.W)) + } + class ViewChildBundle extends Bundle { + val a = Vec(2, UInt(8.W)) + val b = Vec(2, UInt(8.W)) + } + class ViewBundle extends Bundle { + val foo = UInt(8.W) // used to ensure ViewBundle isn't 1-1 + val bar = new ViewChildBundle + } + // The key thing about this mapping is that ViewBundle.bar maps 1-1 (but not identity) to TargetBundle + // and that ViewBundle.bar.b identity maps to TargetBundle.vec. + implicit val myView: DataView[(UInt, TargetBundle), ViewBundle] = + DataView.mapping[(UInt, TargetBundle), ViewBundle]( + _ => new ViewBundle, + { + case ((u, t), v) => + Seq(u -> v.foo, t.fizz.child.value -> v.bar.a(0), t.buzz.value -> v.bar.a(1), t.vec -> v.bar.b) + } + ) +} + +import ReifySpec._ + +class ReifySpec extends AnyFunSpec { + + describe("dataview.reify") { + + it("should reify single targets and identity for all non-view Elements") { + ChiselStage.convert(new Module { + val wires = (new AllElementsBundle).getElements.map(Wire(_)) + + // .getElements returns Data so we have to match that these are Elements. + wires.foreach { + case elt: Element => + reify(elt) should be(elt) + reifyIdentityView(elt) should be(Some(elt)) + reifySingleTarget(elt) should be(Some(elt)) + } + }) + } + + it("should reify single targets and identity for all non-view Elements even as children of an Aggregate") { + ChiselStage.convert(new Module { + val bundle = IO(new AllElementsBundle) + + // .getElements returns Data so we have to match that these are Elements. + bundle.getElements.foreach { + case elt: Element => + reify(elt) should be(elt) + reifyIdentityView(elt) should be(Some(elt)) + reifySingleTarget(elt) should be(Some(elt)) + } + }) + } + + it("should reify single targets and identity for all Elements in an Aggregate identity-view") { + ChiselStage.convert(new Module { + val bundle = IO(new AllElementsBundle) + val view = bundle.viewAs[AllElementsBundle] + + reifyIdentityView(view) should be(Some(bundle)) + reifySingleTarget(view) should be(Some(bundle)) + + // .getElements returns Data so we have to match that these are Elements. + view.getElements.zip(bundle.getElements).foreach { + case (v: Element, t: Element) => + reify(v) should be(t) + reifyIdentityView(v) should be(Some(t)) + reifySingleTarget(v) should be(Some(t)) + } + }) + } + + it("should reify single targets and identity for all Elements in an Aggregate non-identity and non-1-1 view") { + ChiselStage.convert(new Module { + val wires = (new AllElementsBundle).getElements.map(Wire(_)) + val view = wires.viewAs[AllElementsBundle] + + reifyIdentityView(view) should be(None) + reifySingleTarget(view) should be(None) + + // .getElements returns Data so we have to match that these are Elements. + view.getElements.zip(wires).foreach { + case (v: Element, t: Element) => + reify(v) should be(t) + reifyIdentityView(v) should be(Some(t)) + reifySingleTarget(v) should be(Some(t)) + } + }) + } + + it("should distinguish identity views from single-target views (even if the single target is the same type!") { + ChiselStage.convert(new Module { + val vec = IO(Vec(2, UInt(8.W))) + val view = vec.viewAs[ReversedVec[UInt]] + + DataMirror.checkTypeEquivalence(vec, view) should be(true) + + reifyIdentityView(view) should be(None) + reifySingleTarget(view) should be(Some(vec)) + + // But child elements remain identity views, yet note the reverse. + reify(view(0)) should be(vec(1)) + reifyIdentityView(view(0)) should be(Some(vec(1))) + reifySingleTarget(view(0)) should be(Some(vec(1))) + reify(view(1)) should be(vec(0)) + reifyIdentityView(view(1)) should be(Some(vec(0))) + reifySingleTarget(view(1)) should be(Some(vec(0))) + }) + } + + it("should correctly reify single-target views despite complex hierarchy") { + ChiselStage.convert(new Module { + val in0 = IO(Input(UInt(8.W))) + val in1 = IO(Input(new TargetBundle)) + val view = (in0, in1).viewAs[ViewBundle] + + reifySingleTarget(view) should be(None) + reifyIdentityView(view.bar) should be(None) + reifySingleTarget(view.bar) should be(Some(in1)) + // Note how view.bar has a single target, but view.bar.a does not. + reifyIdentityView(view.bar.a) should be(None) + reifySingleTarget(view.bar.a) should be(None) + // Note that view.bar does not have an identity view, but view.bar.b does. + reifyIdentityView(view.bar.b) should be(Some(in1.vec)) + reifySingleTarget(view.bar.b) should be(Some(in1.vec)) + }) + } + } + + // One would like to not duplicate these checks, but it's tricky because T and Instance[T] are different. + // We could box T with .toInstance but we do want to check the 2 main user code paths. + describe("dataview.reify + D/I") { + + it("should reify single targets and identity for all non-view Elements") { + @instantiable + class MyModule extends RawModule { + @public val ios = (new AllElementsBundle).getElements.map(IO(_)) + } + ChiselStage.convert(new RawModule { + + val child = Instantiate(new MyModule) + + // .getElements returns Data so we have to match that these are Elements. + child.ios.foreach { + case elt: Element => + reify(elt) should be(elt) + reifyIdentityView(elt) should be(Some(elt)) + reifySingleTarget(elt) should be(Some(elt)) + } + }) + } + + it("should reify single targets and identity for all non-view Elements even as children of an Aggregate") { + @instantiable + class MyModule extends RawModule { + @public val bundle = IO(new AllElementsBundle) + } + ChiselStage.convert(new Module { + val child = Instantiate(new MyModule) + + // .getElements returns Data so we have to match that these are Elements. + child.bundle.getElements.foreach { + case elt: Element => + reify(elt) should be(elt) + reifyIdentityView(elt) should be(Some(elt)) + reifySingleTarget(elt) should be(Some(elt)) + } + }) + } + + it("should reify single targets and identity for all Elements in an Aggregate identity-view") { + @instantiable + class MyModule extends RawModule { + @public val bundle = IO(new AllElementsBundle) + @public val view = bundle.viewAs[AllElementsBundle] + } + ChiselStage.convert(new Module { + val child = Instantiate(new MyModule) + + reifyIdentityView(child.view) should be(Some(child.bundle)) + reifySingleTarget(child.view) should be(Some(child.bundle)) + + // .getElements returns Data so we have to match that these are Elements. + child.view.getElements.zip(child.bundle.getElements).foreach { + case (v: Element, t: Element) => + reify(v) should be(t) + reifyIdentityView(v) should be(Some(t)) + reifySingleTarget(v) should be(Some(t)) + } + }) + } + + it("should reify single targets and identity for all Elements in an Aggregate non-identity and non-1-1 view") { + @instantiable + class MyModule extends RawModule { + @public val wires = (new AllElementsBundle).getElements.map(Wire(_)) + @public val view = wires.viewAs[AllElementsBundle] + } + ChiselStage.convert(new Module { + val child = Instantiate(new MyModule) + + reifyIdentityView(child.view) should be(None) + reifySingleTarget(child.view) should be(None) + + // .getElements returns Data so we have to match that these are Elements. + child.view.getElements.zip(child.wires).foreach { + case (v: Element, t: Element) => + reify(v) should be(t) + reifyIdentityView(v) should be(Some(t)) + reifySingleTarget(v) should be(Some(t)) + } + }) + } + + it("should distinguish identity views from single-target views (even if the single target is the same type!") { + @instantiable + class MyModule extends RawModule { + @public val vec = IO(Vec(2, UInt(8.W))) + @public val view = vec.viewAs[ReversedVec[UInt]] + } + ChiselStage.convert(new Module { + val child = Instantiate(new MyModule) + val vec = child.vec + val view = child.view + + DataMirror.checkTypeEquivalence(vec, view) should be(true) + + reifyIdentityView(view) should be(None) + reifySingleTarget(view) should be(Some(vec)) + + // But child elements remain identity views, yet note the reverse. + reify(view(0)) should be(vec(1)) + reifyIdentityView(view(0)) should be(Some(vec(1))) + reifySingleTarget(view(0)) should be(Some(vec(1))) + reify(view(1)) should be(vec(0)) + reifyIdentityView(view(1)) should be(Some(vec(0))) + reifySingleTarget(view(1)) should be(Some(vec(0))) + }) + } + + it("should correctly reify single-target views despite complex hierarchy") { + @instantiable + class MyModule extends RawModule { + @public val in0 = IO(Input(UInt(8.W))) + @public val in1 = IO(Input(new TargetBundle)) + @public val view = (in0, in1).viewAs[ViewBundle] + } + ChiselStage.convert(new Module { + val child = Instantiate(new MyModule) + val in0 = child.in0 + val in1 = child.in1 + val view = child.view + + reifySingleTarget(view) should be(None) + reifyIdentityView(view.bar) should be(None) + reifySingleTarget(view.bar) should be(Some(in1)) + // Note how view.bar has a single target, but view.bar.a does not. + reifyIdentityView(view.bar.a) should be(None) + reifySingleTarget(view.bar.a) should be(None) + // Note that view.bar does not have an identity view, but view.bar.b does. + reifyIdentityView(view.bar.b) should be(Some(in1.vec)) + reifySingleTarget(view.bar.b) should be(Some(in1.vec)) + }) + } + } + +} diff --git a/src/test/scala/chiselTests/BulkConnectSpec.scala b/src/test/scala/chiselTests/BulkConnectSpec.scala index 38623791ec0..ba5a8668efa 100644 --- a/src/test/scala/chiselTests/BulkConnectSpec.scala +++ b/src/test/scala/chiselTests/BulkConnectSpec.scala @@ -92,4 +92,24 @@ class BulkConnectSpec extends ChiselPropSpec { }) chirrtl should include("connect w2, w1") } + + property("Chisel connects should not emit a FIRRTL bulk connect for single target but non-identity views") { + import chisel3.experimental.dataview._ + type ReversedVec[T <: Data] = Vec[T] + implicit def reversedVecView[T <: Data]: DataView[Vec[T], ReversedVec[T]] = + DataView.mapping[Vec[T], ReversedVec[T]](v => v.cloneType, { case (a, b) => a.reverse.zip(b) }) + val chirrtl = ChiselStage.emitCHIRRTL(new Module { + val in0, in1 = IO(Input(Vec(2, UInt(8.W)))) + val out0, out1 = IO(Output(Vec(2, UInt(8.W)))) + + out0 := in0.viewAs[ReversedVec[UInt]] + out1 <> in1.viewAs[ReversedVec[UInt]] + }) + chirrtl shouldNot include("connect out0, in0") + chirrtl should include("connect out0[0], in0[1]") + chirrtl should include("connect out0[1], in0[0]") + chirrtl shouldNot include("connect out1, in1") + chirrtl should include("connect out1[0], in1[1]") + chirrtl should include("connect out1[1], in1[0]") + } }