Fix issue where inlined cvt could cause crash (#2124)

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.

2 files changed, 26 insertions, 3 deletions

diff --git a/src/main/scala/firrtl/backends/verilog/VerilogEmitter.scala b/src/main/scala/firrtl/backends/verilog/VerilogEmitter.scala
index c7143f5f..33c6b9a8 100644
--- a/src/main/scala/firrtl/backends/verilog/VerilogEmitter.scala
+++ b/src/main/scala/firrtl/backends/verilog/VerilogEmitter.scala
@@ -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 {
@@ -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 =>
@@ -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, ")")
             }
diff --git a/src/test/scala/firrtlTests/InlineBooleanExpressionsSpec.scala b/src/test/scala/firrtlTests/InlineBooleanExpressionsSpec.scala
index 02ac3cd0..e11c4281 100644
--- a/src/test/scala/firrtlTests/InlineBooleanExpressionsSpec.scala
+++ b/src/test/scala/firrtlTests/InlineBooleanExpressionsSpec.scala
@@ -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 :