Fix visibility for views (#3818) (#3821)

(cherry picked from commit 84a21f8)

Co-authored-by: Jack Koenig <koenig@sifive.com>

core/src/main/scala/chisel3/Data.scala

@@ -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

core/src/main/scala/chisel3/internal/Binding.scala

@@ -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
@@ -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 */

src/test/scala/chiselTests/reflect/DataMirrorSpec.scala

@@ -8,32 +8,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)
   }
 }
 
@@ -87,16 +115,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)
   }