Fix visibility for views (backport #3818)

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

@@ -594,15 +594,88 @@ abstract class Data extends HasId with NamedComponent with SourceInfoDoc {
     */
   private[chisel3] def typeEquivalent(that: Data): Boolean
 
+<<<<<<< HEAD
   private[chisel3] def requireVisible(): Unit = {
+=======
+  /** Find and report any type mismatches
+    *
+    * @param that Data being compared to this
+    * @param strictTypes Does class of Bundles or Records need to match? Inverse of "structural".
+    * @param strictWidths do widths need to match?
+    * @return None if types are equivalent, Some String reporting the first mismatch if not
+    */
+  private[chisel3] final def findFirstTypeMismatch(
+    that:         Data,
+    strictTypes:  Boolean,
+    strictWidths: Boolean
+  ): Option[String] = {
+    def rec(left: Data, right: Data): Option[String] =
+      (left, right) match {
+        // Careful, EnumTypes are Element and if we don't implement this, then they are all always equal
+        case (e1: EnumType, e2: EnumType) =>
+          // TODO, should we implement a form of structural equality for enums?
+          if (e1.factory == e2.factory) None
+          else Some(s": Left ($e1) and Right ($e2) have different types.")
+        // Properties should be considered equal when getPropertyType is equal, not when getClass is equal.
+        case (p1: Property[_], p2: Property[_]) =>
+          if (p1.getPropertyType != p2.getPropertyType) {
+            Some(s": Left ($p1) and Right ($p2) have different types")
+          } else {
+            None
+          }
+        case (e1: Element, e2: Element) if e1.getClass == e2.getClass =>
+          if (strictWidths && e1.width != e2.width) {
+            Some(s": Left ($e1) and Right ($e2) have different widths.")
+          } else {
+            None
+          }
+        case (r1: Record, r2: Record) if !strictTypes || r1.getClass == r2.getClass =>
+          val (larger, smaller, msg) =
+            if (r1._elements.size >= r2._elements.size) (r1, r2, "Left") else (r2, r1, "Right")
+          larger._elements.collectFirst {
+            case (name, data) =>
+              val recurse = smaller._elements.get(name) match {
+                case None        => Some(s": Dangling field on $msg")
+                case Some(data2) => rec(data, data2)
+              }
+              recurse.map("." + name + _)
+          }.flatten
+        case (v1: Vec[_], v2: Vec[_]) =>
+          if (v1.size != v2.size) {
+            Some(s": Left (size ${v1.size}) and Right (size ${v2.size}) have different lengths.")
+          } else {
+            val recurse = rec(v1.sample_element, v2.sample_element)
+            recurse.map("[_]" + _)
+          }
+        case _ => Some(s": Left ($left) and Right ($right) have different types.")
+      }
+    val leftType = if (this.hasBinding) this.cloneType else this
+    val rightType = if (that.hasBinding) that.cloneType else that
+    rec(leftType, rightType)
+  }
+
+  private[chisel3] def isVisible: Boolean = isVisibleFromModule && visibleFromWhen.isEmpty
+  private[chisel3] def isVisibleFromModule: Boolean = {
+    val topBindingOpt = this.topBindingOpt // Only call the function once
+>>>>>>> 84a21f8a7 (Fix visibility for views (#3818))
     val mod = topBindingOpt.flatMap(_.location)
     topBindingOpt match {
       case Some(tb: TopBinding) if (mod == Builder.currentModule) =>
       case Some(pb: PortBinding)
+<<<<<<< HEAD
           if (mod.flatMap(Builder.retrieveParent(_, Builder.currentModule.get)) == Builder.currentModule) =>
       case Some(_: UnconstrainedBinding) =>
       case _ =>
         throwException(s"operand '$this' is not visible from the current module")
+=======
+          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
+>>>>>>> 84a21f8a7 (Fix visibility for views (#3818))
     }
     MonoConnect.checkWhenVisibility(this) match {
       case Some(sourceInfo) =>

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

@@ -129,7 +129,13 @@ case class MemTypeBinding[T <: Data](parent: MemBase[T]) extends Binding {
 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
@@ -138,9 +144,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

@@ -6,21 +6,77 @@ import chisel3._
 import chisel3.reflect.DataMirror
 import chiselTests.ChiselFlatSpec
 import circt.stage.ChiselStage
+<<<<<<< HEAD
+=======
+import chisel3.util.DecoupledIO
+import chisel3.experimental.hierarchy._
+import chisel3.experimental.dataview._
+>>>>>>> 84a21f8a7 (Fix visibility for views (#3818))
 
 object DataMirrorSpec {
   import org.scalatest.matchers.should.Matchers._
   class GrandChild(parent: RawModule) extends Module {
     DataMirror.getParent(this) should be(Some(parent))
+<<<<<<< HEAD
+=======
+    DataMirror.isVisible(internal) should be(true)
+    DataMirror.isVisible(internal.viewAs[Bool]) should be(true)
+>>>>>>> 84a21f8a7 (Fix visibility for views (#3818))
   }
+  @instantiable
   class Child(parent: RawModule) extends Module {
     val inst = Module(new GrandChild(this))
+<<<<<<< HEAD
     DataMirror.getParent(inst) should be(Some(this))
     DataMirror.getParent(this) should be(Some(parent))
+=======
+    @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)
+>>>>>>> 84a21f8a7 (Fix visibility for views (#3818))
   }
+  @instantiable
   class Parent extends Module {
+<<<<<<< HEAD
     val inst = Module(new Child(this))
     DataMirror.getParent(inst) should be(Some(this))
     DataMirror.getParent(this) should be(None)
+=======
+    @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)
+>>>>>>> 84a21f8a7 (Fix visibility for views (#3818))
   }
 }
 
@@ -74,16 +130,28 @@ class DataMirrorSpec extends ChiselFlatSpec {
     ChiselStage.elaborate(new MyModule)
   }
 
+<<<<<<< HEAD
   it should "support getParent for normal modules" in {
     ChiselStage.elaborate(new Parent)
+=======
+  it should "support getParent and isVisible for normal modules" in {
+    ChiselStage.emitCHIRRTL(new Parent)
+>>>>>>> 84a21f8a7 (Fix visibility for views (#3818))
   }
 
-  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.elaborate(new Top)
   }