Merge pull request #1202 from freechipsproject/fix-verilog-mem-delay-en

Fix handling of read enables for write-first (default) memories in VerilogMemDelays

6 files changed, 451 insertions, 0 deletions

diff --git a/src/test/scala/firrtlTests/MemEnFeedbackSpec.scala b/src/test/scala/firrtlTests/MemEnFeedbackSpec.scala
new file mode 100644
index 00000000..d94d199a
--- /dev/null
+++ b/src/test/scala/firrtlTests/MemEnFeedbackSpec.scala
@@ -0,0 +1,41 @@
+// See LICENSE for license details.
+
+package firrtlTests
+
+import firrtl._
+
+// Tests long-standing bug from #1179, VerilogMemDelays producing combinational loops in corner case
+abstract class MemEnFeedbackSpec extends FirrtlFlatSpec {
+  val ruw: String
+  def input: String =
+    s"""circuit loop :
+       |  module loop :
+       |    input clk : Clock
+       |    input raddr : UInt<5>
+       |    mem m :
+       |      data-type => UInt<1>
+       |      depth => 32
+       |      reader => r
+       |      read-latency => 1
+       |      write-latency => 1
+       |      read-under-write => ${ruw}
+       |    m.r.clk <= clk
+       |    m.r.addr <= raddr
+       |    m.r.en <= m.r.data
+       |""".stripMargin
+  def compileInput(): Unit = (new VerilogCompiler).compileAndEmit(CircuitState(parse(input), ChirrtlForm), List.empty)
+}
+
+class WriteFirstMemEnFeedbackSpec extends MemEnFeedbackSpec {
+  val ruw = "new"
+  "A write-first sync-read mem with feedback from data to enable" should "compile without errors" in {
+    compileInput()
+  }
+}
+
+class ReadFirstMemEnFeedbackSpec extends MemEnFeedbackSpec {
+  val ruw = "old"
+  "A read-first sync-read mem with feedback from data to enable" should "compile without errors" in {
+    compileInput()
+  }
+}
diff --git a/src/test/scala/firrtlTests/MemLatencySpec.scala b/src/test/scala/firrtlTests/MemLatencySpec.scala
new file mode 100644
index 00000000..79986cc2
--- /dev/null
+++ b/src/test/scala/firrtlTests/MemLatencySpec.scala
@@ -0,0 +1,130 @@
+package firrtlTests
+
+import firrtlTests.execution._
+
+object MemLatencySpec {
+  case class Write(addr: Int, data: Int, mask: Option[Boolean] = None)
+  case class Read(addr: Int, expectedValue: Int)
+  case class MemAccess(w: Option[Write], r: Option[Read])
+  def writeOnly(addr: Int, data: Int) = MemAccess(Some(Write(addr, data)), None)
+  def readOnly(addr: Int, expectedValue: Int) = MemAccess(None, Some(Read(addr, expectedValue)))
+}
+
+abstract class MemLatencySpec(rLatency: Int, wLatency: Int, ruw: String)
+    extends SimpleExecutionTest
+    with VerilogExecution {
+
+  import MemLatencySpec._
+
+  require(rLatency >= 0, s"Illegal read-latency ${rLatency} supplied to MemLatencySpec")
+  require(wLatency > 0, s"Illegal write-latency ${wLatency} supplied to MemLatencySpec")
+
+  val body =
+    s"""mem m :
+       |  data-type => UInt<32>
+       |  depth => 256
+       |  reader => r
+       |  writer => w
+       |  read-latency => ${rLatency}
+       |  write-latency => ${wLatency}
+       |  read-under-write => ${ruw}
+       |m.r.clk <= clock
+       |m.w.clk <= clock
+       |""".stripMargin
+
+  val memAccesses: Seq[MemAccess]
+
+  def mask2Poke(m: Option[Boolean]) = m match {
+    case Some(false) => Poke("m.w.mask", 0)
+    case _ => Poke("m.w.mask", 1)
+  }
+
+  def wPokes = memAccesses.map {
+    case MemAccess(Some(Write(a, d, m)), _) =>
+      Seq(Poke("m.w.en", 1), Poke("m.w.addr", a), Poke("m.w.data", d), mask2Poke(m))
+    case _ => Seq(Poke("m.w.en", 0), Invalidate("m.w.addr"), Invalidate("m.w.data"))
+  }
+
+  def rPokes = memAccesses.map {
+    case MemAccess(_, Some(Read(a, _))) => Seq(Poke("m.r.en", 1), Poke("m.r.addr", a))
+    case _ => Seq(Poke("m.r.en", 0), Invalidate("m.r.addr"))
+  }
+
+  // Need to idle for <rLatency> cycles at the end
+  val idle = Seq(Poke("m.w.en", 0), Poke("m.r.en", 0))
+  def pokes = (wPokes zip rPokes).map { case (wp, rp) => wp ++ rp } ++ Seq.fill(rLatency)(idle)
+
+  // Need to delay read value expects by <rLatency>
+  def expects = Seq.fill(rLatency)(Seq(Step(1))) ++ memAccesses.map {
+    case MemAccess(_, Some(Read(_, expected))) => Seq(Expect("m.r.data", expected), Step(1))
+    case _ => Seq(Step(1))
+  }
+
+  def commands: Seq[SimpleTestCommand] = (pokes zip expects).flatMap { case (p, e) => p ++ e }
+}
+
+trait ToggleMaskAndEnable {
+  import MemLatencySpec._
+  /**
+    * A canonical sequence of memory accesses for sanity checking memories of different latencies.
+    * The shortest true "RAW" hazard is reading address 14 two accesses after writing it. Since this
+    * access assumed the new value of 87, this means that the access pattern is only valid for
+    * certain combinations of read- and write-latencies that vary between read- and write-first
+    * memories.
+    *
+    * @note Read-first mems should return expected values for (write-latency <= 2)
+    * @note Write-first mems should return expected values for (write-latency <= read-latency + 2)
+    */
+  val memAccesses: Seq[MemAccess] = Seq(
+    MemAccess(Some(Write(6, 32)),              None),
+    MemAccess(Some(Write(14, 87)),             None),
+    MemAccess(None,                            None),
+    MemAccess(Some(Write(19, 63)),             Some(Read(14, 87))),
+    MemAccess(Some(Write(22, 49)),             None),
+    MemAccess(Some(Write(11, 99)),             Some(Read(6, 32))),
+    MemAccess(Some(Write(42, 42)),             None),
+    MemAccess(Some(Write(77, 81)),             None),
+    MemAccess(Some(Write(6, 7)),               Some(Read(19, 63))),
+    MemAccess(Some(Write(39, 5)),              Some(Read(42, 42))),
+    MemAccess(Some(Write(39, 6, Some(false))), Some(Read(77, 81))), // set mask to zero, should not write
+    MemAccess(None,                            Some(Read(6, 7))),   // also read a twice-written address
+    MemAccess(None,                            Some(Read(39, 5)))   // ensure masked writes didn't happen
+  )
+}
+
+/*
+ *  This framework is for execution tests, so these tests all focus on
+ *  *legal* configurations. Illegal memory parameters that should
+ *  result in errors should be tested in MemSpec.
+ */
+
+// These two are the same in practice, but the two tests could help expose bugs in VerilogMemDelays
+class CombMemSpecNewRUW extends MemLatencySpec(rLatency = 0, wLatency = 1, ruw = "new") with ToggleMaskAndEnable
+class CombMemSpecOldRUW extends MemLatencySpec(rLatency = 0, wLatency = 1, ruw = "old") with ToggleMaskAndEnable
+
+// Odd combination: combinational read with 2-cycle write latency
+class CombMemWL2SpecNewRUW extends MemLatencySpec(rLatency = 0, wLatency = 2, ruw = "new") with ToggleMaskAndEnable
+class CombMemWL2SpecOldRUW extends MemLatencySpec(rLatency = 0, wLatency = 2, ruw = "old") with ToggleMaskAndEnable
+
+// Standard sync read mem
+class WriteFirstMemToggleSpec extends MemLatencySpec(rLatency = 1, wLatency = 1, ruw = "new") with ToggleMaskAndEnable
+class ReadFirstMemToggleSpec extends MemLatencySpec(rLatency = 1, wLatency = 1, ruw = "old") with ToggleMaskAndEnable
+
+// Read latency 2
+class WriteFirstMemToggleSpecRL2 extends MemLatencySpec(rLatency = 2, wLatency = 1, ruw = "new") with ToggleMaskAndEnable
+class ReadFirstMemToggleSpecRL2 extends MemLatencySpec(rLatency = 2, wLatency = 1, ruw = "old") with ToggleMaskAndEnable
+
+// Write latency 2
+class WriteFirstMemToggleSpecWL2 extends MemLatencySpec(rLatency = 1, wLatency = 2, ruw = "new") with ToggleMaskAndEnable
+class ReadFirstMemToggleSpecWL2 extends MemLatencySpec(rLatency = 1, wLatency = 2, ruw = "old") with ToggleMaskAndEnable
+
+// Read latency 2, write latency 2
+class WriteFirstMemToggleSpecRL2WL2 extends MemLatencySpec(rLatency = 2, wLatency = 2, ruw = "new") with ToggleMaskAndEnable
+class ReadFirstMemToggleSpecRL2WL2 extends MemLatencySpec(rLatency = 2, wLatency = 2, ruw = "old") with ToggleMaskAndEnable
+
+// Read latency 3, write latency 2
+class WriteFirstMemToggleSpecRL3WL2 extends MemLatencySpec(rLatency = 3, wLatency = 2, ruw = "new") with ToggleMaskAndEnable
+class ReadFirstMemToggleSpecRL3WL2 extends MemLatencySpec(rLatency = 3, wLatency = 2, ruw = "old") with ToggleMaskAndEnable
+
+// Read latency 2, write latency 4 -> ToggleSpec pattern only valid for write-first at this combo
+class WriteFirstMemToggleSpecRL2WL4 extends MemLatencySpec(rLatency = 2, wLatency = 4, ruw = "new") with ToggleMaskAndEnable
diff --git a/src/test/scala/firrtlTests/execution/ExecutionTestHelper.scala b/src/test/scala/firrtlTests/execution/ExecutionTestHelper.scala
new file mode 100644
index 00000000..7d250664
--- /dev/null
+++ b/src/test/scala/firrtlTests/execution/ExecutionTestHelper.scala
@@ -0,0 +1,112 @@
+package firrtlTests.execution
+
+import firrtl._
+import firrtl.ir._
+
+object DUTRules {
+  val dutName = "dut"
+  val clock = Reference("clock", ClockType)
+  val reset = Reference("reset", Utils.BoolType)
+  val counter = Reference("step", UnknownType)
+
+  // Need a flat name for the register that latches poke values
+  val illegal = raw"[\[\]\.]".r
+  val pokeRegSuffix = "_poke"
+  def pokeRegName(e: Expression) = illegal.replaceAllIn(e.serialize, "_") + pokeRegSuffix
+
+  // Naming patterns are static, so DUT has to be checked for proper form + collisions
+  def hasNameConflicts(c: Circuit): Boolean = {
+    val top = c.modules.find(_.name == c.main).get
+    val names = Namespace(top).cloneUnderlying
+    names.contains(counter.name) || names.exists(_.contains(pokeRegSuffix))
+  }
+}
+
+object ExecutionTestHelper {
+  val counterType = UIntType(IntWidth(32))
+  def apply(body: String): ExecutionTestHelper = {
+    // Parse input and check that it complies with test syntax rules
+    val c = ParseStatement.makeDUT(body)
+    require(!DUTRules.hasNameConflicts(c), "Avoid using 'step' or 'poke' in DUT component names")
+
+    // Generate test step counter, create ExecutionTestHelper that represents initial test state
+    val cnt = DefRegister(NoInfo, DUTRules.counter.name, counterType, DUTRules.clock, DUTRules.reset, Utils.zero)
+    val inc = Connect(NoInfo, DUTRules.counter, DoPrim(PrimOps.Add, Seq(DUTRules.counter, UIntLiteral(1)), Nil, UnknownType))
+    ExecutionTestHelper(c, Seq(cnt, inc), Map.empty[Expression, Expression], Nil, Nil)
+  }
+}
+
+case class ExecutionTestHelper(
+  dut: Circuit,
+  setup: Seq[Statement],
+  pokeRegs: Map[Expression, Expression],
+  completedSteps: Seq[Conditionally],
+  activeStep: Seq[Statement]
+) {
+
+  def step(n: Int): ExecutionTestHelper = {
+    require(n > 0, "Step length must be positive")
+    (0 until n).foldLeft(this) { case (eth, int) => eth.next }
+  }
+
+  def poke(expString: String, value: Literal): ExecutionTestHelper = {
+    val pokeExp = ParseExpression(expString)
+    val pokeable = ensurePokeable(pokeExp)
+    pokeable.addStatements(
+      Connect(NoInfo, pokeExp, value),
+      Connect(NoInfo, pokeable.pokeRegs(pokeExp), value))
+  }
+
+  def invalidate(expString: String): ExecutionTestHelper = {
+    addStatements(IsInvalid(NoInfo, ParseExpression(expString)))
+  }
+
+  def expect(expString: String, value: Literal): ExecutionTestHelper = {
+    val peekExp = ParseExpression(expString)
+    val neq = DoPrim(PrimOps.Neq, Seq(peekExp, value), Nil, Utils.BoolType)
+    addStatements(Stop(NoInfo, 1, DUTRules.clock, neq))
+  }
+
+  def finish(): ExecutionTestHelper = {
+    addStatements(Stop(NoInfo, 0, DUTRules.clock, Utils.one)).next
+  }
+
+  // Private helper methods
+
+  private def t = completedSteps.length
+
+  private def addStatements(stmts: Statement*) = copy(activeStep = activeStep ++ stmts)
+
+  private def next: ExecutionTestHelper = {
+    val count = Reference(DUTRules.counter.name, DUTRules.counter.tpe)
+    val ifStep = DoPrim(PrimOps.Eq, Seq(count, UIntLiteral(t)), Nil, Utils.BoolType)
+    val onThisStep = Conditionally(NoInfo, ifStep, Block(activeStep), EmptyStmt)
+    copy(completedSteps = completedSteps :+ onThisStep, activeStep = Nil)
+  }
+
+  private def top: Module = {
+    dut.modules.collectFirst({ case m: Module if m.name == dut.main  => m }).get
+  }
+
+  private[execution] def emit: Circuit = {
+    val finished = finish()
+    val modulesX = dut.modules.collect {
+      case m: Module if m.name == dut.main =>
+        m.copy(body = Block(m.body +: (setup ++ finished.completedSteps)))
+      case m => m
+    }
+    dut.copy(modules = modulesX)
+  }
+
+  private def ensurePokeable(pokeExp: Expression): ExecutionTestHelper = {
+    if (pokeRegs.contains(pokeExp)) {
+      this
+    } else {
+      val pName = DUTRules.pokeRegName(pokeExp)
+      val pRef = Reference(pName, UnknownType)
+      val pReg = DefRegister(NoInfo, pName, UIntType(UnknownWidth), DUTRules.clock, Utils.zero, pRef)
+      val defaultConn = Connect(NoInfo, pokeExp, pRef)
+      copy(setup = setup ++ Seq(pReg, defaultConn), pokeRegs = pokeRegs + (pokeExp -> pRef))
+    }
+  }
+}
diff --git a/src/test/scala/firrtlTests/execution/ParserHelpers.scala b/src/test/scala/firrtlTests/execution/ParserHelpers.scala
new file mode 100644
index 00000000..3472c19c
--- /dev/null
+++ b/src/test/scala/firrtlTests/execution/ParserHelpers.scala
@@ -0,0 +1,52 @@
+package firrtlTests.execution
+
+import firrtl._
+import firrtl.ir._
+
+class ParserHelperException(val pe: ParserException, input: String)
+    extends FirrtlUserException(s"Got error ${pe.toString} while parsing input:\n${input}")
+
+/**
+  * A utility class that parses a FIRRTL string representing a statement to a sub-AST
+  */
+object ParseStatement {
+  private def wrapStmtStr(stmtStr: String): String = {
+    val indent = "    "
+    val indented = stmtStr.split("\n").mkString(indent, s"\n${indent}", "")
+    s"""circuit ${DUTRules.dutName} :
+         |  module ${DUTRules.dutName} :
+         |    input clock : Clock
+         |    input reset : UInt<1>
+         |${indented}""".stripMargin
+  }
+
+  private def parse(stmtStr: String): Circuit = {
+    try {
+      Parser.parseString(wrapStmtStr(stmtStr), Parser.IgnoreInfo)
+    } catch {
+      case e: ParserException => throw new ParserHelperException(e, stmtStr)
+    }
+  }
+
+  def apply(stmtStr: String): Statement = {
+    val c = parse(stmtStr)
+    val stmt = c.modules.collectFirst { case Module(_, _, _, b: Block) => b.stmts.head }
+    stmt.get
+  }
+
+  private[execution] def makeDUT(body: String): Circuit = parse(body)
+}
+
+/**
+  * A utility class that parses a FIRRTL string representing an expression to a sub-AST
+  */
+object ParseExpression {
+  def apply(expStr: String): Expression = {
+    try {
+      val s = ParseStatement(s"${expStr} is invalid")
+      s.asInstanceOf[IsInvalid].expr
+    } catch {
+      case e: ParserHelperException => throw new ParserHelperException(e.pe, expStr)
+    }
+  }
+}
diff --git a/src/test/scala/firrtlTests/execution/SimpleExecutionTest.scala b/src/test/scala/firrtlTests/execution/SimpleExecutionTest.scala
new file mode 100644
index 00000000..5abeb819
--- /dev/null
+++ b/src/test/scala/firrtlTests/execution/SimpleExecutionTest.scala
@@ -0,0 +1,84 @@
+package firrtlTests.execution
+
+import java.io.File
+
+import firrtl.ir._
+import firrtlTests._
+
+sealed trait SimpleTestCommand
+case class Step(n: Int) extends SimpleTestCommand
+case class Invalidate(expStr: String) extends SimpleTestCommand
+case class Poke(expStr: String, value: Int) extends SimpleTestCommand
+case class Expect(expStr: String, value: Int) extends SimpleTestCommand
+
+/**
+  * This trait defines an interface to run a self-contained test circuit.
+  */
+trait TestExecution {
+  def runEmittedDUT(c: Circuit, testDir: File): Unit
+}
+
+/**
+  * A class that makes it easier to write execution-driven tests.
+  * 
+  * By combining a DUT body (supplied as a string without an enclosing
+  * module or circuit) with a sequence of test operations, an
+  * executable, self-contained Verilog testbench may be automatically
+  * created and checked.
+  * 
+  * @note It is necessary to mix in a trait extending TestExecution
+  * @note The DUT has two implicit ports, "clock" and "reset"
+  * @note Execution of the command sequences begins after reset is deasserted
+  * 
+  * @see [[firrtlTests.execution.TestExecution]]
+  * @see [[firrtlTests.execution.VerilogExecution]]
+  * 
+  * @example {{{
+  * class AndTester extends SimpleExecutionTest with VerilogExecution {
+  *   val body = "reg r : UInt<32>, clock with: (reset => (reset, UInt<32>(0)))"
+  *   val commands = Seq(
+  *     Expect("r", 0),
+  *     Poke("r", 3),
+  *     Step(1),
+  *     Expect("r", 3)
+  *   )
+  * }
+  * }}}
+  */
+abstract class SimpleExecutionTest extends FirrtlPropSpec {
+  this: TestExecution =>
+
+  /**
+    * Text representing the body of the DUT. This is useful for testing
+    * statement-level language features, and cuts out the overhead of
+    * writing a top-level DUT module and having peeks/pokes point at
+    * IOs.
+    */
+  val body: String
+
+  /**
+    * A sequence of commands (peeks, pokes, invalidates, steps) that
+    * represents how the testbench will progress. The semantics are
+    * inspired by chisel-testers.
+    */
+  def commands: Seq[SimpleTestCommand]
+
+  private def interpretCommand(eth: ExecutionTestHelper, cmd: SimpleTestCommand) = cmd match {
+    case Step(n) => eth.step(n)
+    case Invalidate(expStr) => eth.invalidate(expStr)
+    case Poke(expStr, value) => eth.poke(expStr, UIntLiteral(value))
+    case Expect(expStr, value) => eth.expect(expStr, UIntLiteral(value))
+  }
+
+  private def runTest(): Unit = {
+    val initial = ExecutionTestHelper(body)
+    val test = commands.foldLeft(initial)(interpretCommand(_, _))
+    val testName = this.getClass.getSimpleName
+    val testDir = createTestDirectory(s"${testName}-generated-src")
+    runEmittedDUT(test.emit, testDir)
+  }
+
+  property("Execution of the compiled Verilog for ExecutionTestHelper should succeed") {
+    runTest()
+  }
+}
diff --git a/src/test/scala/firrtlTests/execution/VerilogExecution.scala b/src/test/scala/firrtlTests/execution/VerilogExecution.scala
new file mode 100644
index 00000000..17eecc65
--- /dev/null
+++ b/src/test/scala/firrtlTests/execution/VerilogExecution.scala
@@ -0,0 +1,32 @@
+package firrtlTests.execution
+
+import java.io.File
+
+import firrtl._
+import firrtl.ir._
+import firrtlTests._
+
+import firrtl.stage.{FirrtlCircuitAnnotation, FirrtlStage}
+import firrtl.options.TargetDirAnnotation
+
+/**
+  * Mixing in this trait causes a SimpleExecutionTest to be run in Verilog simulation.
+  */
+trait VerilogExecution extends TestExecution {
+  this: SimpleExecutionTest =>
+  def runEmittedDUT(c: Circuit, testDir: File): Unit = {
+    // Run FIRRTL, emit Verilog file
+    val cAnno = FirrtlCircuitAnnotation(c)
+    val tdAnno = TargetDirAnnotation(testDir.getAbsolutePath)
+    (new FirrtlStage).run(AnnotationSeq(Seq(cAnno, tdAnno)))
+
+    // Copy harness resource to test directory
+    val harness = new File(testDir, s"top.cpp")
+    copyResourceToFile(cppHarnessResourceName, harness)
+
+    // Make and run Verilog simulation
+    verilogToCpp(c.main, testDir, Nil, harness).!
+    cppToExe(c.main, testDir).!
+    assert(executeExpectingSuccess(c.main, testDir))
+  }
+}