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.

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

Fix visibility for views #3818

Merged
merged 1 commit into from
Feb 10, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
3 changes: 3 additions & 0 deletions core/src/main/scala/chisel3/Data.scala
Original file line number Diff line number Diff line change
Expand Up @@ -584,12 +584,15 @@ abstract class Data extends HasId with NamedComponent with SourceInfoDoc {

private[chisel3] def isVisible: Boolean = isVisibleFromModule && visibleFromWhen.isEmpty
private[chisel3] def isVisibleFromModule: Boolean = {
val topBindingOpt = this.topBindingOpt // Only call the function once
val mod = topBindingOpt.flatMap(_.location)
topBindingOpt match {
case Some(tb: TopBinding) if (mod == Builder.currentModule) => true
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(pb: SecretPortBinding) => true // Ignore secret to not require visibility
case Some(_: UnconstrainedBinding) => true
case _ => false
Expand Down
27 changes: 25 additions & 2 deletions core/src/main/scala/chisel3/internal/Binding.scala
Original file line number Diff line number Diff line change
Expand Up @@ -124,7 +124,13 @@ private[chisel3] case class MemTypeBinding[T <: Data](parent: MemBase[T]) extend
private[chisel3] case class DontCareBinding() extends UnconstrainedBinding

// Views currently only support 1:1 Element-level mappings
private[chisel3] case class ViewBinding(target: Element) extends UnconstrainedBinding
private[chisel3] case class ViewBinding(target: Element) extends Binding with ConditionalDeclarable {
def location: Option[BaseModule] = target.binding.flatMap(_.location)
def visibility: Option[WhenContext] = target.binding.flatMap {
case c: ConditionalDeclarable => c.visibility
case _ => None
}
}

/** Binding for Aggregate Views
* @param childMap Mapping from children of this view to their respective targets
Expand All @@ -133,9 +139,26 @@ private[chisel3] case class ViewBinding(target: Element) extends UnconstrainedBi
* @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 UnconstrainedBinding {
private[chisel3] case class AggregateViewBinding(childMap: Map[Data, Data]) 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])

// 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
lazy val location: Option[BaseModule] = {
val locations = childMap.values.view.flatMap(_.binding.toSeq.flatMap(_.location)).toVector.distinct
if (locations.size == 1) Some(locations.head)
else None
}
lazy val visibility: Option[WhenContext] = {
val contexts = childMap.values.view
.flatMap(_.binding.toSeq.collect { case c: ConditionalDeclarable => c.visibility }.flatten)
.toVector
.distinct
if (contexts.size == 1) Some(contexts.head)
else None
}
}

/** Binding for Data's returned from accessing an Instance/Definition members, if not readable/writable port */
Expand Down
47 changes: 41 additions & 6 deletions src/test/scala/chiselTests/reflect/DataMirrorSpec.scala
Original file line number Diff line number Diff line change
Expand Up @@ -9,32 +9,60 @@ import chisel3.reflect.DataMirror
import chiselTests.ChiselFlatSpec
import circt.stage.ChiselStage
import chisel3.util.DecoupledIO
import chisel3.experimental.hierarchy._
import chisel3.experimental.dataview._

object DataMirrorSpec {
import org.scalatest.matchers.should.Matchers._
class GrandChild(parent: RawModule) extends Module {
val internal = WireInit(false.B)
DataMirror.getParent(this) should be(Some(parent))
DataMirror.isVisible(internal) should be(true)
DataMirror.isVisible(internal.viewAs[Bool]) should be(true)
}
@instantiable
class Child(parent: RawModule) extends Module {
val inst = Module(new GrandChild(this))
val io = IO(Input(Bool()))
@public val io = IO(Input(Bool()))
val internal = WireInit(false.B)
lazy val underWhen = WireInit(false.B)
when(true.B) {
underWhen := true.B // trigger the lazy val
DataMirror.isVisible(underWhen) should be(true)
DataMirror.isVisible((internal, underWhen).viewAs) should be(true)
}
val mixedView = (io, underWhen).viewAs
DataMirror.getParent(inst) should be(Some(this))
DataMirror.getParent(this) should be(Some(parent))
DataMirror.isVisible(io) should be(true)
DataMirror.isVisible(io.viewAs[Bool]) should be(true)
DataMirror.isVisible(internal) should be(true)
DataMirror.isVisible(internal.viewAs[Bool]) should be(true)
DataMirror.isVisible(inst.internal) should be(false)
DataMirror.isVisible(inst.internal.viewAs[Bool]) should be(false)
DataMirror.isVisible(underWhen) should be(false)
DataMirror.isVisible(underWhen.viewAs) should be(false)
DataMirror.isVisible(mixedView) should be(false)
DataMirror.isVisible(mixedView._1) should be(true)
}
@instantiable
class Parent extends Module {
val inst = Module(new Child(this))
inst.io := false.B
@public val io = IO(Input(Bool()))
@public val inst = Module(new Child(this))
@public val internal = WireInit(io)
@public val tuple = (io, internal).viewAs
inst.io := internal
DataMirror.getParent(inst) should be(Some(this))
DataMirror.getParent(this) should be(None)
DataMirror.isVisible(inst.io) should be(true)
DataMirror.isVisible(inst.io.viewAs[Bool]) should be(true)
DataMirror.isVisible(inst.internal) should be(false)
DataMirror.isVisible(inst.internal.viewAs[Bool]) should be(false)
DataMirror.isVisible(inst.inst.internal) should be(false)
DataMirror.isVisible(inst.inst.internal.viewAs[Bool]) should be(false)
DataMirror.isVisible(inst.mixedView) should be(false)
DataMirror.isVisible(inst.mixedView._1) should be(true)
DataMirror.isVisible(tuple) should be(true)
}
}

Expand Down Expand Up @@ -88,16 +116,23 @@ class DataMirrorSpec extends ChiselFlatSpec {
ChiselStage.emitCHIRRTL(new MyModule)
}

it should "support getParent for normal modules" in {
it should "support getParent and isVisible for normal modules" in {
ChiselStage.emitCHIRRTL(new Parent)
}

it should "support getParent for normal modules even when used in a D/I context" in {
import chisel3.experimental.hierarchy._
it should "support getParent and isVisible for normal modules even when used in a D/I context" in {
class Top extends Module {
val defn = Definition(new Parent)
val inst = Instance(defn)
DataMirror.getParent(this) should be(None)
DataMirror.isVisible(inst.io) should be(true)
DataMirror.isVisible(inst.io.viewAs) should be(true)
DataMirror.isVisible(inst.inst.io) should be(false)
DataMirror.isVisible(inst.inst.io.viewAs) should be(false)
DataMirror.isVisible(inst.internal) should be(false)
DataMirror.isVisible(inst.internal.viewAs) should be(false)
DataMirror.isVisible(inst.tuple) should be(false)
DataMirror.isVisible(inst.tuple._1) should be(true)
}
ChiselStage.emitCHIRRTL(new Top)
}
Expand Down