Fix VerilogMemDelays use before declaration (#2278)

The pass injects pipe registers immediately after the declaration of the
memory. This can be problematic if the clock for the associated memory
port is defined after the declaration of the memory. For any memory port
clocks that are driven by non-ports, we now inject a wire before the
pipe register declarations to be sure there are no
use-before-declaration issues.

2 files changed, 92 insertions, 9 deletions

diff --git a/src/main/scala/firrtl/passes/memlib/VerilogMemDelays.scala b/src/main/scala/firrtl/passes/memlib/VerilogMemDelays.scala
index a9b42eba..11184e60 100644
--- a/src/main/scala/firrtl/passes/memlib/VerilogMemDelays.scala
+++ b/src/main/scala/firrtl/passes/memlib/VerilogMemDelays.scala
@@ -56,6 +56,14 @@ object MemDelayAndReadwriteTransformer {
   private val metaChars = raw"[\[\]\.]".r
   private def flatName(e: Expression) = metaChars.replaceAllIn(e.serialize, "_")
 
+  /** Determines if all `RefLikeExpression` in this Expression are of kind [[PortKind]] */
+  def allPortKinds(expr: Expression): Boolean = expr match {
+    case reflike: RefLikeExpression => kind(expr) == PortKind
+    case other =>
+      other.foreach { (e: Expression) => if (!allPortKinds(e)) return false } // Early out
+      true
+  }
+
   // Pipeline a group of signals with an associated valid signal. Gate registers when possible.
   def pipelineWithValid(
     ns:           Namespace
@@ -126,12 +134,28 @@ class MemDelayAndReadwriteTransformer(m: DefModule, passthroughSimpleSyncReadMem
       val rCmdDelay = if (mem.readUnderWrite == ReadUnderWrite.Old) 0 else mem.readLatency
       val rRespDelay = if (mem.readUnderWrite == ReadUnderWrite.Old) mem.readLatency else 0
       val wCmdDelay = mem.writeLatency - 1
+      val clockWires = new mutable.LinkedHashMap[WrappedExpression, (DefWire, Connect, Reference)]
+      // Memory port clocks may depend on something defined after the memory, create wires for clock
+      // Expressions that contain non-port references
+      def maybeInsertWire(expr: Expression): Expression = {
+        if (allPortKinds(expr)) expr
+        else {
+          def mkWire() = {
+            val wire = DefWire(NoInfo, ns.newName(s"${mem.name}_clock"), expr.tpe)
+            val ref = Reference(wire).copy(flow = SourceFlow)
+            val con = Connect(NoInfo, ref.copy(flow = SinkFlow), expr)
+            (wire, con, ref)
+          }
+          clockWires.getOrElseUpdate(we(expr), mkWire())._3
+        }
+      }
 
       val readStmts = (mem.readers ++ mem.readwriters).map {
         case r =>
           def oldDriver(f: String) = swapMemRefs(netlist(we(memPortField(mem, r, f))))
           def newField(f:  String) = memPortField(newMem, rMap.getOrElse(r, r), f)
-          val clk = oldDriver("clk")
+          // The memory port clock could depend on something defined after the memory
+          val clk = maybeInsertWire(oldDriver("clk"))
 
           // Pack sources of read command inputs into WithValid object -> different for readwriter
           val enSrc = if (rMap.contains(r)) AND(oldDriver("en"), NOT(oldDriver("wmode"))) else oldDriver("en")
@@ -160,7 +184,8 @@ class MemDelayAndReadwriteTransformer(m: DefModule, passthroughSimpleSyncReadMem
         case w =>
           def oldDriver(f: String) = swapMemRefs(netlist(we(memPortField(mem, w, f))))
           def newField(f:  String) = memPortField(newMem, wMap.getOrElse(w, w), f)
-          val clk = oldDriver("clk")
+          // The memory port clock could depend on something defined after the memory
+          val clk = maybeInsertWire(oldDriver("clk"))
 
           // Pack sources of write command inputs into WithValid object -> different for readwriter
           val cmdSrc = if (wMap.contains(w)) {
@@ -180,8 +205,9 @@ class MemDelayAndReadwriteTransformer(m: DefModule, passthroughSimpleSyncReadMem
           SplitStatements(cmdDecls, cmdConns ++ cmdPortConns)
       }
 
+      newConns ++= clockWires.values.map(_._2)
       newConns ++= (readStmts ++ writeStmts).flatMap(_.conns)
-      Block(newMem +: (readStmts ++ writeStmts).flatMap(_.decls))
+      Block(newMem +: clockWires.values.map(_._1) ++: (readStmts ++ writeStmts).flatMap(_.decls))
     case sx: Connect if kind(sx.loc) == MemKind =>
       val (memRef, _) = Utils.splitRef(sx.loc)
       // Filter old mem connections for *transformed* memories only
diff --git a/src/test/scala/firrtlTests/VerilogMemDelaySpec.scala b/src/test/scala/firrtlTests/VerilogMemDelaySpec.scala
index 7611dec9..32b1c55d 100644
--- a/src/test/scala/firrtlTests/VerilogMemDelaySpec.scala
+++ b/src/test/scala/firrtlTests/VerilogMemDelaySpec.scala
@@ -3,8 +3,9 @@
 package firrtlTests
 
 import firrtl._
-import firrtl.passes.memlib.VerilogMemDelays
-import firrtl.passes.CheckHighForm
+import firrtl.testutils._
+import firrtl.testutils.FirrtlCheckers._
+import firrtl.ir.Circuit
 import firrtl.stage.{FirrtlCircuitAnnotation, FirrtlSourceAnnotation, FirrtlStage}
 
 import org.scalatest.freespec.AnyFreeSpec
@@ -12,12 +13,20 @@ import org.scalatest.matchers.should.Matchers
 
 class VerilogMemDelaySpec extends AnyFreeSpec with Matchers {
 
-  private def compileTwice(input: String): Unit = {
-    val result1 = (new FirrtlStage).transform(Seq(FirrtlSourceAnnotation(input))).toSeq.collectFirst {
-      case fca: FirrtlCircuitAnnotation => (new FirrtlStage).transform(Seq(fca))
-    }
+  private def compileTwiceReturnFirst(input: String): Circuit = {
+    (new FirrtlStage)
+      .transform(Seq(FirrtlSourceAnnotation(input)))
+      .toSeq
+      .collectFirst {
+        case fca: FirrtlCircuitAnnotation =>
+          (new FirrtlStage).transform(Seq(fca))
+          fca.circuit
+      }
+      .get
   }
 
+  private def compileTwice(input: String): Unit = compileTwiceReturnFirst(input)
+
   "The following low FIRRTL should be parsed by VerilogMemDelays" in {
     val input =
       """
@@ -141,4 +150,52 @@ class VerilogMemDelaySpec extends AnyFreeSpec with Matchers {
 
     compileTwice(input)
   }
+
+  "VerilogMemDelays should not violate use before declaration of clocks" in {
+    val input =
+      """
+        |circuit Test :
+        |  extmodule ClockMaker :
+        |    input in : UInt<8>
+        |    output clock : Clock
+        |  module Test :
+        |    input clock : Clock
+        |    input addr : UInt<5>
+        |    input mask : UInt<8>
+        |    input in : UInt<8>
+        |    output out : UInt<8>
+        |    mem m :
+        |      data-type => UInt<8>
+        |      depth => 32
+        |      read-latency => 1
+        |      write-latency => 2
+        |      reader => read
+        |      writer => write
+        |      read-under-write => old  ; this is important
+        |    inst cm of ClockMaker
+        |    m.read.clk <= cm.clock
+        |    m.read.en <= UInt<1>(1)
+        |    m.read.addr <= addr
+        |    out <= m.read.data
+        |    ; This makes this really funky for injected pipe ordering
+        |    node read = not(m.read.data)
+        |    cm.in <= and(read, UInt<8>("hf0"))
+        |
+        |    m.write.clk <= clock
+        |    m.write.en <= UInt<1>(1)
+        |    m.write.mask <= mask
+        |    m.write.addr <= addr
+        |    m.write.data <= in
+      """.stripMargin
+
+    val res = compileTwiceReturnFirst(input).serialize
+    // Inject a Wire when using a clock not derived from ports
+    res should include("wire m_clock : Clock")
+    res should include("m_clock <= cm.clock")
+    res should include("reg m_read_data_pipe_0 : UInt<8>, m_clock")
+    res should include("m.read.clk <= m_clock")
+    // No need to insert Wire when clock is derived from a port
+    res should include("m.write.clk <= clock")
+    res should include("reg m_write_data_pipe_0 : UInt<8>, clock")
+  }
 }