Skip to content

Commit

Permalink
Fix issue where inlined cvt could cause crash (#2124)
Browse files Browse the repository at this point in the history
Due to inlining of Boolean expressions, the following circuit is handled
directly by the VerilogEmitter:

input a: UInt<4>
input b: SInt<1>
output o: UInt<5>
o <= dshl(a, asUInt(cvt(b)))

Priot to this change, this could crash due to mishandling of cvt in the
logic to inject parentheses based on Verilog precedence rules.

This is a corner case, but similar bugs would drop up if we open up the
VerilogEmitter to more expression inlining.
  • Loading branch information
jackkoenig committed Mar 16, 2021
1 parent 0eb7afd commit 94d1bee
Show file tree
Hide file tree
Showing 2 changed files with 26 additions and 3 deletions.
13 changes: 10 additions & 3 deletions src/main/scala/firrtl/backends/verilog/VerilogEmitter.scala
Expand Up @@ -57,6 +57,13 @@ object VerilogEmitter {
private def precedenceGt(op1: PrimOp, op2: PrimOp): Boolean = {
precedenceMap(op1) < precedenceMap(op2)
}

/** Identifies PrimOps that never need parentheses
*
* These PrimOps emit either {..., a0, ...} or a0 so they never need parentheses
*/
private val neverParens: PrimOp => Boolean =
Set(Shl, Cat, Cvt, AsUInt, AsSInt, AsClock, AsAsyncReset, Pad)
}

class VerilogEmitter extends SeqTransform with Emitter {
Expand Down Expand Up @@ -229,8 +236,7 @@ class VerilogEmitter extends SeqTransform with Emitter {
// to ensure Verilog operations are signed.
def op_stream(doprim: DoPrim): Seq[Any] = {
def parenthesize(e: Expression, isFirst: Boolean): Any = doprim.op match {
// these PrimOps emit either {..., a0, ...} or a0 so they never need parentheses
case Shl | Cat | Cvt | AsUInt | AsSInt | AsClock | AsAsyncReset => e
case op if neverParens(op) => e
case _ =>
e match {
case e: DoPrim =>
Expand All @@ -247,7 +253,8 @@ class VerilogEmitter extends SeqTransform with Emitter {
*/
case other =>
val noParens =
precedenceGt(e.op, doprim.op) ||
neverParens(e.op) ||
precedenceGt(e.op, doprim.op) ||
(isFirst && precedenceEq(e.op, doprim.op) && !isUnaryOp(e.op))
if (noParens) other else Seq("(", other, ")")
}
Expand Down
16 changes: 16 additions & 0 deletions src/test/scala/firrtlTests/InlineBooleanExpressionsSpec.scala
Expand Up @@ -392,6 +392,22 @@ class InlineBooleanExpressionsSpec extends FirrtlFlatSpec {
firrtlEquivalenceTest(input, Seq(new InlineBooleanExpressions))
}

// https://github.com/chipsalliance/firrtl/issues/2035
// This is interesting because other ways of trying to express this get split out by
// SplitExpressions and don't get inlined again
// If we were to inline more expressions (ie. not just boolean ones) the issue this represents
// would come up more often
it should "handle cvt nested inside of a dshl" in {
val input =
"""circuit DshlCvt:
| module DshlCvt:
| input a: UInt<4>
| input b: SInt<1>
| output o: UInt
| o <= dshl(a, asUInt(cvt(b)))""".stripMargin
firrtlEquivalenceTest(input, Seq(new InlineBooleanExpressions))
}

it should s"respect --${PrettyNoExprInlining.longOption}" in {
val input =
"""circuit Top :
Expand Down

0 comments on commit 94d1bee

Please sign in to comment.