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 ++ 3 files changed, 110 insertions(+), 87 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 (limited to 'src/main/scala/firrtl/transforms') 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 +} -- cgit v1.2.3