Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Distinguish identity views from single-target views (backport #4186) #4189

Merged
merged 1 commit into from
Jun 18, 2024
Merged

Distinguish identity views from single-target views (backport #4186) #4189

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 5 additions & 7 deletions core/src/main/scala/chisel3/Aggregate.scala
View file
Open in desktop
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
Open in desktop
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
Open in desktop
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
Open in desktop
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
Open in desktop
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
Open in desktop
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
Loading