Skip to content

Commit

Permalink
Distinguish identity from single-target views (#4186)
Browse files Browse the repository at this point in the history 
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.

(cherry picked from commit 9d2f156)
  • Loading branch information
@jackkoenig @mergify
jackkoenig authored and mergify[bot] committed Jun 18, 2024
1 parent 4c2d349 commit a75ad07
Show file tree
Hide file tree
Showing 11 changed files with 482 additions and 108 deletions.
12 changes: 5 additions & 7 deletions core/src/main/scala/chisel3/Aggregate.scala
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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}
Expand Down Expand Up @@ -46,12 +46,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
Expand Down Expand Up @@ -352,7 +350,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
Expand Down
8 changes: 4 additions & 4 deletions core/src/main/scala/chisel3/Data.scala
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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.sourceinfo._
Expand Down Expand Up @@ -426,7 +426,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
Expand Down Expand Up @@ -678,9 +678,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()
}
Expand Down
7 changes: 2 additions & 5 deletions core/src/main/scala/chisel3/Element.scala
View file
Original file line number Diff line number Diff line change
Expand Up @@ -41,11 +41,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
}
Expand Down
100 changes: 72 additions & 28 deletions core/src/main/scala/chisel3/experimental/dataview/package.scala
View file
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,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)
Expand Down Expand Up @@ -202,23 +204,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))
}
Expand Down Expand Up @@ -259,14 +260,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
Expand All @@ -275,19 +277,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")

}
}
}
}
47 changes: 16 additions & 31 deletions core/src/main/scala/chisel3/experimental/hierarchy/core/Lookupable.scala
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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, AggregateViewBinding, Builder, ChildBinding, ViewBinding, ViewParent}

Expand Down Expand Up @@ -103,11 +103,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",
Expand Down Expand Up @@ -180,43 +180,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")
}
Expand Down
12 changes: 7 additions & 5 deletions core/src/main/scala/chisel3/internal/BiConnect.scala
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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.Builder.pushCommand
Expand Down Expand Up @@ -120,8 +120,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(
Expand Down Expand Up @@ -170,8 +172,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(
Expand Down
Loading

0 comments on commit a75ad07

Please sign in to comment.