Fold VerilogModulusCleanup into LegalizeVerilog (#2485)

This fixes handling of signed modulus and removes some redundant work.

2 files changed, 33 insertions, 63 deletions

diff --git a/src/main/scala/firrtl/backends/verilog/LegalizeVerilog.scala b/src/main/scala/firrtl/backends/verilog/LegalizeVerilog.scala
index f063f395..a7637767 100644
--- a/src/main/scala/firrtl/backends/verilog/LegalizeVerilog.scala
+++ b/src/main/scala/firrtl/backends/verilog/LegalizeVerilog.scala
@@ -9,6 +9,7 @@ import firrtl.transforms.ConstantPropagation
 import firrtl.{bitWidth, Dshlw, Transform}
 import firrtl.Mappers._
 import firrtl.passes.{Pass, SplitExpressions}
+import firrtl.passes.PadWidths.forceWidth
 
 /** Rewrites some expressions for valid/better Verilog emission.
   * - solves shift right overflows by replacing the shift with 0 for UInts and MSB for SInts
@@ -47,16 +48,44 @@ object LegalizeVerilog extends Pass {
     }
   }
 
-  import firrtl.passes.PadWidths.forceWidth
   private def getWidth(e: Expression): Int = bitWidth(e.tpe).toInt
 
+  /* Verilog has the width of (a % b) = Max(W(a), W(b))
+   * FIRRTL has the width of (a % b) = Min(W(a), W(b)), which makes more sense,
+   * but nevertheless is a problem when emitting verilog
+   *
+   * This function pads the arguments to be the same [max] width (to avoid lint issues)
+   * and then performs a bit extraction back down to the correct [min] width
+   */
+  private def legalizeRem(e: Expression): Expression = e match {
+    case rem @ DoPrim(Rem, Seq(a, b), _, tpe) =>
+      val awidth = getWidth(a)
+      val bwidth = getWidth(b)
+      // Do nothing if the widths are the same
+      if (awidth == bwidth) {
+        rem
+      } else {
+        // First pad the arguments to fix lint warnings because Verilog width is max of arguments
+        val maxWidth = awidth.max(bwidth)
+        val newType = tpe.mapWidth(_ => IntWidth(maxWidth))
+        val paddedRem =
+          rem
+            .map(forceWidth(maxWidth)(_)) // Pad the input arguments
+            .mapType(_ => newType) // Also make the width for this op correct
+        // Second, bit extract back down to the min width of original arguments to match FIRRTL semantics
+        val minWidth = awidth.min(bwidth)
+        forceWidth(minWidth)(paddedRem)
+      }
+    case _ => e
+  }
+
   private def onExpr(expr: Expression): Expression = expr.map(onExpr) match {
     case prim: DoPrim =>
       prim.op match {
         case Shr                => ConstantPropagation.foldShiftRight(prim)
         case Bits | Head | Tail => legalizeBitExtract(prim)
         case Neg                => legalizeNeg(prim)
-        case Rem                => prim.map(forceWidth(prim.args.map(getWidth).max))
+        case Rem                => legalizeRem(prim)
         case Dshl               =>
           // special case as args aren't all same width
           prim.copy(op = Dshlw, args = Seq(forceWidth(getWidth(prim))(prim.args.head), prim.args(1)))
diff --git a/src/main/scala/firrtl/passes/VerilogModulusCleanup.scala b/src/main/scala/firrtl/passes/VerilogModulusCleanup.scala
index 3ca862b9..d0582478 100644
--- a/src/main/scala/firrtl/passes/VerilogModulusCleanup.scala
+++ b/src/main/scala/firrtl/passes/VerilogModulusCleanup.scala
@@ -4,9 +4,6 @@ package firrtl
 package passes
 
 import firrtl.ir._
-import firrtl.Mappers._
-import firrtl.PrimOps.{Bits, Rem}
-import firrtl.Utils._
 import firrtl.options.Dependency
 
 import scala.collection.mutable
@@ -24,6 +21,7 @@ import scala.collection.mutable
   *  This is technically incorrect firrtl, but allows the verilog emitter
   *  to emit correct verilog without needing to add temporary nodes
   */
+@deprecated("This pass's functionality has been moved to LegalizeVerilog", "FIRRTL 1.5.2")
 object VerilogModulusCleanup extends Pass {
 
   override def prerequisites = firrtl.stage.Forms.LowFormMinimumOptimized ++
@@ -43,62 +41,5 @@ object VerilogModulusCleanup extends Pass {
 
   override def invalidates(a: Transform) = false
 
-  private def onModule(m: Module): Module = {
-    val namespace = Namespace(m)
-    def onStmt(s: Statement): Statement = {
-      val v = mutable.ArrayBuffer[Statement]()
-
-      def getWidth(e: Expression): Width = e.tpe match {
-        case t: GroundType => t.width
-        case t => UnknownWidth
-      }
-
-      def maxWidth(ws: Seq[Width]): Width = ws.reduceLeft { (x, y) =>
-        (x, y) match {
-          case (IntWidth(x), IntWidth(y)) => IntWidth(x.max(y))
-          case (x, y)                     => UnknownWidth
-        }
-      }
-
-      def verilogRemWidth(e: DoPrim)(tpe: Type): Type = {
-        val newWidth = maxWidth(e.args.map(exp => getWidth(exp)))
-        tpe.mapWidth(w => newWidth)
-      }
-
-      def removeRem(e: Expression): Expression = e match {
-        case e: DoPrim =>
-          e.op match {
-            case Rem =>
-              val name = namespace.newTemp
-              val newType = e.mapType(verilogRemWidth(e))
-              v += DefNode(get_info(s), name, e.mapType(verilogRemWidth(e)))
-              val remRef = WRef(name, newType.tpe, kind(e), flow(e))
-              val remWidth = bitWidth(e.tpe)
-              DoPrim(Bits, Seq(remRef), Seq(remWidth - 1, BigInt(0)), e.tpe)
-            case _ => e
-          }
-        case _ => e
-      }
-
-      s.map(removeRem) match {
-        case x: Block => x.map(onStmt)
-        case EmptyStmt => EmptyStmt
-        case x =>
-          v += x
-          v.size match {
-            case 1 => v.head
-            case _ => Block(v.toSeq)
-          }
-      }
-    }
-    Module(m.info, m.name, m.ports, onStmt(m.body))
-  }
-
-  def run(c: Circuit): Circuit = {
-    val modules = c.modules.map {
-      case m: Module    => onModule(m)
-      case m: ExtModule => m
-    }
-    Circuit(c.info, modules, c.main)
-  }
+  def run(c: Circuit): Circuit = c
 }