Error eagerly when attempting to annotate dynamic indices (#4896)

This is done by adding DynamicIndexBinding so that Chisel can more
precisely track when a Data is part of a dynamic index.

Author: jackkoenig

core/src/main/scala/chisel3/Aggregate.scala

@@ -415,7 +415,7 @@ sealed class Vec[T <: Data] private[chisel3] (gen: => T, val length: Int)
     // TODO port technically isn't directly child of this data structure, but the result of some
     // muxes / demuxes. However, this does make access consistent with the top-level bindings.
     // Perhaps there's a cleaner way of accomplishing this...
-    port.bind(ChildBinding(this), reconstructedResolvedDirection)
+    port.bind(DynamicIndexBinding(this), reconstructedResolvedDirection)
 
     val i = Vec.truncateIndex(p, length)(UnlocatableSourceInfo)
     port.setRef(Node(this), i)

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

@@ -510,6 +510,7 @@ abstract class Data extends HasId with NamedComponent with DataIntf {
       case ElementLitBinding(litArg) => "(unhandled literal)"
       case BundleLitBinding(litMap)  => "(unhandled bundle literal)"
       case VecLitBinding(litMap)     => "(unhandled vec literal)"
+      case DynamicIndexBinding(vec)  => _bindingToString(vec.topBinding)
       case _                         => ""
     }
 
@@ -687,9 +688,10 @@ abstract class Data extends HasId with NamedComponent with DataIntf {
         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
+      case Some(DynamicIndexBinding(vec)) => vec.isVisibleFromModule // Use underlying Vec visibility for dynamic index
+      case Some(pb: SecretPortBinding)    => true // Ignore secret to not require visibility
+      case Some(_: UnconstrainedBinding)  => true
+      case _                              => false
     }
   }
   private[chisel3] def visibleFromBlock: Option[SourceInfo] = MonoConnect.checkBlockVisibility(this)

core/src/main/scala/chisel3/experimental/package.scala

@@ -4,6 +4,7 @@ package chisel3
 
 import chisel3.reflect.DataMirror.internal.chiselTypeClone
 import chisel3.experimental.SourceInfo
+import chisel3.internal.binding.DynamicIndexBinding
 
 /** Package for experimental features, which may have their API changed, be removed, etc.
   *
@@ -66,13 +67,18 @@ package object experimental {
     */
   object requireIsAnnotatable {
     def apply(node: Data, msg: String = ""): Unit = {
+      def prefix = if (msg.nonEmpty) s"$msg " else ""
       requireIsHardware(node, msg)
       if (node.isLit) {
-        val prefix = if (msg.nonEmpty) s"$msg " else ""
         throw ExpectedAnnotatableException(
           s"$prefix'$node' must not be a literal."
         )
       }
+      if (node.topBinding.isInstanceOf[DynamicIndexBinding]) {
+        throw ExpectedAnnotatableException(
+          s"$prefix'$node' must not be a dynamic index into a Vec. Try assigning it to a Wire."
+        )
+      }
     }
   }
 

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

@@ -37,7 +37,9 @@ private[chisel3] object binding {
             case ActualDirection.Input  => Input
             case dir                    => throw new RuntimeException(s"Unexpected port element direction '$dir'")
           }
-        case _ => Internal
+        // Dynamic indices use the direction of the underlying Vec
+        case DynamicIndexBinding(vec) => from(vec.topBinding, direction)
+        case _                        => Internal
       }
     }
   }
@@ -89,6 +91,15 @@ private[chisel3] object binding {
   case class RegBinding(enclosure: RawModule, parentBlock: Option[Block]) extends ConstrainedBinding with BlockBinding
   case class WireBinding(enclosure: RawModule, parentBlock: Option[Block]) extends ConstrainedBinding with BlockBinding
 
+  /** Special binding for Vec dynamic indexing */
+  case class DynamicIndexBinding(vec: Vec[_]) extends ConstrainedBinding with BlockBinding {
+    def parentBlock: Option[Block] = vec.topBinding match {
+      case b: BlockBinding => b.parentBlock
+      case _ => None
+    }
+    def enclosure: BaseModule = vec._parent.get
+  }
+
   case class ClassBinding(enclosure: Class) extends ConstrainedBinding with ReadOnlyBinding
 
   case class ObjectFieldBinding(enclosure: BaseModule) extends ConstrainedBinding

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

@@ -645,6 +645,7 @@ private[chisel3] object Builder extends LazyLogging {
             // And name of the field if we have one, we don't for dynamic indexing of Vecs
             getSubName(data).map(p + "_" + _).getOrElse(p)
           }
+        case DynamicIndexBinding(vec)                                => recData(vec)
         case SampleElementBinding(parent)                            => recData(parent)
         case PortBinding(mod) if Builder.currentModule.contains(mod) => data.seedOpt
         case PortBinding(mod)                                        => map2(mod.seedOpt, data.seedOpt)(_ + "_" + _)

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

@@ -95,6 +95,7 @@ private[chisel3] object MonoConnect {
     x.topBinding match {
       case mp: MemoryPortBinding =>
         None // TODO (albert-magyar): remove this "bridge" for odd enable logic of current CHIRRTL memories
+      case DynamicIndexBinding(vec) => checkBlockVisibility(vec) // This goes with the MemoryPortBinding hack
       case bb: BlockBinding => bb.parentBlock.collect { case b: Block if !visible(b) => b.sourceInfo }
       case _ => None
     }

src/test/scala-2/chiselTests/DontTouchSpec.scala

@@ -116,6 +116,40 @@ class DontTouchSpec extends AnyFlatSpec with Matchers with FileCheck {
     e.getMessage should include("must not be a literal")
   }
 
+  it should "give a decent error when used on dynamic indices" in {
+    val e = the[chisel3.ExpectedAnnotatableException] thrownBy {
+      ChiselStage.emitCHIRRTL(new Module {
+        override def desiredName = "Top"
+        val idx = Wire(UInt(2.W))
+        val vec = Wire(Vec(4, UInt(8.W)))
+        val x = vec(idx)
+        dontTouch(x)
+      })
+    }
+    e.getMessage should include(
+      "Data marked dontTouch 'Top.vec[idx]: Wire[UInt<8>]' must not be a dynamic index into a Vec. Try assigning it to a Wire."
+    )
+  }
+
+  it should "give a decent error when used on subfields of dynamic indices" in {
+    class MyBundle extends Bundle {
+      val a = UInt(8.W)
+      val b = UInt(8.W)
+    }
+    val e = the[chisel3.ExpectedAnnotatableException] thrownBy {
+      ChiselStage.emitCHIRRTL(new Module {
+        override def desiredName = "Top"
+        val idx = Wire(UInt(2.W))
+        val vec = Wire(Vec(4, new MyBundle))
+        val x = vec(idx).a
+        dontTouch(x)
+      })
+    }
+    e.getMessage should include(
+      "Data marked dontTouch 'Top.vec[idx].a: Wire[UInt<8>]' must not be a dynamic index into a Vec. Try assigning it to a Wire."
+    )
+  }
+
   "fields" should "be marked don't touch by default" in {
     ChiselStage
       .emitCHIRRTL(new HasDeadCodeLeaves())

src/test/scala-2/chiselTests/Vec.scala

@@ -7,6 +7,7 @@ import chisel3.util.{Counter, DecoupledIO}
 import circt.stage.ChiselStage
 import org.scalatest.matchers.should.Matchers
 import org.scalatest.propspec.AnyPropSpec
+import chisel3.testing.scalatest.FileCheck
 
 class IOTesterModFill(vecSize: Int) extends Module {
   // This should generate a BindingException when we attempt to wire up the Vec.fill elements
@@ -55,7 +56,7 @@ class ReduceTreeTester extends Module {
   dut.io := DontCare
 }
 
-class VecSpec extends AnyPropSpec with Matchers with LogUtils {
+class VecSpec extends AnyPropSpec with Matchers with LogUtils with FileCheck {
 
   private def uint(value: BigInt): String = uint(value, value.bitLength.max(1))
   private def uint(value: BigInt, width: Int): String =
@@ -475,4 +476,50 @@ class VecSpec extends AnyPropSpec with Matchers with LogUtils {
     chirrtl should include("connect _out_WIRE, UInt<1>(0h0)")
     chirrtl should include("connect out, vec[_out_WIRE]")
   }
+
+  property("It should be legal to dynamically index ports of child modules") {
+    class Child extends RawModule {
+      val in = IO(Input(Vec(4, UInt(8.W))))
+      val out = IO(Output(Vec(4, UInt(8.W))))
+      out := in
+    }
+    class Top extends RawModule {
+      val in = IO(Input(Vec(4, UInt(8.W))))
+      val idx = IO(Input(UInt(2.W)))
+      val out = IO(Output(UInt(8.W)))
+      val child = Module(new Child)
+      child.in := in
+      val index = idx + 1.U // roll-over
+      child.in(index) := 0.U
+      out := child.out(idx)
+    }
+    ChiselStage
+      .emitCHIRRTL(new Top)
+      .fileCheck()(
+        """|CHECK: connect child.in[index], UInt<1>(0h0)
+           |CHECK: connect out, child.out[idx]
+           |""".stripMargin
+      )
+  }
+
+  property("It should be legal to dynamically index the result of a SyncReadMem read") {
+    class Top extends Module {
+      val idx = IO(Input(UInt(2.W)))
+      val jdx = IO(Input(UInt(1.W)))
+      val en = IO(Input(Bool()))
+      val out = IO(Output(UInt(8.W)))
+      val mem = SyncReadMem(4, Vec(2, UInt(8.W)))
+      // SyncReadMem read gets placed in a when block
+      // There is sketchy logic to allow this, make sure it also works for dynamic indexing the mem-port
+      val port = mem.read(idx, en)
+      out := port(jdx)
+    }
+    ChiselStage
+      .emitCHIRRTL(new Top)
+      .fileCheck()(
+        """|CHECK: read mport port = mem[_port_WIRE], clock
+           |CHECK: connect out, port[jdx]
+           |""".stripMargin
+      )
+  }
 }