From 72d3983b313fb20b819c2555a13a627cbb9d63c3 Mon Sep 17 00:00:00 2001 From: David Biancolin Date: Fri, 21 Aug 2020 23:22:24 -0700 Subject: 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>--- .../scala/firrtl/transforms/LegalizeClocks.scala | 87 --------------- .../transforms/LegalizeClocksAndAsyncResets.scala | 102 ++++++++++++++++++ src/main/scala/firrtl/transforms/package.scala | 8 ++ src/test/scala/firrtlTests/AsyncResetSpec.scala | 1 - .../firrtlTests/transforms/LegalizeClocks.scala | 67 ------------ .../transforms/LegalizeClocksAndAsyncResets.scala | 117 +++++++++++++++++++++ 6 files changed, 227 insertions(+), 155 deletions(-) delete mode 100644 src/main/scala/firrtl/transforms/LegalizeClocks.scala create mode 100644 src/main/scala/firrtl/transforms/LegalizeClocksAndAsyncResets.scala create mode 100644 src/main/scala/firrtl/transforms/package.scala delete mode 100644 src/test/scala/firrtlTests/transforms/LegalizeClocks.scala create mode 100644 src/test/scala/firrtlTests/transforms/LegalizeClocksAndAsyncResets.scala (limited to 'src') diff --git a/src/main/scala/firrtl/transforms/LegalizeClocks.scala b/src/main/scala/firrtl/transforms/LegalizeClocks.scala deleted file mode 100644 index 248775d9..00000000 --- a/src/main/scala/firrtl/transforms/LegalizeClocks.scala +++ /dev/null @@ -1,87 +0,0 @@ -package firrtl -package transforms - -import firrtl.ir._ -import firrtl.Mappers._ -import firrtl.options.Dependency -import firrtl.Utils.isCast - -// Fixup otherwise legal Verilog that lint tools and other tools don't like -// Currently: -// - don't emit "always @(posedge )" -// Hitting this case is rare, but legal FIRRTL -// TODO This should be unified with all Verilog legalization transforms -object LegalizeClocksTransform { - - // Checks if an Expression is illegal in use in a @(posedge ) 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 { - case _: Literal => true - case DoPrim(op, args, _, _) if isCast(op) => args.exists(illegalClockExpr) - case _ => false - } - - /** Legalize Clocks in a Statement - * - * Enforces legal Verilog semantics on all Clock Expressions. - * Legal is defined as what standard lint tools accept. - * Currently only Literal Expressions (guarded by casts) are handled. - * - * @note namespace is lazy because it should not typically be needed - */ - 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) => - 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) => - 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) => - val node = DefNode(s.info, namespace.newTemp, s.clk) - val sx = s.copy(clk = WRef(node)) - Block(Seq(node, sx)) - case other => other - } - - def onMod(mod: DefModule): DefModule = { - // It's actually *extremely* important that this Namespace is a lazy val - // onStmt accepts it lazily so that we don't perform the namespacing traversal unless necessary - // If we were to inline the declaration, it would create a Namespace for every problem, causing - // name collisions - lazy val namespace = Namespace(mod) - mod.map(onStmt(namespace)) - } -} - -/** Ensure Clocks to be emitted are legal Verilog */ -class LegalizeClocksTransform extends Transform with DependencyAPIMigration { - - override def prerequisites = firrtl.stage.Forms.LowFormMinimumOptimized ++ - Seq( - Dependency[BlackBoxSourceHelper], - Dependency[FixAddingNegativeLiterals], - Dependency[ReplaceTruncatingArithmetic], - Dependency[InlineBitExtractionsTransform], - Dependency[InlineCastsTransform] - ) - - override def optionalPrerequisites = firrtl.stage.Forms.LowFormOptimized - - override def optionalPrerequisiteOf = Seq.empty - - override def invalidates(a: Transform) = false - - def execute(state: CircuitState): CircuitState = { - val modulesx = state.circuit.modules.map(LegalizeClocksTransform.onMod(_)) - state.copy(circuit = state.circuit.copy(modules = modulesx)) - } -} diff --git a/src/main/scala/firrtl/transforms/LegalizeClocksAndAsyncResets.scala b/src/main/scala/firrtl/transforms/LegalizeClocksAndAsyncResets.scala new file mode 100644 index 00000000..5a1ccdbf --- /dev/null +++ b/src/main/scala/firrtl/transforms/LegalizeClocksAndAsyncResets.scala @@ -0,0 +1,102 @@ +package firrtl +package transforms + +import firrtl.ir._ +import firrtl.Mappers._ +import firrtl.options.Dependency +import firrtl.Utils.isCast + +// Fixup otherwise legal Verilog that lint tools and other tools don't like +// Currently: +// - don't emit "always @(posedge )" +// Hitting this case is rare, but legal FIRRTL +// TODO This should be unified with all Verilog legalization transforms +object LegalizeClocksAndAsyncResetsTransform { + + // Checks if an Expression is illegal in use in a @(posedge ) construct + // Legality is defined here by what standard lint tools accept + // Currently only looks for literals nested within casts + private def isLiteralExpression(expr: Expression): Boolean = expr match { + case _: Literal => true + case DoPrim(op, args, _, _) if isCast(op) => args.exists(isLiteralExpression) + case _ => false + } + + // 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 and AsyncReset Expressions. + * Legal is defined as what standard lint tools accept. + * Currently only Literal Expressions (guarded by casts) are handled. + * + * @note namespace is lazy because it should not typically be needed + */ + def onStmt(namespace: => Namespace)(stmt: Statement): Statement = + stmt.map(onStmt(namespace)) match { + // Proper union types would deduplicate this code + 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 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 isLiteralExpression(s.clk) => + val node = DefNode(s.info, namespace.newTemp, s.clk) + val sx = s.copy(clk = WRef(node)) + Block(Seq(node, sx)) + case other => other + } + + def onMod(mod: DefModule): DefModule = { + // It's actually *extremely* important that this Namespace is a lazy val + // onStmt accepts it lazily so that we don't perform the namespacing traversal unless necessary + // If we were to inline the declaration, it would create a Namespace for every problem, causing + // name collisions + lazy val namespace = Namespace(mod) + mod.map(onStmt(namespace)) + } +} + +/** Ensure Clocks and AsyncResets to be emitted are legal Verilog */ +class LegalizeClocksAndAsyncResetsTransform extends Transform with DependencyAPIMigration { + + override def prerequisites = firrtl.stage.Forms.LowFormMinimumOptimized ++ + Seq( + Dependency[BlackBoxSourceHelper], + Dependency[FixAddingNegativeLiterals], + Dependency[ReplaceTruncatingArithmetic], + Dependency[InlineBitExtractionsTransform], + Dependency[InlineCastsTransform] + ) + + override def optionalPrerequisites = firrtl.stage.Forms.LowFormOptimized + + override def optionalPrerequisiteOf = Seq.empty + + override def invalidates(a: Transform) = false + + def execute(state: CircuitState): CircuitState = { + 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) + + } +} -- cgit v1.2.3