Static shift right width change part 2 (#3841)

* Apply some suggestions from post-merge code review
* Add "use" to CLI option: --use-legacy-shift-right-width
* Improve stability of emitted FIRRTL and Verilog when using legacy
  option
* DRY out the tests

core/src/main/scala/chisel3/Bits.scala

@@ -684,12 +684,14 @@ sealed class UInt private[chisel3] (width: Width) extends Bits(width) with Num[U
     val resultWidth = this.width.shiftRight(that)
     val op = binop(sourceInfo, UInt(resultWidth), ShiftRightOp, validateShiftAmount(that))
     resultWidth match {
-      // If result width is known and 0, wrap in pad(_, 1) to emulate old FIRRTL behavior,
-      //   but lie and say width = 0 to emulate old Chisel behavior.
-      case w @ KnownWidth(0) => op.binop(sourceInfo, UInt(w), PadOp, 1)
-      // If result width is unknown, still wrap in pad(_, 1) to emulate old FIRRTL behavior.
-      case UnknownWidth() => op.binop(sourceInfo, UInt(UnknownWidth()), PadOp, 1)
-      case _              => op
+      // To emulate old FIRRTL behavior where minimum width is 1, we need to insert pad(_, 1) whenever
+      // the width is or could be 0. Thus we check if it is known to be 0 or is unknown.
+      case w @ (KnownWidth(0) | UnknownWidth()) =>
+        // Because we are inserting an extra op but we want stable emission (so the user can diff the output),
+        // we need to seed a name to avoid name collisions.
+        op.autoSeed("_shrLegacyWidthFixup")
+        op.binop(sourceInfo, UInt(w), PadOp, 1)
+      case _ => op
     }
   }
 
@@ -1011,8 +1013,8 @@ sealed class SInt private[chisel3] (width: Width) extends Bits(width) with Num[S
 
   @nowarn("msg=method shiftRight in class Width is deprecated")
   override def do_>>(that: Int)(implicit sourceInfo: SourceInfo): SInt = {
-    // We don't need to pad to emulate old behavior for SInt, just emulate old Chisel behavior with reported width,
-    // FIRRTL will give a minimum of 1 bit for SInt
+    // We don't need to pad to emulate old behavior for SInt, just emulate old Chisel behavior with reported width.
+    // FIRRTL will give a minimum of 1 bit for SInt.
     val newWidth = if (Builder.legacyShiftRightWidth) this.width.shiftRight(that) else this.width.signedShiftRight(that)
     binop(sourceInfo, SInt(newWidth), ShiftRightOp, validateShiftAmount(that))
   }

src/main/scala/chisel3/aop/injecting/InjectingAspect.scala

@@ -61,7 +61,7 @@ abstract class InjectorAspect[T <: RawModule, M <: RawModule](
         new DynamicContext(
           annotationsInAspect,
           chiselOptions.throwOnFirstError,
-          chiselOptions.legacyShiftRightWidth,
+          chiselOptions.useLegacyShiftRightWidth,
           chiselOptions.warningFilters,
           chiselOptions.sourceRoots,
           None,

src/main/scala/chisel3/stage/ChiselAnnotations.scala

@@ -366,7 +366,7 @@ case class DesignAnnotation[DUT <: RawModule](design: DUT) extends NoTargetAnnot
   *
   * '''This should only be used for checking for unexpected semantic changes when bumping to Chisel 7.0.0'''
   *
-  * Use as CLI option `--legacy-shift-right-width`.
+  * Use as CLI option `--use-legacy-shift-right-width`.
   *
   * This behavior is inconsistent between Chisel and FIRRTL
   * - Chisel will report the width of a UInt or SInt shifted right by a number >= its width as a 0-bit value
@@ -380,7 +380,7 @@ case object UseLegacyShiftRightWidthBehavior
 
   val options = Seq(
     new ShellOption[Unit](
-      longOption = "legacy-shift-right-width",
+      longOption = "use-legacy-shift-right-width",
       toAnnotationSeq = _ => Seq(UseLegacyShiftRightWidthBehavior),
       helpText = "Use legacy (buggy) shift right width behavior (pre Chisel 7.0.0)"
     )

src/main/scala/chisel3/stage/ChiselOptions.scala

@@ -7,22 +7,22 @@ import chisel3.internal.WarningFilter
 import java.io.File
 
 class ChiselOptions private[stage] (
-  val printFullStackTrace:   Boolean = false,
-  val throwOnFirstError:     Boolean = false,
-  val outputFile:            Option[String] = None,
-  val chiselCircuit:         Option[Circuit] = None,
-  val sourceRoots:           Vector[File] = Vector.empty,
-  val warningFilters:        Vector[WarningFilter] = Vector.empty,
-  val legacyShiftRightWidth: Boolean = false) {
+  val printFullStackTrace:      Boolean = false,
+  val throwOnFirstError:        Boolean = false,
+  val outputFile:               Option[String] = None,
+  val chiselCircuit:            Option[Circuit] = None,
+  val sourceRoots:              Vector[File] = Vector.empty,
+  val warningFilters:           Vector[WarningFilter] = Vector.empty,
+  val useLegacyShiftRightWidth: Boolean = false) {
 
   private[stage] def copy(
-    printFullStackTrace:   Boolean = printFullStackTrace,
-    throwOnFirstError:     Boolean = throwOnFirstError,
-    outputFile:            Option[String] = outputFile,
-    chiselCircuit:         Option[Circuit] = chiselCircuit,
-    sourceRoots:           Vector[File] = sourceRoots,
-    warningFilters:        Vector[WarningFilter] = warningFilters,
-    legacyShiftRightWidth: Boolean = legacyShiftRightWidth
+    printFullStackTrace:      Boolean = printFullStackTrace,
+    throwOnFirstError:        Boolean = throwOnFirstError,
+    outputFile:               Option[String] = outputFile,
+    chiselCircuit:            Option[Circuit] = chiselCircuit,
+    sourceRoots:              Vector[File] = sourceRoots,
+    warningFilters:           Vector[WarningFilter] = warningFilters,
+    useLegacyShiftRightWidth: Boolean = useLegacyShiftRightWidth
   ): ChiselOptions = {
 
     new ChiselOptions(
@@ -32,7 +32,7 @@ class ChiselOptions private[stage] (
       chiselCircuit = chiselCircuit,
       sourceRoots = sourceRoots,
       warningFilters = warningFilters,
-      legacyShiftRightWidth = legacyShiftRightWidth
+      useLegacyShiftRightWidth = useLegacyShiftRightWidth
     )
 
   }

src/main/scala/chisel3/stage/package.scala

@@ -29,7 +29,7 @@ package object stage {
           case SourceRootAnnotation(s)       => c.copy(sourceRoots = c.sourceRoots :+ s)
           case a: WarningConfigurationAnnotation     => c.copy(warningFilters = c.warningFilters ++ a.filters)
           case a: WarningConfigurationFileAnnotation => c.copy(warningFilters = c.warningFilters ++ a.filters)
-          case UseLegacyShiftRightWidthBehavior => c.copy(legacyShiftRightWidth = true)
+          case UseLegacyShiftRightWidthBehavior => c.copy(useLegacyShiftRightWidth = true)
         }
       }
 

src/main/scala/chisel3/stage/phases/Elaborate.scala

@@ -33,7 +33,7 @@ class Elaborate extends Phase {
           new DynamicContext(
             annotations,
             chiselOptions.throwOnFirstError,
-            chiselOptions.legacyShiftRightWidth,
+            chiselOptions.useLegacyShiftRightWidth,
             chiselOptions.warningFilters,
             chiselOptions.sourceRoots,
             None,

src/test/scala/chiselTests/SIntOps.scala

@@ -111,7 +111,7 @@ class SIntLitZeroWidthTester extends BasicTester {
   stop()
 }
 
-class SIntOpsSpec extends ChiselPropSpec with Utils {
+class SIntOpsSpec extends ChiselPropSpec with Utils with ShiftRightWidthBehavior {
 
   property("SIntOps should elaborate") {
     ChiselStage.emitCHIRRTL { new SIntOps }
@@ -199,100 +199,14 @@ class SIntOpsSpec extends ChiselPropSpec with Utils {
   }
 
   property("Static right-shift should have a minimum width of 1") {
-    assertKnownWidth(4) {
-      val in = IO(Input(SInt(8.W)))
-      in >> 4
-    }
-    assertKnownWidth(1) {
-      val in = IO(Input(SInt(8.W)))
-      in >> 8
-    }
-    assertKnownWidth(1) {
-      val in = IO(Input(SInt(8.W)))
-      in >> 16
-    }
-    assertKnownWidth(1) {
-      val in = IO(Input(SInt(0.W)))
-      in >> 8
-    }
-    assertKnownWidth(1) {
-      val in = IO(Input(SInt(0.W)))
-      in >> 0
-    }
-    assertInferredWidth(4) {
-      val in = IO(Input(SInt(8.W)))
-      val w = WireInit(SInt(), in)
-      w >> 4
-    }
-    assertInferredWidth(1) {
-      val in = IO(Input(SInt(8.W)))
-      val w = WireInit(SInt(), in)
-      w >> 8
-    }
-    assertInferredWidth(1) {
-      val in = IO(Input(SInt(8.W)))
-      val w = WireInit(SInt(), in)
-      w >> 16
-    }
-    assertInferredWidth(1) {
-      val in = IO(Input(SInt(0.W)))
-      val w = WireInit(SInt(), in)
-      w >> 8
-    }
-    assertInferredWidth(1) {
-      val in = IO(Input(SInt(0.W)))
-      val w = WireInit(SInt(), in)
-      w >> 0
-    }
+    testShiftRightWidthBehavior(SInt)(chiselMinWidth = 1, firrtlMinWidth = 1)
   }
 
-  property("Static right-shift should have width of 0 in Chisel and 1 in FIRRTL with --legacy-shift-right-width") {
-    val args = Array("--legacy-shift-right-width")
-    assertKnownWidth(4, args) {
-      val in = IO(Input(SInt(8.W)))
-      in >> 4
-    }
-    assertKnownWidth(0, args) {
-      val in = IO(Input(SInt(8.W)))
-      in >> 8
-    }
-    assertKnownWidth(0, args) {
-      val in = IO(Input(SInt(8.W)))
-      in >> 16
-    }
-    assertKnownWidth(0, args) {
-      val in = IO(Input(SInt(0.W)))
-      in >> 8
-    }
-    assertKnownWidth(0, args) {
-      val in = IO(Input(SInt(0.W)))
-      in >> 0
-    }
-    assertInferredWidth(4, args) {
-      val in = IO(Input(SInt(8.W)))
-      val w = WireInit(SInt(), in)
-      w >> 4
-    }
-    assertInferredWidth(1, args) {
-      val in = IO(Input(SInt(8.W)))
-      val w = WireInit(SInt(), in)
-      w >> 8
-    }
-    assertInferredWidth(1, args) {
-      val in = IO(Input(SInt(8.W)))
-      val w = WireInit(SInt(), in)
-      w >> 16
-    }
-    assertInferredWidth(1, args) {
-      val in = IO(Input(SInt(0.W)))
-      val w = WireInit(SInt(), in)
-      w >> 8
-    }
-    assertInferredWidth(1, args) {
-      val in = IO(Input(SInt(0.W)))
-      val w = WireInit(SInt(), in)
-      w >> 0
-    }
+  property("Static right-shift should have width of 0 in Chisel and 1 in FIRRTL with --use-legacy-shift-right-width") {
+    val args = Array("--use-legacy-shift-right-width")
+
+    testShiftRightWidthBehavior(SInt)(chiselMinWidth = 0, firrtlMinWidth = 1, args = args)
+
     // Focused test to show the mismatch
     class TestModule extends Module {
       val in = IO(Input(SInt(8.W)))