From c9957f9fd8c3d301c2600c4bd4d0681305097c0d Mon Sep 17 00:00:00 2001 From: "mergify[bot]" <37929162+mergify[bot]@users.noreply.github.com> Date: Thu, 20 Jun 2024 15:19:16 -0700 Subject: [PATCH] Add support for marking things as readOnly (backport #4190) (#4194) * Add support for marking things as readOnly (#4190) Also add internal support for marking things as "deprecated read only". (cherry picked from commit 7be42430a2d007c52323d7830a8efce3c7b8200e) # Conflicts: # core/src/main/scala/chisel3/experimental/hierarchy/core/Lookupable.scala # core/src/main/scala/chisel3/internal/Binding.scala * Resolve backport conflicts --------- Co-authored-by: Jack Koenig --- core/src/main/scala/chisel3/Aggregate.scala | 4 +- core/src/main/scala/chisel3/Data.scala | 29 +- core/src/main/scala/chisel3/Element.scala | 9 +- .../experimental/dataview/package.scala | 98 +++-- .../hierarchy/core/Lookupable.scala | 34 +- .../scala/chisel3/internal/BiConnect.scala | 107 +++-- .../main/scala/chisel3/internal/Binding.scala | 80 +++- .../main/scala/chisel3/internal/Builder.scala | 7 +- .../scala/chisel3/internal/MonoConnect.scala | 55 ++- .../main/scala/chisel3/probe/package.scala | 2 +- .../scala/chisel3/properties/Property.scala | 2 +- .../scala/chisel3/simulator/package.scala | 3 +- .../experimental/dataview/ReifySpec.scala | 77 ++-- src/test/scala/chiselTests/ReadOnlySpec.scala | 367 ++++++++++++++++++ 14 files changed, 730 insertions(+), 144 deletions(-) create mode 100644 src/test/scala/chiselTests/ReadOnlySpec.scala diff --git a/core/src/main/scala/chisel3/Aggregate.scala b/core/src/main/scala/chisel3/Aggregate.scala index 893d7eb7276..134e983155a 100644 --- a/core/src/main/scala/chisel3/Aggregate.scala +++ b/core/src/main/scala/chisel3/Aggregate.scala @@ -47,7 +47,7 @@ sealed abstract class Aggregate extends Data { // Don't accidentally invent a literal value for a view that is empty case Some(_: AggregateViewBinding) if this.getElements.isEmpty => reifyIdentityView(this) match { - case Some(target: Aggregate) => target.checkingLitOption(checkForDontCares) + case Some((target: Aggregate, _)) => target.checkingLitOption(checkForDontCares) // This can occur with empty Vecs or Bundles case _ => None } @@ -354,7 +354,7 @@ sealed class Vec[T <: Data] private[chisel3] (gen: => T, val length: Int) extend // 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 - case Some(target: Vec[T @unchecked]) + case Some((target: Vec[T @unchecked], _)) if this.length == target.length && this.sample_element.typeEquivalent(target.sample_element) => return target.apply(p) diff --git a/core/src/main/scala/chisel3/Data.scala b/core/src/main/scala/chisel3/Data.scala index d68887280e6..cdd897ffc98 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.{reifyIdentityView, reifySingleTarget} +import chisel3.experimental.dataview.{reifyIdentityView, reifySingleTarget, DataViewable} import chisel3.internal.Builder.pushCommand import chisel3.internal._ import chisel3.internal.sourceinfo._ @@ -628,8 +628,8 @@ abstract class Data extends HasId with NamedComponent with SourceInfoDoc { case Some(pb: PortBinding) if mod.flatMap(Builder.retrieveParent(_, Builder.currentModule.get)) == Builder.currentModule => true - case Some(ViewBinding(target)) => target.isVisibleFromModule - case Some(AggregateViewBinding(mapping)) => mapping.values.forall(_.isVisibleFromModule) + case Some(ViewBinding(target, _)) => target.isVisibleFromModule + case Some(AggregateViewBinding(mapping, _)) => mapping.values.forall(_.isVisibleFromModule) case Some(pb: SecretPortBinding) => true // Ignore secret to not require visibility case Some(_: UnconstrainedBinding) => true case _ => false @@ -650,13 +650,16 @@ abstract class Data extends HasId with NamedComponent with SourceInfoDoc { } // Internal API: returns a ref that can be assigned to, if consistent with the binding - private[chisel3] def lref: Node = { + private[chisel3] def lref(implicit info: SourceInfo): Node = { requireIsHardware(this) requireVisible() topBindingOpt match { case Some(binding: ReadOnlyBinding) => throwException(s"internal error: attempted to generate LHS ref to ReadOnlyBinding $binding") - case Some(ViewBinding(target)) => reify(target).lref + case Some(ViewBinding(target1, wr1)) => + val (target2, wr2) = reify(target1) + val writability = wr1.combine(wr2) + writability.reportIfReadOnly(target2.lref)(Wire(chiselTypeOf(target2)).lref) case Some(binding: TopBinding) => Node(this) case opt => throwException(s"internal error: unknown binding $opt in generating LHS ref") } @@ -676,11 +679,11 @@ abstract class Data extends HasId with NamedComponent with SourceInfoDoc { requireIsHardware(this) topBindingOpt match { // DataView - case Some(ViewBinding(target)) => reify(target).ref + case Some(ViewBinding(target, _)) => reify(target)._1.ref case Some(_: AggregateViewBinding) => reifyIdentityView(this) match { // If this is an identity view (a view of something of the same type), return ref of target - case Some(target) => target.ref + case Some((target, _)) => target.ref // Otherwise, we need to materialize hardware of the correct type case _ => materializeWire() } @@ -998,6 +1001,18 @@ object Data { } } } + + implicit class AsReadOnly[T <: Data](self: T) { + + /** Returns a read-only view of this Data + * + * It is illegal to connect to the return value of this method. + * This Data this method is called on must be a hardware type. + */ + def readOnly(implicit sourceInfo: SourceInfo): T = { + self.viewAsReadOnly(_ => "Cannot connect to read-only value") + } + } } trait WireFactory { diff --git a/core/src/main/scala/chisel3/Element.scala b/core/src/main/scala/chisel3/Element.scala index d2195c2dff6..ba7ff3d672f 100644 --- a/core/src/main/scala/chisel3/Element.scala +++ b/core/src/main/scala/chisel3/Element.scala @@ -38,9 +38,12 @@ abstract class Element extends Data { case _ => Some(DontCareBinding()) } // TODO Do we even need this? Looking up things in the AggregateViewBinding is fine - case Some(b @ AggregateViewBinding(viewMap)) => + case Some(b @ AggregateViewBinding(viewMap, _)) => viewMap.get(this) match { - case Some(elt: Element) => Some(ViewBinding(elt)) + case Some(elt: Element) => + // Very important to use this instead of elt as "this" is the key to the viewMap. + val wr = b.lookupWritability(this) + Some(ViewBinding(elt, wr)) // Children of Probes won't be in viewMap, just return the binding case _ => Some(b) } @@ -50,7 +53,7 @@ abstract class Element extends Data { private[chisel3] def litArgOption: Option[LitArg] = topBindingOpt match { case Some(ElementLitBinding(litArg)) => Some(litArg) case Some(_: ViewBinding) => - reify(this).litArgOption + reify(this)._1.litArgOption case _ => None } diff --git a/core/src/main/scala/chisel3/experimental/dataview/package.scala b/core/src/main/scala/chisel3/experimental/dataview/package.scala index adfdf573f00..28756d5d81f 100644 --- a/core/src/main/scala/chisel3/experimental/dataview/package.scala +++ b/core/src/main/scala/chisel3/experimental/dataview/package.scala @@ -19,7 +19,13 @@ package object dataview { * Calling `viewAs` also requires an implementation of [[DataView]] for the target type */ implicit class DataViewable[T](target: T) { - def viewAs[V <: Data](implicit dataproduct: DataProduct[T], dataView: DataView[T, V], sourceInfo: SourceInfo): V = { + private def _viewAsImpl[V <: Data]( + writability: ViewWriteability + )( + implicit dataproduct: DataProduct[T], + dataView: DataView[T, V], + sourceInfo: SourceInfo + ): V = { // TODO put a try catch here for ExpectedHardwareException and perhaps others // It's likely users will accidentally use chiselTypeOf or something that may error, // The right thing to use is DataMirror...chiselTypeClone because of composition with DataView.andThen @@ -27,7 +33,7 @@ package object dataview { val result: V = dataView.mkView(target) requireIsChiselType(result, "viewAs") - doBind(target, result, dataView) + doBind(target, result, dataView, writability) // Setting the parent marks these Data as Views result.setAllParents(Some(ViewParent)) @@ -37,6 +43,28 @@ package object dataview { result.forceName("view", Builder.viewNamespace) result } + + def viewAs[V <: Data]( + implicit dataproduct: DataProduct[T], + dataView: DataView[T, V], + sourceInfo: SourceInfo + ): V = _viewAsImpl(ViewWriteability.Default) + + private[chisel3] def viewAsReadOnlyDeprecated[V <: Data]( + getWarning: SourceInfo => Warning + )( + implicit dataproduct: DataProduct[T], + dataView: DataView[T, V], + sourceInfo: SourceInfo + ): V = _viewAsImpl(ViewWriteability.ReadOnlyDeprecated(getWarning)) + + private[chisel3] def viewAsReadOnly[V <: Data]( + getError: SourceInfo => String + )( + implicit dataproduct: DataProduct[T], + dataView: DataView[T, V], + sourceInfo: SourceInfo + ): V = _viewAsImpl(ViewWriteability.ReadOnly(getError)) } /** Provides `viewAsSupertype` for subclasses of [[Record]] */ @@ -74,9 +102,10 @@ package object dataview { // TODO should this be moved to class Aggregate / can it be unified with Aggregate.bind? private def doBind[T: DataProduct, V <: Data]( - target: T, - view: V, - dataView: DataView[T, V] + target: T, + view: V, + dataView: DataView[T, V], + writability: ViewWriteability )( implicit sourceInfo: SourceInfo ): Unit = { @@ -204,7 +233,7 @@ package object dataview { } view match { - case elt: Element => view.bind(ViewBinding(elementResult(elt))) + case elt: Element => view.bind(ViewBinding(elementResult(elt), writability)) case agg: Aggregate => val fullResult = elementResult ++ aggregateMappings @@ -221,15 +250,19 @@ package object dataview { case _ => // Do nothing } } - agg.bind(AggregateViewBinding(fullResult)) + val aggWritability = Option.when(writability.isReadOnly)( + Map((agg: Data) -> writability) + ) + agg.bind(AggregateViewBinding(fullResult, aggWritability)) } } // Traces an Element that may (or may not) be a view until it no longer maps // Inclusive of the argument + // Note that this does *not* include writability so do not use this in place of reify. private def unfoldView(elt: Element): LazyList[Element] = { def rec(e: Element): LazyList[Element] = e.topBindingOpt match { - case Some(ViewBinding(target)) => target #:: rec(target) + case Some(ViewBinding(target, _)) => target #:: rec(target) case Some(avb: AggregateViewBinding) => val target = avb.lookup(e).get target #:: rec(target) @@ -246,19 +279,25 @@ package object dataview { * This is the fundamental "unwrapping" or "tracing" primitive operation for handling Views within * Chisel. */ - private[chisel3] def reify(elt: Element): Element = - reify(elt, elt.topBinding) + private[chisel3] def reify(elt: Element): (Element, ViewWriteability) = + reify(elt, elt.topBinding, ViewWriteability.Default) /** Turn any [[Element]] that could be a View into a concrete Element * * This is the fundamental "unwrapping" or "tracing" primitive operation for handling Views within * Chisel. */ - @tailrec private[chisel3] def reify(elt: Element, topBinding: TopBinding): Element = + @tailrec private[chisel3] def reify( + elt: Element, + topBinding: TopBinding, + wrAcc: ViewWriteability + ): (Element, ViewWriteability) = { topBinding match { - case ViewBinding(target) => reify(target, target.topBinding) - case _ => elt + case ViewBinding(target, writeability) => + reify(target, target.topBinding, wrAcc.combine(writeability)) + case _ => (elt, wrAcc) } + } /** Determine the target of a View if the view is an identity mapping. * @@ -268,19 +307,24 @@ package object dataview { * @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 reifyIdentityView[T <: Data](data: T): Option[T] = { - val candidate: Option[Data] = + private[chisel3] def reifyIdentityView[T <: Data]( + data: T, + wrAcc: ViewWriteability = ViewWriteability.Default + ): Option[(T, ViewWriteability)] = { + val candidate: Option[(Data, ViewWriteability)] = data.topBindingOpt match { - case None => None - case Some(ViewBinding(target)) => Some(target) - case Some(AggregateViewBinding(lookup)) => lookup.get(data) - case Some(_) => Some(data) + case None => None + case Some(ViewBinding(target, wr)) => Some(target -> wr) + case Some(vb @ AggregateViewBinding(lookup, _)) => lookup.get(data).map(_ -> vb.lookupWritability(data)) + case Some(_) => Some(data -> ViewWriteability.Default) } - 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)) reifyIdentityView(cast) else Some(cast) + candidate.flatMap { + case (d, wr) => + val wrx = wrAcc.combine(wr) + // 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)) reifyIdentityView(cast, wrx) else Some(cast -> wrx) } } @@ -297,15 +341,17 @@ package object dataview { * * @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. + * @note You should never attempt to write the result of this function. */ 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 { + // We ignore writability because the return of this function should never be written. + reifyIdentityView(data).map(_._1).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) => + case AggregateViewBinding(mapping, _) => // Take every single leaf and map to its target val leaves = DataMirror.collectLeafMembers(data) val targets = leaves.map { l => 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 ca42c5c264f..6e6f5b25253 100644 --- a/core/src/main/scala/chisel3/experimental/hierarchy/core/Lookupable.scala +++ b/core/src/main/scala/chisel3/experimental/hierarchy/core/Lookupable.scala @@ -10,7 +10,15 @@ import scala.collection.mutable.HashMap import chisel3._ 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} +import chisel3.internal.{ + throwException, + AggregateViewBinding, + Builder, + ChildBinding, + ViewBinding, + ViewParent, + ViewWriteability +} /** Represents lookup typeclass to determine how a value accessed from an original IsInstantiable * should be tweaked to return the Instance's version @@ -175,19 +183,25 @@ object Lookupable { // We have to lookup the target(s) of the view since they may need to be underlying into the current context val newBinding = data.topBinding match { - case ViewBinding(target) => ViewBinding(lookupData(reify(target))) - case avb @ AggregateViewBinding(map) => + case ViewBinding(target, writable1) => + val (reified, writable2) = reify(target) + ViewBinding(lookupData(reified), writable1.combine(writable2)) + case avb @ AggregateViewBinding(map, _) => data match { - case e: Element => ViewBinding(lookupData(reify(avb.lookup(e).get))) + case e: Element => + val (reified, writable) = reify(avb.lookup(e).get) + ViewBinding(lookupData(reified), avb.lookupWritability(e).combine(writable)) case _: Aggregate => // 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(_)) + def reifyOpt(data: Data): Option[(Data, ViewWriteability)] = 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) + val mapping = coiterate(result, data).flatMap { + case (res, from) => reifyOpt(from).map { case (t, w) => (res, lookupData(t), w) } + } + val newMap = mapping.map { case (from, to, _) => from -> to }.toMap + val wrMap = mapping.flatMap { case (from, _, wr) => Option.when(wr.isReadOnly)(from -> wr) }.toMap + AggregateViewBinding(newMap, Option.when(wrMap.nonEmpty)(wrMap)) } case _ => throw new InternalErrorException("Match error: data.topBinding=${data.topBinding}") } @@ -196,7 +210,7 @@ object Lookupable { // We must also mark any non-identity Aggregates as unnammed newBinding match { case _: ViewBinding => // Do nothing - case AggregateViewBinding(childMap) => + case AggregateViewBinding(childMap, _) => // TODO we could do reifySingleTarget instead of just marking non-identity mappings getRecursiveFields.lazily(result, "_").foreach { case (agg: Aggregate, _) if !childMap.contains(agg) => diff --git a/core/src/main/scala/chisel3/internal/BiConnect.scala b/core/src/main/scala/chisel3/internal/BiConnect.scala index e278d0a6903..d036f51e13b 100644 --- a/core/src/main/scala/chisel3/internal/BiConnect.scala +++ b/core/src/main/scala/chisel3/internal/BiConnect.scala @@ -9,6 +9,7 @@ import chisel3.properties.Property import chisel3.internal.Builder.pushCommand import chisel3.internal.firrtl.ir.{Connect, DefInvalid} import chisel3.internal.firrtl.Converter +import chisel3.internal.MonoConnect.reportIfReadOnly import scala.language.experimental.macros import _root_.firrtl.passes.CheckTypes @@ -64,6 +65,10 @@ private[chisel3] object BiConnect { def RightProbeBiConnectionException(right: Data) = BiConnectException(s"Right of Probed type cannot participate in a bi connection (<>)") + private def collectNotReadOnly[T <: Data](x: Option[(T, ViewWriteability)]): Option[T] = { + x.collect { case (z, ViewWriteability.Default) => z } + } + /** This function is what recursively tries to connect a left and right together * * There is some cleverness in the use of internal try-catch to catch exceptions @@ -108,7 +113,7 @@ private[chisel3] object BiConnect { } catch { // convert attach exceptions to BiConnectExceptions case attach.AttachException(message) => throw BiConnectException(message) } - pushCommand(DefInvalid(sourceInfo, left_a.lref)) + pushCommand(DefInvalid(sourceInfo, left_a.lref(sourceInfo))) case (DontCare, right_a: Analog) => connect(sourceInfo, right, left, context_mod) case (left_e: Element, right_e: Element) => { elemConnect(sourceInfo, left_e, right_e, context_mod) @@ -120,10 +125,11 @@ private[chisel3] object BiConnect { throw MismatchedVecException } + // Filter out read-only because we don't know if it's actually an error until we inspect the elements. val leftReified: Option[Vec[Data @unchecked]] = - if (isView(left_v)) reifyIdentityView(left_v) else Some(left_v) + if (isView(left_v)) collectNotReadOnly(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 (isView(right_v)) collectNotReadOnly(reifyIdentityView(right_v)) else Some(right_v) if ( leftReified.nonEmpty && rightReified.nonEmpty && canFirrtlConnectData( @@ -133,7 +139,7 @@ private[chisel3] object BiConnect { context_mod ) ) { - pushCommand(Connect(sourceInfo, leftReified.get.lref, rightReified.get.lref)) + pushCommand(Connect(sourceInfo, leftReified.get.lref(sourceInfo), rightReified.get.lref(sourceInfo))) } else { for (idx <- 0 until left_v.length) { try { @@ -172,8 +178,11 @@ 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[Record] = if (isView(newLeft)) reifyIdentityView(newLeft) else Some(newLeft) - val rightReified: Option[Record] = if (isView(newRight)) reifyIdentityView(newRight) else Some(newRight) + // Filter out read-only because we don't know if it's actually an error until we inspect the elements. + val leftReified: Option[Record] = + if (isView(newLeft)) collectNotReadOnly(reifyIdentityView(newLeft)) else Some(newLeft) + val rightReified: Option[Record] = + if (isView(newRight)) collectNotReadOnly(reifyIdentityView(newRight)) else Some(newRight) if ( leftReified.nonEmpty && rightReified.nonEmpty && canFirrtlConnectData( @@ -183,7 +192,7 @@ private[chisel3] object BiConnect { context_mod ) ) { - pushCommand(Connect(sourceInfo, leftReified.get.lref, rightReified.get.lref)) + pushCommand(Connect(sourceInfo, leftReified.get.lref(sourceInfo), rightReified.get.lref(sourceInfo))) } else { recordConnect(sourceInfo, left_r, right_r, context_mod) } @@ -309,25 +318,57 @@ private[chisel3] object BiConnect { // These functions (finally) issue the connection operation // Issue with right as sink, left as source - private def issueConnectL2R(left: Element, right: Element)(implicit sourceInfo: SourceInfo): Unit = { + private def issueConnectL2R( + left: Element, + leftWr: ViewWriteability, + right: Element, + rightWr: ViewWriteability + )( + implicit sourceInfo: SourceInfo + ): Unit = { // Source and sink are ambiguous in the case of a Bi/Bulk Connect (<>). // If either is a DontCareBinding, just issue a DefInvalid for the other, // otherwise, issue a Connect. (left.topBinding, right.topBinding) match { - case (lb: DontCareBinding, _) => pushCommand(DefInvalid(sourceInfo, right.lref)) - case (_, rb: DontCareBinding) => pushCommand(DefInvalid(sourceInfo, left.lref)) - case (_, _) => pushCommand(Connect(sourceInfo, right.lref, left.ref)) + case (lb: DontCareBinding, _) => + rightWr.reportIfReadOnlyUnit { + pushCommand(DefInvalid(sourceInfo, right.lref)) + } + case (_, rb: DontCareBinding) => + leftWr.reportIfReadOnlyUnit { + pushCommand(DefInvalid(sourceInfo, left.lref)) + } + case (_, _) => + rightWr.reportIfReadOnlyUnit { + pushCommand(Connect(sourceInfo, right.lref, left.ref)) + } } } // Issue with left as sink, right as source - private def issueConnectR2L(left: Element, right: Element)(implicit sourceInfo: SourceInfo): Unit = { + private def issueConnectR2L( + left: Element, + leftWr: ViewWriteability, + right: Element, + rightWr: ViewWriteability + )( + implicit sourceInfo: SourceInfo + ): Unit = { // Source and sink are ambiguous in the case of a Bi/Bulk Connect (<>). // If either is a DontCareBinding, just issue a DefInvalid for the other, // otherwise, issue a Connect. (left.topBinding, right.topBinding) match { - case (lb: DontCareBinding, _) => pushCommand(DefInvalid(sourceInfo, right.lref)) - case (_, rb: DontCareBinding) => pushCommand(DefInvalid(sourceInfo, left.lref)) - case (_, _) => pushCommand(Connect(sourceInfo, left.lref, right.ref)) + case (lb: DontCareBinding, _) => + rightWr.reportIfReadOnlyUnit { + pushCommand(DefInvalid(sourceInfo, right.lref)) + } + case (_, rb: DontCareBinding) => + leftWr.reportIfReadOnlyUnit { + pushCommand(DefInvalid(sourceInfo, left.lref)) + } + case (_, _) => + leftWr.reportIfReadOnlyUnit { + pushCommand(Connect(sourceInfo, left.lref, right.ref)) + } } } @@ -340,8 +381,8 @@ private[chisel3] object BiConnect { context_mod: RawModule ): Unit = { import BindingDirection.{Input, Internal, Output} // Using extensively so import these - val left = reify(_left) - val right = reify(_right) + val (left, lwr) = reify(_left) + val (right, rwr) = reify(_right) // If left or right have no location, assume in context module // This can occur if one of them is a literal, unbound will error previously val left_mod: BaseModule = left.topBinding.location.getOrElse(context_mod) @@ -359,11 +400,11 @@ private[chisel3] object BiConnect { // Thus, right node better be a port node and thus have a direction hint ((left_direction, right_direction): @unchecked) match { // CURRENT MOD CHILD MOD - case (Input, Input) => issueConnectL2R(left, right) - case (Internal, Input) => issueConnectL2R(left, right) + case (Input, Input) => issueConnectL2R(left, lwr, right, rwr) + case (Internal, Input) => issueConnectL2R(left, lwr, right, rwr) - case (Output, Output) => issueConnectR2L(left, right) - case (Internal, Output) => issueConnectR2L(left, right) + case (Output, Output) => issueConnectR2L(left, lwr, right, rwr) + case (Internal, Output) => issueConnectR2L(left, lwr, right, rwr) case (Input, Output) => throw BothDriversException case (Output, Input) => throw NeitherDriverException @@ -376,11 +417,11 @@ private[chisel3] object BiConnect { // Thus, left node better be a port node and thus have a direction hint ((left_direction, right_direction): @unchecked) match { // CHILD MOD CURRENT MOD - case (Input, Input) => issueConnectR2L(left, right) - case (Input, Internal) => issueConnectR2L(left, right) + case (Input, Input) => issueConnectR2L(left, lwr, right, rwr) + case (Input, Internal) => issueConnectR2L(left, lwr, right, rwr) - case (Output, Output) => issueConnectL2R(left, right) - case (Output, Internal) => issueConnectL2R(left, right) + case (Output, Output) => issueConnectL2R(left, lwr, right, rwr) + case (Output, Internal) => issueConnectL2R(left, lwr, right, rwr) case (Input, Output) => throw NeitherDriverException case (Output, Input) => throw BothDriversException @@ -392,13 +433,13 @@ private[chisel3] object BiConnect { else if ((context_mod == left_mod) && (context_mod == right_mod)) { ((left_direction, right_direction): @unchecked) match { // CURRENT MOD CURRENT MOD - case (Input, Output) => issueConnectL2R(left, right) - case (Input, Internal) => issueConnectL2R(left, right) - case (Internal, Output) => issueConnectL2R(left, right) + case (Input, Output) => issueConnectL2R(left, lwr, right, rwr) + case (Input, Internal) => issueConnectL2R(left, lwr, right, rwr) + case (Internal, Output) => issueConnectL2R(left, lwr, right, rwr) - case (Output, Input) => issueConnectR2L(left, right) - case (Output, Internal) => issueConnectR2L(left, right) - case (Internal, Input) => issueConnectR2L(left, right) + case (Output, Input) => issueConnectR2L(left, lwr, right, rwr) + case (Output, Internal) => issueConnectR2L(left, lwr, right, rwr) + case (Internal, Input) => issueConnectR2L(left, lwr, right, rwr) case (Input, Input) => throw BothDriversException case (Output, Output) => throw BothDriversException @@ -413,8 +454,8 @@ private[chisel3] object BiConnect { // Thus both nodes must be ports and have a direction hint ((left_direction, right_direction): @unchecked) match { // CHILD MOD CHILD MOD - case (Input, Output) => issueConnectR2L(left, right) - case (Output, Input) => issueConnectL2R(left, right) + case (Input, Output) => issueConnectR2L(left, lwr, right, rwr) + case (Output, Input) => issueConnectL2R(left, lwr, right, rwr) case (Input, Input) => throw NeitherDriverException case (Output, Output) => throw BothDriversException diff --git a/core/src/main/scala/chisel3/internal/Binding.scala b/core/src/main/scala/chisel3/internal/Binding.scala index f6837fba379..5cc38a35052 100644 --- a/core/src/main/scala/chisel3/internal/Binding.scala +++ b/core/src/main/scala/chisel3/internal/Binding.scala @@ -3,7 +3,7 @@ package chisel3.internal import chisel3._ -import chisel3.experimental.BaseModule +import chisel3.experimental.{BaseModule, SourceInfo} import chisel3.internal.firrtl.ir.{LitArg, PropertyLit} import chisel3.properties.Class @@ -123,8 +123,57 @@ private[chisel3] case class MemTypeBinding[T <: Data](parent: MemBase[T]) extend // It is a source (RHS). It may only be connected/applied to sinks. private[chisel3] case class DontCareBinding() extends UnconstrainedBinding +/** Views are able to restrict writability of the target */ +private[chisel3] sealed trait ViewWriteability { + + final def isReadOnly: Boolean = this != ViewWriteability.Default + + /** Report errors if this is a read-only view + * + * Will use onPass value if normal operation can continue. + * Will use onFail value if this is a read-only view to continue elaboration and aggregate more errors. + */ + final def reportIfReadOnly[A](onPass: => A)(onFail: => A)(implicit info: SourceInfo): A = this match { + case ViewWriteability.Default => onPass + case ViewWriteability.ReadOnlyDeprecated(getWarning) => + Builder.warning(getWarning(info)) + onPass // This is just a warning so we propagate the pass value. + case ViewWriteability.ReadOnly(getError) => + Builder.error(getError(info)) + onFail + } + + final def reportIfReadOnlyUnit(onPass: => Unit)(implicit info: SourceInfo): Unit = + reportIfReadOnly[Unit](onPass)(()) + + /** Combine two writabilities into one */ + def combine(that: ViewWriteability): ViewWriteability +} +private[chisel3] object ViewWriteability { + + /** Default is no modification, writability of target applies */ + case object Default extends ViewWriteability { + override def combine(that: ViewWriteability): ViewWriteability = that + } + + /** Signals that will eventually become read only */ + case class ReadOnlyDeprecated(getWarning: SourceInfo => Warning) extends ViewWriteability { + override def combine(that: ViewWriteability): ViewWriteability = that match { + case ro: ReadOnly => ro + case _ => this + } + } + + /** Signals that are read only */ + case class ReadOnly(getError: SourceInfo => String) extends ViewWriteability { + override def combine(that: ViewWriteability): ViewWriteability = this + } +} + // Views currently only support 1:1 Element-level mappings -private[chisel3] case class ViewBinding(target: Element) extends Binding with ConditionalDeclarable { +private[chisel3] case class ViewBinding(target: Element, writability: ViewWriteability) + extends Binding + with ConditionalDeclarable { def location: Option[BaseModule] = target.binding.flatMap(_.location) def visibility: Option[WhenContext] = target.binding.flatMap { case c: ConditionalDeclarable => c.visibility @@ -134,15 +183,38 @@ private[chisel3] case class ViewBinding(target: Element) extends Binding with Co /** Binding for Aggregate Views * @param childMap Mapping from children of this view to their respective targets - * @param target Optional Data this Aggregate views if the view is total and the target is a Data + * @param writabilityMap Information about writability of this Aggregate or its children. + * None means all children are writable. This Map is hierarchical, access should go through `lookupWritability`. * @note For any Elements in the childMap, both key and value must be Elements * @note The types of key and value need not match for the top Data in a total view of type * Aggregate */ -private[chisel3] case class AggregateViewBinding(childMap: Map[Data, Data]) extends Binding with ConditionalDeclarable { +private[chisel3] case class AggregateViewBinding( + childMap: Map[Data, Data], + writabilityMap: Option[Map[Data, ViewWriteability]]) + extends Binding + with ConditionalDeclarable { // Helper lookup function since types of Elements always match def lookup(key: Element): Option[Element] = childMap.get(key).map(_.asInstanceOf[Element]) + /** Lookup the writability of a member of this view. + * + * Use this instead of accessing the Map directly. + */ + def lookupWritability(key: Data): ViewWriteability = { + def rec(map: Map[Data, ViewWriteability], key: Data): ViewWriteability = { + map.getOrElse( + key, { + key.binding match { + case Some(ChildBinding(parent)) => rec(map, parent) + case _ => throwException(s"Internal error! $key not found in AggregateViewBinding writabilityMap!") + } + } + ) + } + writabilityMap.map(rec(_, key)).getOrElse(ViewWriteability.Default) + } + // FIXME Technically an AggregateViewBinding can have multiple locations and visibilities // Fixing this requires an overhaul to this code so for now we just do the best we can // Return a location if there is a unique one for all targets, None otherwise diff --git a/core/src/main/scala/chisel3/internal/Builder.scala b/core/src/main/scala/chisel3/internal/Builder.scala index a9f5211fe3e..ad0334430b3 100644 --- a/core/src/main/scala/chisel3/internal/Builder.scala +++ b/core/src/main/scala/chisel3/internal/Builder.scala @@ -300,7 +300,10 @@ private[chisel3] trait HasId extends chisel3.InstanceId { nameGuess + parentGuess } - // Helper for reifying views if they map to a single Target + /** Helper for reifying views if they map to a single Target. + * + * This ignores writability, use chisel3.experimental.dataview.reify for non-Target use cases. + */ private[chisel3] def reifyTarget: Option[Data] = this match { case d: Data => reifySingleTarget(d) // Only Data can be views case bad => throwException(s"This shouldn't be possible - got $bad with ${_parent}") @@ -936,7 +939,7 @@ private[chisel3] object Builder extends LazyLogging { // 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)) + Some(reify(elt)._1) // Writability is irrelevant here } catch { case _: NoSuchElementException => None } diff --git a/core/src/main/scala/chisel3/internal/MonoConnect.scala b/core/src/main/scala/chisel3/internal/MonoConnect.scala index 0f28bb6f86a..ef299eaade3 100644 --- a/core/src/main/scala/chisel3/internal/MonoConnect.scala +++ b/core/src/main/scala/chisel3/internal/MonoConnect.scala @@ -102,6 +102,11 @@ private[chisel3] object MonoConnect { } } + private[chisel3] def reportIfReadOnly[T <: Data](x: (T, ViewWriteability))(implicit info: SourceInfo): T = { + val (data, writable) = x + writable.reportIfReadOnly(data)(WireInit(data)) + } + /** This function is what recursively tries to connect a sink and source together * * There is some cleverness in the use of internal try-catch to catch exceptions @@ -157,9 +162,9 @@ private[chisel3] object MonoConnect { if (sink_v.length != source_v.length) { throw MismatchedVecException } val sinkReified: Option[Vec[Data @unchecked]] = - if (isView(sink_v)) reifyIdentityView(sink_v) else Some(sink_v) + if (isView(sink_v)) reifyIdentityView(sink_v).map(reportIfReadOnly(_)(sourceInfo)) else Some(sink_v) val sourceReified: Option[Vec[Data @unchecked]] = - if (isView(source_v)) reifyIdentityView(source_v) else Some(source_v) + if (isView(source_v)) reifyIdentityView(source_v).map(_._1) else Some(source_v) if ( sinkReified.nonEmpty && sourceReified.nonEmpty && canFirrtlConnectData( @@ -169,7 +174,7 @@ private[chisel3] object MonoConnect { context_mod ) ) { - pushCommand(Connect(sourceInfo, sinkReified.get.lref, sourceReified.get.ref)) + pushCommand(Connect(sourceInfo, sinkReified.get.lref(sourceInfo), sourceReified.get.ref)) } else { for (idx <- 0 until sink_v.length) { try { @@ -191,8 +196,10 @@ private[chisel3] object MonoConnect { // Handle Record case case (sink_r: Record, source_r: Record) => - 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) + val sinkReified: Option[Record] = + if (isView(sink_r)) reifyIdentityView(sink_r).map(reportIfReadOnly(_)(sourceInfo)) else Some(sink_r) + val sourceReified: Option[Record] = + if (isView(source_r)) reifyIdentityView(source_r).map(_._1) else Some(source_r) if ( sinkReified.nonEmpty && sourceReified.nonEmpty && canFirrtlConnectData( @@ -202,7 +209,7 @@ private[chisel3] object MonoConnect { context_mod ) ) { - pushCommand(Connect(sourceInfo, sinkReified.get.lref, sourceReified.get.ref)) + pushCommand(Connect(sourceInfo, sinkReified.get.lref(sourceInfo), sourceReified.get.ref)) } else { // For each field, descend with right for ((field, sink_sub) <- sink_r._elements) { @@ -229,8 +236,11 @@ private[chisel3] object MonoConnect { // Source is DontCare - it may be connected to anything. It generates a defInvalid for the sink. case (_sink: Element, DontCare) => - val sink = reify(_sink) // Handle views - pushCommand(DefInvalid(sourceInfo, sink.lref)) + val (sink, writable) = reify(_sink) // Handle views. + writable.reportIfReadOnlyUnit { + pushCommand(DefInvalid(sourceInfo, sink.lref(sourceInfo))): Unit + }(sourceInfo) // Nothing to push if an error. + // DontCare as a sink is illegal. case (DontCare, _) => throw DontCareCantBeSink // Analog is illegal in mono connections. @@ -414,11 +424,11 @@ private[chisel3] object MonoConnect { context_mod: BaseModule ): Unit = { // Reify sink and source if they're views. - val sink = reify(_sink) - val source = reify(_source) + val (sink, writable) = reify(_sink) + val (source, _) = reify(_source) checkConnect(sourceInfo, sink, source, context_mod) - issueConnect(sink, source) + writable.reportIfReadOnlyUnit(issueConnect(sink, source)) } def propConnect( @@ -428,14 +438,20 @@ private[chisel3] object MonoConnect { context: BaseModule ): Unit = { // Reify sink and source if they're views. - val sink = reify(sinkProp) - val source = reify(sourceProp) + val (sink, writable) = reify(sinkProp) + val (source, _) = reify(sourceProp) checkConnect(sourceInfo, sink, source, context) // Add the PropAssign command directly onto the correct BaseModule subclass. context match { - case rm: RawModule => rm.addCommand(PropAssign(sourceInfo, sink.lref, source.ref)) - case cls: Class => cls.addCommand(PropAssign(sourceInfo, sink.lref, source.ref)) + case rm: RawModule => + writable.reportIfReadOnlyUnit { + rm.addCommand(PropAssign(sourceInfo, sink.lref(sourceInfo), source.ref)) + }(sourceInfo) + case cls: Class => + writable.reportIfReadOnlyUnit { + cls.addCommand(PropAssign(sourceInfo, sink.lref(sourceInfo), source.ref)) + }(sourceInfo) case _ => throwException("Internal Error! Property connection can only occur within RawModule or Class.") } } @@ -447,19 +463,22 @@ private[chisel3] object MonoConnect { context: BaseModule ): Unit = { - val sink = reifyIdentityView(sinkProbe).getOrElse( + val (sink, writable) = 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 = reifyIdentityView(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." ) ) checkConnect.checkConnection(sourceInfo, sink, source, context) context match { - case rm: RawModule => rm.addCommand(ProbeDefine(sourceInfo, sink.lref, source.ref)) + case rm: RawModule => + writable.reportIfReadOnlyUnit { + rm.addCommand(ProbeDefine(sourceInfo, sink.lref(sourceInfo), source.ref)) + }(sourceInfo) // Nothing to push if an error. case _ => throwException("Internal Error! Probe connection can only occur within RawModule.") } } diff --git a/core/src/main/scala/chisel3/probe/package.scala b/core/src/main/scala/chisel3/probe/package.scala index 6057154013c..6c8b471cc47 100644 --- a/core/src/main/scala/chisel3/probe/package.scala +++ b/core/src/main/scala/chisel3/probe/package.scala @@ -45,7 +45,7 @@ package object probe extends SourceInfoDoc { "Cannot use a non-writable probe expression to define a writable probe." ) } - pushCommand(ProbeDefine(sourceInfo, sink.ref, probeExpr.ref)) + pushCommand(ProbeDefine(sourceInfo, sink.lref, probeExpr.ref)) } /** Access the value of a probe. diff --git a/core/src/main/scala/chisel3/properties/Property.scala b/core/src/main/scala/chisel3/properties/Property.scala index 0832272d042..9937f59b072 100644 --- a/core/src/main/scala/chisel3/properties/Property.scala +++ b/core/src/main/scala/chisel3/properties/Property.scala @@ -299,7 +299,7 @@ sealed trait Property[T] extends Element { self => /** Internal API: returns a ref that can be assigned to, if consistent with the binding. */ - private[chisel3] override def lref: ir.Node = { + private[chisel3] override def lref(implicit info: SourceInfo): ir.Node = { requireIsHardware(this) requireVisible() topBindingOpt match { diff --git a/src/main/scala/chisel3/simulator/package.scala b/src/main/scala/chisel3/simulator/package.scala index 72ff7695358..6c5e539348d 100644 --- a/src/main/scala/chisel3/simulator/package.scala +++ b/src/main/scala/chisel3/simulator/package.scala @@ -36,7 +36,8 @@ package object simulator { def port(data: Data): Simulation.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 = reifyIdentityView(data).getOrElse { + // TODO implement support for read-only. + 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 index 006a5fc6d43..4e7b1b321af 100644 --- a/src/test/scala/chisel3/experimental/dataview/ReifySpec.scala +++ b/src/test/scala/chisel3/experimental/dataview/ReifySpec.scala @@ -15,6 +15,11 @@ import chisel3.reflect.DataMirror import chisel3.experimental.hierarchy.{instantiable, public, Instantiate} object ReifySpec { + + // Helpers to unpack the tuple returned by reify. + def _reify(elt: Element): Element = reify(elt)._1 + def _reifyIdentityView(data: Data): Option[Data] = reifyIdentityView(data).map(_._1) + // 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]] = @@ -81,8 +86,8 @@ class ReifySpec extends AnyFunSpec { // .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)) + _reify(elt) should be(elt) + _reifyIdentityView(elt) should be(Some(elt)) reifySingleTarget(elt) should be(Some(elt)) } }) @@ -95,8 +100,8 @@ class ReifySpec extends AnyFunSpec { // .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)) + _reify(elt) should be(elt) + _reifyIdentityView(elt) should be(Some(elt)) reifySingleTarget(elt) should be(Some(elt)) } }) @@ -107,14 +112,14 @@ class ReifySpec extends AnyFunSpec { val bundle = IO(new AllElementsBundle) val view = bundle.viewAs[AllElementsBundle] - reifyIdentityView(view) should be(Some(bundle)) + _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)) + _reify(v) should be(t) + _reifyIdentityView(v) should be(Some(t)) reifySingleTarget(v) should be(Some(t)) } }) @@ -125,14 +130,14 @@ class ReifySpec extends AnyFunSpec { val wires = (new AllElementsBundle).getElements.map(Wire(_)) val view = wires.viewAs[AllElementsBundle] - reifyIdentityView(view) should be(None) + _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)) + _reify(v) should be(t) + _reifyIdentityView(v) should be(Some(t)) reifySingleTarget(v) should be(Some(t)) } }) @@ -145,15 +150,15 @@ class ReifySpec extends AnyFunSpec { DataMirror.checkTypeEquivalence(vec, view) should be(true) - reifyIdentityView(view) should be(None) + _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))) + _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))) + _reify(view(1)) should be(vec(0)) + _reifyIdentityView(view(1)) should be(Some(vec(0))) reifySingleTarget(view(1)) should be(Some(vec(0))) }) } @@ -165,13 +170,13 @@ class ReifySpec extends AnyFunSpec { val view = (in0, in1).viewAs[ViewBundle] reifySingleTarget(view) should be(None) - reifyIdentityView(view.bar) 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) + _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)) + _reifyIdentityView(view.bar.b) should be(Some(in1.vec)) reifySingleTarget(view.bar.b) should be(Some(in1.vec)) }) } @@ -193,8 +198,8 @@ class ReifySpec extends AnyFunSpec { // .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)) + _reify(elt) should be(elt) + _reifyIdentityView(elt) should be(Some(elt)) reifySingleTarget(elt) should be(Some(elt)) } }) @@ -211,8 +216,8 @@ class ReifySpec extends AnyFunSpec { // .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)) + _reify(elt) should be(elt) + _reifyIdentityView(elt) should be(Some(elt)) reifySingleTarget(elt) should be(Some(elt)) } }) @@ -227,14 +232,14 @@ class ReifySpec extends AnyFunSpec { ChiselStage.convert(new Module { val child = Instantiate(new MyModule) - reifyIdentityView(child.view) should be(Some(child.bundle)) + _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)) + _reify(v) should be(t) + _reifyIdentityView(v) should be(Some(t)) reifySingleTarget(v) should be(Some(t)) } }) @@ -249,14 +254,14 @@ class ReifySpec extends AnyFunSpec { ChiselStage.convert(new Module { val child = Instantiate(new MyModule) - reifyIdentityView(child.view) should be(None) + _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)) + _reify(v) should be(t) + _reifyIdentityView(v) should be(Some(t)) reifySingleTarget(v) should be(Some(t)) } }) @@ -275,15 +280,15 @@ class ReifySpec extends AnyFunSpec { DataMirror.checkTypeEquivalence(vec, view) should be(true) - reifyIdentityView(view) should be(None) + _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))) + _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))) + _reify(view(1)) should be(vec(0)) + _reifyIdentityView(view(1)) should be(Some(vec(0))) reifySingleTarget(view(1)) should be(Some(vec(0))) }) } @@ -302,13 +307,13 @@ class ReifySpec extends AnyFunSpec { val view = child.view reifySingleTarget(view) should be(None) - reifyIdentityView(view.bar) 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) + _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)) + _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/ReadOnlySpec.scala b/src/test/scala/chiselTests/ReadOnlySpec.scala new file mode 100644 index 00000000000..4f3162d5963 --- /dev/null +++ b/src/test/scala/chiselTests/ReadOnlySpec.scala @@ -0,0 +1,367 @@ +// SPDX-License-Identifier: Apache-2.0 + +package chiselTests + +import circt.stage.ChiselStage +import chisel3._ +import chisel3.util._ +import chisel3.probe._ +import chisel3.properties._ +import chisel3.experimental.SourceInfo +import chisel3.experimental.dataview._ + +import org.scalactic.source.Position + +object ReadOnlySpec { + class SimpleBundle extends Bundle { + val a = UInt(8.W) + } + object SimpleBundle { + implicit val toOther: DataView[SimpleBundle, OtherBundle] = DataView( + _ => new OtherBundle, + _.a -> _.foo + ) + implicit val toBigger: DataView[(SimpleBundle, UInt), BiggerBundle] = DataView( + _ => new BiggerBundle, + _._1.a -> _.fizz, + _._2 -> _.buzz + ) + } + class OtherBundle extends Bundle { + val foo = UInt(8.W) + } + class BiggerBundle extends Bundle { + val fizz = UInt(8.W) + val buzz = UInt(8.W) + } + + class BidirectionalBundle extends Bundle { + val foo = UInt(8.W) + val bar = Flipped(UInt(8.W)) + } + object BidirectionalBundle { + implicit val fromTuple: DataView[(UInt, UInt), BidirectionalBundle] = DataView( + _ => new BidirectionalBundle, + _._1 -> _.foo, + _._2 -> _.bar + ) + } + + class ProbeBundle extends Bundle { + val probe = Probe(UInt(8.W)) + val rwProbe = RWProbe(UInt(8.W)) + val field = UInt(8.W) + } + object ProbeBundle { + implicit val fromTuple: DataView[(UInt, UInt, UInt), ProbeBundle] = DataView( + _ => new ProbeBundle, + _._1 -> _.probe, + _._2 -> _.rwProbe, + _._3 -> _.field + ) + } + + class PropertyBundle extends Bundle { + val field = UInt(8.W) + val prop = Property[String]() + } + object PropertyBundle { + implicit val fromTuple: DataView[(UInt, Property[String]), PropertyBundle] = DataView( + _ => new PropertyBundle, + _._1 -> _.field, + _._2 -> _.prop + ) + } +} + +class ReadOnlySpec extends ChiselFlatSpec with Utils { + import ReadOnlySpec._ + + behavior.of("readOnly") + + def check(m: => RawModule)(implicit pos: Position): Unit = { + val e = the[ChiselException] thrownBy { + ChiselStage.convert(m, Array("--throw-on-first-error")) + } + e.getMessage should include("Cannot connect to read-only value") + } + + // Probes don't support '<>' + def rightToLeftConnectOpsNoLegacyBulkConnect(implicit info: SourceInfo): Seq[(Data, Data) => Unit] = Seq( + _ := _, + _ :<= _, + _ :#= _, + _ :<>= _ + ) + + def rightToLeftConnectOps(implicit info: SourceInfo): Seq[(Data, Data) => Unit] = + rightToLeftConnectOpsNoLegacyBulkConnect :+ (_ <> _) + + def leftToRightConnectOps(implicit info: SourceInfo): Seq[(Data, Data) => Unit] = Seq( + _ :>= _, + _ :<>= _, + _ <> _ + ) + + it should "be an error to connect to a read-only UInt" in { + for (op <- rightToLeftConnectOps) { + check(new RawModule { + val foo = IO(UInt(8.W)) + op(foo.readOnly, 1.U) + }) + } + } + + it should "be an error to connect to a field of a read-only Bundle" in { + for (op <- rightToLeftConnectOps) { + check(new RawModule { + val foo = IO(new SimpleBundle) + val bar = foo.readOnly + op(bar.a, 1.U) + }) + } + } + + it should "be an error to connect to a read-only Bundle" in { + for (op <- rightToLeftConnectOps) { + check(new RawModule { + val in = IO(Input(new SimpleBundle)) + val foo = IO(new SimpleBundle) + op(foo.readOnly, in) + }) + } + } + + it should "be an error to connect to a read-only view of a UInt" in { + for (op <- rightToLeftConnectOps) { + check(new RawModule { + val foo = IO(UInt(8.W)) + val x = foo.viewAs[UInt].readOnly + op(x, 1.U) + }) + } + } + + it should "be an error to connect to a field of a read-only view of a Bundle" in { + for (op <- rightToLeftConnectOps) { + check(new RawModule { + val out = IO(new SimpleBundle) + val x = out.viewAs[OtherBundle].readOnly + op(x.foo, 1.U) + }) + } + } + + it should "be an error to connect to a read-only view of a Bundle" in { + for (op <- rightToLeftConnectOps) { + check(new RawModule { + val in = IO(Input(new OtherBundle)) + val out = IO(new SimpleBundle) + val x = out.viewAs[OtherBundle].readOnly + op(x, in) + }) + } + } + + it should "be an error to connect to a field of a view of a read-only Bundle" in { + for (op <- rightToLeftConnectOps) { + check(new RawModule { + val out = IO(new SimpleBundle) + val x = out.readOnly.viewAs[OtherBundle] + op(x.foo, 1.U) + }) + } + } + + it should "be an error to connect to a view of a read-only Bundle" in { + for (op <- rightToLeftConnectOps) { + check(new RawModule { + val in = IO(Input(new OtherBundle)) + val out = Wire(new SimpleBundle).readOnly + val x = out.viewAs[OtherBundle] + op(x, in) + }) + } + } + + it should "be an error to connect to a field of read-only non-identity view" in { + for (op <- rightToLeftConnectOps) { + check(new RawModule { + val w = IO(new SimpleBundle) + val x = IO(UInt(8.W)) + val y = (w, x).viewAs[BiggerBundle].readOnly + op(y.fizz, 1.U) + }) + } + } + + it should "be an error to connect to a read-only non-identity view" in { + for (op <- rightToLeftConnectOps) { + check(new RawModule { + val in = IO(Input(new BiggerBundle)) + val w = Wire(new SimpleBundle) + val x = Wire(UInt(8.W)) + val y = (w, x).viewAs[BiggerBundle].readOnly + op(y, in) + }) + } + } + + it should "be an error to connect to a field of a non-identity view targeting a read-only Bundle" in { + for (op <- rightToLeftConnectOps) { + check(new RawModule { + val w = IO(new SimpleBundle).readOnly + val x = IO(UInt(8.W)) + val y = (w, x).viewAs[BiggerBundle] + op(y.fizz, 1.U) + }) + } + } + + it should "NOT be an error to connect to a field of a non-identity view targeting a normal Bundle (where another targeted field is read-only)" in { + for (op <- rightToLeftConnectOps) { + val chirrtl = ChiselStage.emitCHIRRTL(new RawModule { + val w = IO(new SimpleBundle) + val x = IO(UInt(8.W)) + val y = (w, x.readOnly).viewAs[BiggerBundle] + op(y.fizz, 1.U) + }) + chirrtl should include("connect w.a, UInt<1>(0h1)") + } + } + + it should "be an error to connect to a non-identity view targeting a read-only Bundle (where another targeted field is normal)" in { + for (op <- rightToLeftConnectOps) { + check(new RawModule { + val in = IO(Input(new BiggerBundle)) + val w = Wire(new SimpleBundle).readOnly + val x = Wire(UInt(8.W)) + val y = (w, x).viewAs[BiggerBundle] + op(y, in) + }) + } + } + + it should "be an error to bidirectionally connect to a read-only Bundle on the RHS" in { + for (op <- leftToRightConnectOps) { + check(new RawModule { + val in = IO(Flipped(new BidirectionalBundle)) + val w = Wire(new BidirectionalBundle) + op(w, in.readOnly) + }) + } + } + + it should "be an error to bidirectionally connect to a view containing a read-only field on the RHS" in { + for (op <- leftToRightConnectOps) { + check(new RawModule { + val x, y = Wire(UInt(8.W)) + val z = (x, y.readOnly).viewAs[BidirectionalBundle] + val out = IO(new BidirectionalBundle) + op(out, z) + }) + // But note that it's fine if x (not flipped) is readOnly. + ChiselStage.convert(new RawModule { + val x, y = Wire(UInt(8.W)) + val z = (x.readOnly, y).viewAs[BidirectionalBundle] + val out = IO(new BidirectionalBundle) + op(out, z) + }) + } + } + + it should "be an error to define to a read-only probe" in { + check(new RawModule { + val p = IO(Output(Probe(UInt(8.W)))) + define(p.readOnly, ProbeValue(8.U(8.W))) + }) + check(new RawModule { + val p = IO(Output(RWProbe(UInt(8.W)))) + val w = Wire(UInt(8.W)) + define(p.readOnly, RWProbeValue(w)) + }) + } + + it should "be an error to connect to a read-only Bundle containing a probe" in { + for (op <- rightToLeftConnectOpsNoLegacyBulkConnect) { + check(new RawModule { + val w1 = Wire(new ProbeBundle) + val w2 = Wire(new ProbeBundle).readOnly + op(w2, w1) + }) + } + } + + it should "be an error to define to a probe field of a read-only Bundle" in { + check(new RawModule { + val w = Wire(new ProbeBundle).readOnly + define(w.probe, ProbeValue(8.U(8.W))) + }) + // Check the RWProbe too. + check(new RawModule { + val w = Wire(new ProbeBundle).readOnly + val w2 = Wire(UInt(8.W)) + define(w.rwProbe, RWProbeValue(w2)) + }) + } + + it should "be an error to connect to a view containing a read-only probe" in { + for (op <- rightToLeftConnectOpsNoLegacyBulkConnect) { + check(new RawModule { + val a = IO(Output(Probe(UInt(8.W)))) + val b = IO(Output(RWProbe(UInt(8.W)))) + val c = IO(Output(UInt(8.W))) + val d = (a.readOnly, b, c).viewAs[ProbeBundle] + val w = Wire(new ProbeBundle) + op(d, w) + }) + } + // Check the RWProbe too. + for (op <- rightToLeftConnectOpsNoLegacyBulkConnect) { + check(new RawModule { + val a = IO(Output(Probe(UInt(8.W)))) + val b = IO(Output(RWProbe(UInt(8.W)))) + val c = IO(Output(UInt(8.W))) + val d = (a, b.readOnly, c).viewAs[ProbeBundle] + val w = Wire(new ProbeBundle) + op(d, w) + }) + } + } + + it should "be an error to connect to a read-only property" in { + check(new RawModule { + val p = IO(Output(Property[String]())) + p.readOnly := Property("foo") + }) + } + + it should "be an error to connect to a read-only Bundle containing a property" in { + for (op <- rightToLeftConnectOps) { + check(new RawModule { + val w1 = Wire(new PropertyBundle) + val w2 = IO(new PropertyBundle) + op(w2.readOnly, w1) + }) + } + } + + it should "be an error to connect to a property field of a read-only Bundle" in { + check(new RawModule { + val w = IO(new PropertyBundle).readOnly + w.prop := Property("foo") + }) + } + + it should "be an error to connect to a view containing a read-only property" in { + for (op <- rightToLeftConnectOps) { + check(new RawModule { + val a = IO(Output(UInt(8.W))) + val b = IO(Output(Property[String]())) + val x = (a, b.readOnly).viewAs[PropertyBundle] + val w = Wire(new PropertyBundle) + op(x, w) + }) + } + } +}