Async reset tieoff bug (#1854)

* Elide emission of literals  for async reset in sensitivity lists

* Deprecate LegalizeClocksTransform

Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>

5 files changed, 155 insertions, 83 deletions

diff --git a/src/main/scala/firrtl/transforms/LegalizeClocks.scala b/src/main/scala/firrtl/transforms/LegalizeClocksAndAsyncResets.scala
index 248775d9..5a1ccdbf 100644
--- a/src/main/scala/firrtl/transforms/LegalizeClocks.scala
+++ b/src/main/scala/firrtl/transforms/LegalizeClocksAndAsyncResets.scala
@@ -11,20 +11,25 @@ import firrtl.Utils.isCast
 //   - don't emit "always @(posedge <literal>)"
 //     Hitting this case is rare, but legal FIRRTL
 // TODO This should be unified with all Verilog legalization transforms
-object LegalizeClocksTransform {
+object LegalizeClocksAndAsyncResetsTransform {
 
   // Checks if an Expression is illegal in use in a @(posedge <Expression>) construct
   // Legality is defined here by what standard lint tools accept
   // Currently only looks for literals nested within casts
-  private def illegalClockExpr(expr: Expression): Boolean = expr match {
+  private def isLiteralExpression(expr: Expression): Boolean = expr match {
     case _: Literal => true
-    case DoPrim(op, args, _, _) if isCast(op) => args.exists(illegalClockExpr)
+    case DoPrim(op, args, _, _) if isCast(op) => args.exists(isLiteralExpression)
     case _                                    => false
   }
 
-  /** Legalize Clocks in a Statement
+  // Wraps the above function to check if a Rest is Async to avoid unneeded
+  // hoisting of sync reset literals.
+  private def isAsyncResetLiteralExpr(expr: Expression): Boolean =
+    if (expr.tpe == AsyncResetType) isLiteralExpression(expr) else false
+
+  /** Legalize Clocks and AsyncResets in a Statement
     *
-    * Enforces legal Verilog semantics on all Clock Expressions.
+    * Enforces legal Verilog semantics on all Clock and AsyncReset Expressions.
     * Legal is defined as what standard lint tools accept.
     * Currently only Literal Expressions (guarded by casts) are handled.
     *
@@ -33,19 +38,29 @@ object LegalizeClocksTransform {
   def onStmt(namespace: => Namespace)(stmt: Statement): Statement =
     stmt.map(onStmt(namespace)) match {
       // Proper union types would deduplicate this code
-      case r: DefRegister if illegalClockExpr(r.clock) =>
-        val node = DefNode(r.info, namespace.newTemp, r.clock)
-        val rx = r.copy(clock = WRef(node))
-        Block(Seq(node, rx))
-      case p: Print if illegalClockExpr(p.clk) =>
+      case r: DefRegister if (isLiteralExpression(r.clock) || isAsyncResetLiteralExpr(r.reset)) =>
+        val (clockNodeOpt, rxClock) = if (isLiteralExpression(r.clock)) {
+          val node = DefNode(r.info, namespace.newTemp, r.clock)
+          (Some(node), r.copy(clock = WRef(node)))
+        } else {
+          (None, r)
+        }
+        val (resetNodeOpt, rx) = if (isAsyncResetLiteralExpr(r.reset)) {
+          val node = DefNode(r.info, namespace.newTemp, r.reset)
+          (Some(node), rxClock.copy(reset = WRef(node)))
+        } else {
+          (None, rxClock)
+        }
+        Block(clockNodeOpt ++: resetNodeOpt ++: Seq(rx))
+      case p: Print if isLiteralExpression(p.clk) =>
         val node = DefNode(p.info, namespace.newTemp, p.clk)
         val px = p.copy(clk = WRef(node))
         Block(Seq(node, px))
-      case s: Stop if illegalClockExpr(s.clk) =>
+      case s: Stop if isLiteralExpression(s.clk) =>
         val node = DefNode(s.info, namespace.newTemp, s.clk)
         val sx = s.copy(clk = WRef(node))
         Block(Seq(node, sx))
-      case s: Verification if illegalClockExpr(s.clk) =>
+      case s: Verification if isLiteralExpression(s.clk) =>
         val node = DefNode(s.info, namespace.newTemp, s.clk)
         val sx = s.copy(clk = WRef(node))
         Block(Seq(node, sx))
@@ -62,8 +77,8 @@ object LegalizeClocksTransform {
   }
 }
 
-/** Ensure Clocks to be emitted are legal Verilog */
-class LegalizeClocksTransform extends Transform with DependencyAPIMigration {
+/** Ensure Clocks and AsyncResets to be emitted are legal Verilog */
+class LegalizeClocksAndAsyncResetsTransform extends Transform with DependencyAPIMigration {
 
   override def prerequisites = firrtl.stage.Forms.LowFormMinimumOptimized ++
     Seq(
@@ -81,7 +96,7 @@ class LegalizeClocksTransform extends Transform with DependencyAPIMigration {
   override def invalidates(a: Transform) = false
 
   def execute(state: CircuitState): CircuitState = {
-    val modulesx = state.circuit.modules.map(LegalizeClocksTransform.onMod(_))
+    val modulesx = state.circuit.modules.map(LegalizeClocksAndAsyncResetsTransform.onMod(_))
     state.copy(circuit = state.circuit.copy(modules = modulesx))
   }
 }
diff --git a/src/main/scala/firrtl/transforms/package.scala b/src/main/scala/firrtl/transforms/package.scala
new file mode 100644
index 00000000..4043598c
--- /dev/null
+++ b/src/main/scala/firrtl/transforms/package.scala
@@ -0,0 +1,8 @@
+package firrtl
+
+package object transforms {
+  @deprecated("Replaced by LegalizeClocksAndAsyncResetsTransform", "FIRRTL 1.4.0")
+  type LegalizeClocksTransform = LegalizeClocksAndAsyncResetsTransform
+  @deprecated("Replaced by LegalizeClocksAndAsyncResetsTransform", "FIRRTL 1.4.0")
+  val LegalizeClocksTransform = LegalizeClocksAndAsyncResetsTransform
+}
diff --git a/src/test/scala/firrtlTests/AsyncResetSpec.scala b/src/test/scala/firrtlTests/AsyncResetSpec.scala
index 04b558e9..a27d44d3 100644
--- a/src/test/scala/firrtlTests/AsyncResetSpec.scala
+++ b/src/test/scala/firrtlTests/AsyncResetSpec.scala
@@ -428,7 +428,6 @@ class AsyncResetSpec extends VerilogTransformSpec {
       "end",
       "end"
     )
-
   }
 }
 
diff --git a/src/test/scala/firrtlTests/transforms/LegalizeClocks.scala b/src/test/scala/firrtlTests/transforms/LegalizeClocks.scala
deleted file mode 100644
index 6ee0f5a0..00000000
--- a/src/test/scala/firrtlTests/transforms/LegalizeClocks.scala
+++ /dev/null
@@ -1,67 +0,0 @@
-// See LICENSE for license details.
-
-package firrtlTests.transforms
-
-import firrtl._
-import firrtl.testutils._
-import firrtl.testutils.FirrtlCheckers.containLine
-
-class LegalizeClocksTransformSpec extends FirrtlFlatSpec {
-  def compile(input: String): CircuitState =
-    (new MinimumVerilogCompiler).compileAndEmit(CircuitState(parse(input), ChirrtlForm), Nil)
-
-  behavior.of("LegalizeClocksTransform")
-
-  it should "not emit @(posedge 1'h0) for stop" in {
-    val input =
-      """circuit test :
-        |  module test :
-        |    stop(asClock(UInt(1)), UInt(1), 1)
-        |""".stripMargin
-    val result = compile(input)
-    result should containLine(s"always @(posedge _GEN_0) begin")
-    result.getEmittedCircuit.value shouldNot include("always @(posedge 1")
-  }
-
-  it should "not emit @(posedge 1'h0) for printf" in {
-    val input =
-      """circuit test :
-        |  module test :
-        |    printf(asClock(UInt(1)), UInt(1), "hi")
-        |""".stripMargin
-    val result = compile(input)
-    result should containLine(s"always @(posedge _GEN_0) begin")
-    result.getEmittedCircuit.value shouldNot include("always @(posedge 1")
-  }
-
-  it should "not emit @(posedge 1'h0) for reg" in {
-    val input =
-      """circuit test :
-        |  module test :
-        |    output out : UInt<8>
-        |    input in : UInt<8>
-        |    reg r : UInt<8>, asClock(UInt(0))
-        |    r <= in
-        |    out <= r
-        |""".stripMargin
-    val result = compile(input)
-    result should containLine(s"always @(posedge _GEN_0) begin")
-    result.getEmittedCircuit.value shouldNot include("always @(posedge 1")
-  }
-
-  it should "deduplicate injected nodes for literal clocks" in {
-    val input =
-      """circuit test :
-        |  module test :
-        |    printf(asClock(UInt(1)), UInt(1), "hi")
-        |    stop(asClock(UInt(1)), UInt(1), 1)
-        |""".stripMargin
-    val result = compile(input)
-    result should containLine(s"wire  _GEN_0 = 1'h1;")
-    // Check that there's only 1 _GEN_0 instantiation
-    val verilog = result.getEmittedCircuit.value
-    val matches = "wire\\s+_GEN_0\\s+=\\s+1'h1".r.findAllIn(verilog)
-    matches.size should be(1)
-
-  }
-}
diff --git a/src/test/scala/firrtlTests/transforms/LegalizeClocksAndAsyncResets.scala b/src/test/scala/firrtlTests/transforms/LegalizeClocksAndAsyncResets.scala
new file mode 100644
index 00000000..32563428
--- /dev/null
+++ b/src/test/scala/firrtlTests/transforms/LegalizeClocksAndAsyncResets.scala
@@ -0,0 +1,117 @@
+// See LICENSE for license details.
+
+package firrtlTests.transforms
+
+import firrtl._
+import firrtl.testutils._
+import firrtl.testutils.FirrtlCheckers.containLine
+
+class LegalizeClocksTransformSpec extends FirrtlFlatSpec {
+  def compile(input: String): CircuitState =
+    (new MinimumVerilogCompiler).compileAndEmit(CircuitState(parse(input), ChirrtlForm), Nil)
+
+  behavior.of("LegalizeClocksTransform")
+
+  it should "not emit @(posedge 1'h0) for stop" in {
+    val input =
+      """circuit test :
+        |  module test :
+        |    stop(asClock(UInt(1)), UInt(1), 1)
+        |""".stripMargin
+    val result = compile(input)
+    result should containLine(s"always @(posedge _GEN_0) begin")
+    result.getEmittedCircuit.value shouldNot include("always @(posedge 1")
+  }
+
+  it should "not emit @(posedge 1'h0) for printf" in {
+    val input =
+      """circuit test :
+        |  module test :
+        |    printf(asClock(UInt(1)), UInt(1), "hi")
+        |""".stripMargin
+    val result = compile(input)
+    result should containLine(s"always @(posedge _GEN_0) begin")
+    result.getEmittedCircuit.value shouldNot include("always @(posedge 1")
+  }
+
+  it should "not emit @(posedge 1'h0) for reg" in {
+    val input =
+      """circuit test :
+        |  module test :
+        |    output out : UInt<8>
+        |    input in : UInt<8>
+        |    reg r : UInt<8>, asClock(UInt(0))
+        |    r <= in
+        |    out <= r
+        |""".stripMargin
+    val result = compile(input)
+    result should containLine(s"always @(posedge _GEN_0) begin")
+    result.getEmittedCircuit.value shouldNot include("always @(posedge 1")
+  }
+
+  it should "not emit @(posedge clock or posedge 1'h0) for a constantly deasserted areset" in {
+    val input = """circuit test :
+                  |  module test :
+                  |    input clock : Clock
+                  |    input i: UInt<1>
+                  |    output z: UInt<1>
+                  |    wire reset : AsyncReset
+                  |    reset <= asAsyncReset(UInt<1>("h0"))
+                  |    reg r : UInt<1>, clock with : (reset => (reset, UInt(0)))
+                  |    r <= i
+                  |    z <= r""".stripMargin
+    val result = compile(input)
+    result should containLine(s"always @(posedge clock or posedge _GEN_0) begin")
+    result.getEmittedCircuit.value shouldNot include("always @(posedge clock or posedge 1")
+  }
+
+  it should "not emit @(posedge clock or posedge 1'h1) for a constantly asserted areset" in {
+    val input = """circuit test :
+                  |  module test :
+                  |    input clock : Clock
+                  |    input i: UInt<1>
+                  |    output z: UInt<1>
+                  |    wire reset : AsyncReset
+                  |    reset <= asAsyncReset(UInt<1>("h1"))
+                  |    reg r : UInt<1>, clock with : (reset => (reset, UInt(0)))
+                  |    r <= i
+                  |    z <= r""".stripMargin
+    val result = compile(input)
+    result should containLine(s"always @(posedge clock or posedge _GEN_0) begin")
+    result.getEmittedCircuit.value shouldNot include("always @(posedge clock or posedge 1")
+  }
+
+  it should "not emit @(posedge 1'h0 or posedge 1'h0) for a reg with tied off clocks and areset" in {
+    val input = """circuit test :
+                  |  module test :
+                  |    input i: UInt<1>
+                  |    output z: UInt<1>
+                  |    wire clock : Clock
+                  |    clock <= asClock(UInt(1))
+                  |    wire reset : AsyncReset
+                  |    reset <= asAsyncReset(UInt<1>("h0"))
+                  |    reg r : UInt<1>, clock with : (reset => (reset, UInt(0)))
+                  |    r <= i
+                  |    z <= r""".stripMargin
+    val result = compile(input)
+    result should containLine(s"always @(posedge _GEN_0 or posedge _GEN_1) begin")
+    result.getEmittedCircuit.value shouldNot include("always @(posedge clock or posedge 1")
+    result.getEmittedCircuit.value shouldNot include("or posedge 1")
+  }
+
+  it should "deduplicate injected nodes for literal clocks" in {
+    val input =
+      """circuit test :
+        |  module test :
+        |    printf(asClock(UInt(1)), UInt(1), "hi")
+        |    stop(asClock(UInt(1)), UInt(1), 1)
+        |""".stripMargin
+    val result = compile(input)
+    result should containLine(s"wire  _GEN_0 = 1'h1;")
+    // Check that there's only 1 _GEN_0 instantiation
+    val verilog = result.getEmittedCircuit.value
+    val matches = "wire\\s+_GEN_0\\s+=\\s+1'h1".r.findAllIn(verilog)
+    matches.size should be(1)
+
+  }
+}