From c98ee33827d21f88911f2ca1a6e04bcbb3864fe8 Mon Sep 17 00:00:00 2001 From: Kevin Laeufer Date: Wed, 19 Jan 2022 11:42:25 -0800 Subject: preset: make PropagatePreset play nice with verification statements (#2453) Verification statements are guarded by reset. If this reset happens to be a "preset" type reset, they should always be active. The easiest way to achieve that is to replace all uses of "preset" resets with zero.--- .../transforms/PropagatePresetAnnotations.scala | 25 +++-- src/test/scala/firrtlTests/PresetSpec.scala | 109 ++++++++++++++++----- 2 files changed, 99 insertions(+), 35 deletions(-) (limited to 'src') diff --git a/src/main/scala/firrtl/transforms/PropagatePresetAnnotations.scala b/src/main/scala/firrtl/transforms/PropagatePresetAnnotations.scala index f9b635d4..802050a2 100644 --- a/src/main/scala/firrtl/transforms/PropagatePresetAnnotations.scala +++ b/src/main/scala/firrtl/transforms/PropagatePresetAnnotations.scala @@ -11,8 +11,10 @@ import firrtl.options.Dependency import scala.collection.mutable object PropagatePresetAnnotations { + @deprecated("This message will be removed in the next release.", "FIRRTL 1.5") val advice = "Please Note that a Preset-annotated AsyncReset shall NOT be casted to other types with any of the following functions: asInterval, asUInt, asSInt, asClock, asFixedPoint, asAsyncReset." + @deprecated("This exception will no longer be thrown.", "FIRRTL 1.5") case class TreeCleanUpOrphanException(message: String) extends FirrtlUserException(s"Node left an orphan during tree cleanup: $message $advice") } @@ -366,15 +368,22 @@ class PropagatePresetAnnotations extends Transform with DependencyAPIMigration { } } + // replaces all references to removed (clean up) preset signals with zero + def replaceCleanUpRefs(e: ir.Expression): ir.Expression = e match { + case r: ir.RefLikeExpression => + getRef(r) match { + case rt: ReferenceTarget if toCleanUp.contains(rt) => + Utils.getGroundZero(r.tpe.asInstanceOf[ir.GroundType]) + case _ => r.mapExpr(replaceCleanUpRefs) + } + case other => other.mapExpr(replaceCleanUpRefs) + } + def processNode(n: DefNode): Statement = { if (toCleanUp.contains(moduleTarget.ref(n.name))) { EmptyStmt } else { - getRef(n.value) match { - case rt: ReferenceTarget if (toCleanUp.contains(rt)) => - throw TreeCleanUpOrphanException(s"Orphan (${moduleTarget.ref(n.name)}) the way.") - case _ => n - } + n } } @@ -383,7 +392,7 @@ class PropagatePresetAnnotations extends Transform with DependencyAPIMigration { case rhs: ReferenceTarget if (toCleanUp.contains(rhs)) => getRef(c.loc) match { case lhs: ReferenceTarget if (!toCleanUp.contains(lhs)) => - throw TreeCleanUpOrphanException(s"Orphan ${lhs} connected deleted node $rhs.") + c case _ => EmptyStmt } case _ => c @@ -402,14 +411,14 @@ class PropagatePresetAnnotations extends Transform with DependencyAPIMigration { } def processStatements(statement: Statement): Statement = { - statement match { + (statement match { case i: WDefInstance => processInstance(i) case r: DefRegister => processRegister(r) case w: DefWire => processWire(w) case n: DefNode => processNode(n) case c: Connect => processConnect(c) case s => s.mapStmt(processStatements) - } + }).mapExpr(replaceCleanUpRefs) } m match { diff --git a/src/test/scala/firrtlTests/PresetSpec.scala b/src/test/scala/firrtlTests/PresetSpec.scala index 369a2776..d967aa65 100644 --- a/src/test/scala/firrtlTests/PresetSpec.scala +++ b/src/test/scala/firrtlTests/PresetSpec.scala @@ -6,15 +6,15 @@ import firrtl._ import firrtl.annotations._ import firrtl.testutils._ import firrtl.testutils.FirrtlCheckers._ +import logger.{LogLevel, LogLevelAnnotation, Logger} -class PresetSpec extends FirrtlFlatSpec { +class PresetSpec extends VerilogTransformSpec { type Mod = Seq[String] type ModuleSeq = Seq[Mod] - def compile(input: String, annos: AnnotationSeq): CircuitState = - (new VerilogCompiler).compileAndEmit(CircuitState(parse(input), ChirrtlForm, annos), List.empty) + def compileBody(modules: ModuleSeq) = { val annos = - Seq(new PresetAnnotation(CircuitTarget("Test").module("Test").ref("reset")), firrtl.transforms.NoDCEAnnotation) + Seq(PresetAnnotation(CircuitTarget("Test").module("Test").ref("reset")), firrtl.transforms.NoDCEAnnotation) var str = """ |circuit Test : |""".stripMargin @@ -25,7 +25,10 @@ class PresetSpec extends FirrtlFlatSpec { str += """ |""".stripMargin }) - compile(str, annos) + val logLevel = LogLevel.Warn + Logger.makeScope(Seq(LogLevelAnnotation(logLevel))) { + compile(str, annos) + } } "Preset" should """behave properly given a `Preset` annotated `AsyncReset` INPUT reset: @@ -74,6 +77,7 @@ class PresetSpec extends FirrtlFlatSpec { ) ) ) + result shouldNot containLine("always @(posedge clock or posedge reset) begin") result shouldNot containLines("if (reset) begin", "r = 1'h0;", "end") result should containLine("always @(posedge clock) begin") @@ -82,31 +86,31 @@ class PresetSpec extends FirrtlFlatSpec { result shouldNot containLine("wire reset;") result shouldNot containLine("assign reset = 1'h0;") } - it should "raise TreeCleanUpOrphantException on cast of annotated AsyncReset" in { - an[firrtl.transforms.PropagatePresetAnnotations.TreeCleanUpOrphanException] shouldBe thrownBy { - compileBody( + it should "replace usages of the preset reset with the constant 0 since the reset is never active" in { + val result = compileBody( + Seq( Seq( - Seq( - "Test", - s""" - |input clock : Clock - |input x : UInt<1> - |output z : UInt<1> - |output sz : UInt<1> - |wire reset : AsyncReset - |reset <= asAsyncReset(UInt(0)) - |reg r : UInt<1>, clock with : (reset => (reset, UInt(0))) - |wire sreset : UInt<1> - |sreset <= asUInt(reset) ; this is FORBIDDEN - |reg s : UInt<1>, clock with : (reset => (sreset, UInt(0))) - |r <= x - |s <= x - |z <= r - |sz <= s""".stripMargin - ) + "Test", + s""" + |input clock : Clock + |input x : UInt<1> + |output z : UInt<1> + |output sz : UInt<1> + |wire reset : AsyncReset + |reset <= asAsyncReset(UInt(0)) + |reg r : UInt<1>, clock with : (reset => (reset, UInt(0))) + |wire sreset : UInt<1> + |sreset <= asUInt(reset) ; this is ok, essentially like assigning zero to the wire + |reg s : UInt<1>, clock with : (reset => (sreset, UInt(0))) + |r <= x + |s <= x + |z <= r + |sz <= s""".stripMargin ) ) - } + ) + + result should containLine("wire sreset = 1'h0;") } it should "propagate through bundles" in { @@ -235,6 +239,31 @@ class PresetSpec extends FirrtlFlatSpec { result should containLine("reg r = 1'h0;") } + it should "propagate zeros for all other uses of an async reset" in { + val result = compileBody( + Seq( + Seq( + "Test", + s""" + |input clock : Clock + |input reset : AsyncReset + |output a : UInt<2> + |output b : UInt<2> + | + |node t = reset + |node not_t = not(asUInt(reset)) + |a <= cat(asUInt(t), not_t) + |node t2 = asUInt(t) + |node t3 = asAsyncReset(t2) + |b <= cat(asUInt(asSInt(reset)), asUInt(t3)) + |""".stripMargin + ) + ) + ) + + result should containLine("assign b = {1'h0,1'h0};") + } + it should "propagate even through disordonned statements" in { val result = compileBody( Seq( @@ -261,6 +290,32 @@ class PresetSpec extends FirrtlFlatSpec { result should containLine("reg r = 1'h0;") } + it should "work with verification statements that are guarded by a preset reset" in { + val result = compileBody( + Seq( + Seq( + "Test", + s""" + |input clock : Clock + |input in : UInt<4> + |input reset : AsyncReset + | + |node _T = eq(in, UInt<2>("h3")) @[main.scala 19:15] + |node _T_1 = asUInt(reset) @[main.scala 19:11] + |node _T_2 = eq(_T_1, UInt<1>("h0")) @[main.scala 19:11] + |when _T_2 : @[main.scala 19:11] + | assert(clock, _T, UInt<1>("h1"), "") : assert @[main.scala 19:11] + | node _T_3 = eq(_T, UInt<1>("h0")) @[main.scala 19:11] + | when _T_3 : @[main.scala 19:11] + | printf(clock, UInt<1>("h1"), "Assertion failed") : printf @[main.scala 19:11] + | + |""".stripMargin + ) + ) + ) + // just getting here without falling over the fact that `reset` gets removed is great! + } + } class PresetExecutionTest -- cgit v1.2.3