Error when blackboxing memories with unsupported masking (#1238)

* Types containing bundles can't generally be converted to a single mask granularity

* Update ReplSeqMemTests to check for illegal masks

2 files changed, 80 insertions, 25 deletions

diff --git a/src/main/scala/firrtl/passes/memlib/ReplaceMemMacros.scala b/src/main/scala/firrtl/passes/memlib/ReplaceMemMacros.scala
index 1b3e18b0..f81dc71b 100644
--- a/src/main/scala/firrtl/passes/memlib/ReplaceMemMacros.scala
+++ b/src/main/scala/firrtl/passes/memlib/ReplaceMemMacros.scala
@@ -16,6 +16,10 @@ import wiring._
 /** Annotates the name of the pins to add for WiringTransform */
 case class PinAnnotation(pins: Seq[String]) extends NoTargetAnnotation
 
+object ReplaceMemMacros {
+  class UnsupportedBlackboxMemoryException(msg: String) extends PassException(msg)
+}
+
 /** Replace DefAnnotatedMemory with memory blackbox + wrapper + conf file.
   * This will not generate wmask ports if not needed.
   * Creates the minimum # of black boxes needed by the design.
@@ -36,17 +40,50 @@ class ReplaceMemMacros(writer: ConfWriter) extends Transform {
   private def rPortToFlattenBundle(mem: DefAnnotatedMemory) = BundleType(
     defaultPortSeq(mem) :+ Field("data", Flip, flattenType(mem.dataType)))
 
+  /** Catch incorrect memory instantiations when there are masked memories with unsupported aggregate types.
+    *
+    * Example:
+    *
+    * val memory = SyncReadMem(N, Vec(M, new Bundle {
+    *   val a = Bool()
+    *   val b = UInt(3.W)
+    * }))
+    *
+    * This memory wrapper will have created M*NUM_BUNDLE_ENTRIES bits or M*2 since createMask matches the
+    * structure of the memory datatype. However, the MemConf output will have
+    * a maskGran of 4b and thus M mask bits (since M*4b is the total mem. width and (M*4b)/4b = M).
+    * Thus, when connecting the blackbox module created from the MemConf file and the FIRRTL wrapper,
+    * there will be a mismatch in port size (M != M*2).
+    */
+  private def checkMaskDatatype(mem: DefAnnotatedMemory) = {
+    mem.dataType match {
+      case VectorType(at: AggregateType, _) =>
+        val msg = s"${mem.info} : Cannot blackbox masked-write memory ${mem.name} with nested aggregate data type."
+        throw new ReplaceMemMacros.UnsupportedBlackboxMemoryException(msg)
+      case BundleType(_) =>
+        val msg = s"${mem.info} : Cannot blackbox masked-write memory ${mem.name} with bundle data type."
+        throw new ReplaceMemMacros.UnsupportedBlackboxMemoryException(msg)
+      case _ =>
+    }
+  }
+
   private def wPortToBundle(mem: DefAnnotatedMemory) = BundleType(
     (defaultPortSeq(mem) :+ Field("data", Default, mem.dataType)) ++ (mem.maskGran match {
       case None => Nil
-      case Some(_) => Seq(Field("mask", Default, createMask(mem.dataType)))
+      case Some(_) => {
+        checkMaskDatatype(mem)
+        Seq(Field("mask", Default, createMask(mem.dataType)))
+      }
     })
   )
   private def wPortToFlattenBundle(mem: DefAnnotatedMemory) = BundleType(
     (defaultPortSeq(mem) :+ Field("data", Default, flattenType(mem.dataType))) ++ (mem.maskGran match {
       case None => Nil
       case Some(_) if getFillWMask(mem) => Seq(Field("mask", Default, flattenType(mem.dataType)))
-      case Some(_) => Seq(Field("mask", Default, flattenType(createMask(mem.dataType))))
+      case Some(_) => {
+        checkMaskDatatype(mem)
+        Seq(Field("mask", Default, flattenType(createMask(mem.dataType))))
+      }
     })
   )
   // TODO(shunshou): Don't use createMask???
@@ -58,7 +95,10 @@ class ReplaceMemMacros(writer: ConfWriter) extends Transform {
       Field("rdata", Flip, mem.dataType)
     ) ++ (mem.maskGran match {
       case None => Nil
-      case Some(_) => Seq(Field("wmask", Default, createMask(mem.dataType)))
+      case Some(_) => {
+        checkMaskDatatype(mem)
+        Seq(Field("wmask", Default, createMask(mem.dataType)))
+      }
     })
   )
   private def rwPortToFlattenBundle(mem: DefAnnotatedMemory) = BundleType(
@@ -69,7 +109,10 @@ class ReplaceMemMacros(writer: ConfWriter) extends Transform {
     ) ++ (mem.maskGran match {
       case None => Nil
       case Some(_) if (getFillWMask(mem)) => Seq(Field("wmask", Default, flattenType(mem.dataType)))
-      case Some(_) => Seq(Field("wmask", Default, flattenType(createMask(mem.dataType))))
+      case Some(_) => {
+        checkMaskDatatype(mem)
+        Seq(Field("wmask", Default, flattenType(createMask(mem.dataType))))
+      }
     })
   )
 
@@ -115,7 +158,7 @@ class ReplaceMemMacros(writer: ConfWriter) extends Transform {
   def defaultConnects(wrapperPort: WRef, bbPort: WSubField): Seq[Connect] =
     Seq("clk", "en", "addr") map (f => connectFields(bbPort, f, wrapperPort, f))
 
-  // Generates mask bits (concatenates an aggregate to ground type) 
+  // Generates mask bits (concatenates an aggregate to ground type)
   // depending on mask granularity (# bits = data width / mask granularity)
   def maskBits(mask: WSubField, dataType: Type, fillMask: Boolean): Expression =
     if (fillMask) toBitMask(mask, dataType) else toBits(mask)
@@ -142,7 +185,7 @@ class ReplaceMemMacros(writer: ConfWriter) extends Transform {
     val wrapperWData = WSubField(wrapperPort, "wdata")
     val defaultSeq = defaultConnects(wrapperPort, bbPort) ++ Seq(
       fromBits(WSubField(wrapperPort, "rdata"), WSubField(bbPort, "rdata")),
-      connectFields(bbPort, "wmode", wrapperPort, "wmode"), 
+      connectFields(bbPort, "wmode", wrapperPort, "wmode"),
       Connect(NoInfo, WSubField(bbPort, "wdata"), toBits(wrapperWData)))
     hasMask match {
       case false => defaultSeq
@@ -174,7 +217,7 @@ class ReplaceMemMacros(writer: ConfWriter) extends Transform {
                      memPortMap: MemPortMap,
                      memMods: Modules)
                      (s: Statement): Statement = s match {
-    case m: DefAnnotatedMemory => 
+    case m: DefAnnotatedMemory =>
       if (m.maskGran.isEmpty) {
         m.writers foreach { w => memPortMap(s"${m.name}.$w.mask") = EmptyExpression }
         m.readwriters foreach { w => memPortMap(s"${m.name}.$w.wmask") = EmptyExpression }
diff --git a/src/test/scala/firrtlTests/ReplSeqMemTests.scala b/src/test/scala/firrtlTests/ReplSeqMemTests.scala
index 9a5a2f72..72171d43 100644
--- a/src/test/scala/firrtlTests/ReplSeqMemTests.scala
+++ b/src/test/scala/firrtlTests/ReplSeqMemTests.scala
@@ -47,10 +47,8 @@ circuit Top :
     input tail_ptr : UInt<5>
     input wmask : {takens : UInt<2>, history : UInt<14>, info : UInt<14>}
     output io : {backend : {flip allocate : {valid : UInt<1>, bits : {info : {takens : UInt<2>, history : UInt<14>, info : UInt<14>}}}}, commit_entry : {valid : UInt<1>, bits : {info : {takens : UInt<2>, history : UInt<14>, info : UInt<14>}}}}
-    output io2 : {backend : {flip allocate : {valid : UInt<1>, bits : {info : {takens : UInt<2>, history : UInt<14>, info : UInt<14>}}}}, commit_entry : {valid : UInt<1>, bits : {info : {takens : UInt<2>, history : UInt<14>, info : UInt<14>}}}}
 
     io is invalid
-    io2 is invalid
 
     smem entries_info : {takens : UInt<2>, history : UInt<14>, info : UInt<14>}[24]
     when io.backend.allocate.valid :
@@ -59,23 +57,9 @@ circuit Top :
 
     read mport R = entries_info[head_ptr], clock
     io.commit_entry.bits.info <- R
-
-    smem entries_info2 : {takens : UInt<2>, history : UInt<14>, info : UInt<14>}[24]
-    when io2.backend.allocate.valid :
-      write mport W1 = entries_info2[tail_ptr], clock
-      when wmask.takens :
-        W1.takens <- io.backend.allocate.bits.info.takens
-      when wmask.history :
-        W1.history <- io.backend.allocate.bits.info.history
-      when wmask.info :
-        W1.info <- io.backend.allocate.bits.info.history
-      
-    read mport R1 = entries_info2[head_ptr], clock
-    io2.commit_entry.bits.info <- R1
 """.stripMargin
     val mems = Set(
-      MemConf("entries_info_ext", 24, 30, Map(WritePort -> 1, ReadPort -> 1), None),
-      MemConf("entries_info2_ext", 24, 30, Map(MaskedWritePort -> 1, ReadPort -> 1), Some(10))
+      MemConf("entries_info_ext", 24, 30, Map(WritePort -> 1, ReadPort -> 1), None)
     )
     val confLoc = "ReplSeqMemTests.confTEMP"
     val annos = Seq(ReplSeqMemAnnotation.parse("-c:Top:-o:"+confLoc))
@@ -509,6 +493,34 @@ circuit NoMemsHere :
     checkMemConf(confLoc, mems)
     (new java.io.File(confLoc)).delete()
   }
+
+ "ReplSeqMem" should "throw an exception when encountering masks with variable granularity" in {
+    val input = """
+circuit Top : 
+  module Top : 
+    input clock : Clock
+    input wmask : {a : UInt<1>, b : UInt<1>}
+    input waddr : UInt<5>
+    input wdata : {a : UInt<8>, b : UInt<6>}
+    input raddr : UInt<5>
+    output rdata : {a : UInt<8>, b : UInt<6>}
+
+    smem testmem : {a : UInt<8>, b : UInt<6>}[32]
+    write mport w = testmem[waddr], clock
+    when wmask.a :
+        w.a <- wdata.a
+    when wmask.b :
+        w.b <- wdata.b
+      
+    read mport r = testmem[raddr], clock
+    rdata <- r
+""".stripMargin
+    intercept[ReplaceMemMacros.UnsupportedBlackboxMemoryException] {
+      val confLoc = "ReplSeqMemTests.confTEMP"
+      val annos = Seq(ReplSeqMemAnnotation.parse("-c:Top:-o:"+confLoc))
+      val res = compileAndEmit(CircuitState(parse(input), ChirrtlForm, annos))
+    }
+  }
+
 }
 
-// TODO: make more checks