From 01b10725163c5bbe239c11f14b6136c737160d34 Mon Sep 17 00:00:00 2001 From: Albert Magyar Date: Wed, 16 Oct 2019 01:33:30 -0700 Subject: Add test for #1179: comb-loops from VerilogMemDelays --- src/test/scala/firrtlTests/MemEnFeedbackSpec.scala | 41 ++++++++++++++++++++++ 1 file changed, 41 insertions(+) create mode 100644 src/test/scala/firrtlTests/MemEnFeedbackSpec.scala (limited to 'src/test') 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() + } +} -- cgit v1.2.3 From 24df1cdcc0981f6e32662a42fe01135681db18c4 Mon Sep 17 00:00:00 2001 From: Albert Magyar Date: Wed, 16 Oct 2019 01:40:34 -0700 Subject: Add library for streamlined Verilog execution tests --- .../execution/ExecutionTestHelper.scala | 112 +++++++++++++++++++++ .../firrtlTests/execution/ParserHelpers.scala | 52 ++++++++++ .../execution/SimpleExecutionTest.scala | 84 ++++++++++++++++ .../firrtlTests/execution/VerilogExecution.scala | 32 ++++++ 4 files changed, 280 insertions(+) create mode 100644 src/test/scala/firrtlTests/execution/ExecutionTestHelper.scala create mode 100644 src/test/scala/firrtlTests/execution/ParserHelpers.scala create mode 100644 src/test/scala/firrtlTests/execution/SimpleExecutionTest.scala create mode 100644 src/test/scala/firrtlTests/execution/VerilogExecution.scala (limited to 'src/test') 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)) + } +} -- cgit v1.2.3 From 24f7d90b032f7058ae379ff3592c9d29c7f987e7 Mon Sep 17 00:00:00 2001 From: Albert Magyar Date: Wed, 16 Oct 2019 01:43:56 -0700 Subject: Add tests for memories with latency >1, toggling enables --- src/test/scala/firrtlTests/MemLatencySpec.scala | 130 ++++++++++++++++++++++++ 1 file changed, 130 insertions(+) create mode 100644 src/test/scala/firrtlTests/MemLatencySpec.scala (limited to 'src/test') 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 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 + 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 -- cgit v1.2.3