Fix widths for literal values in Bundle literals (backport #4082) (#4090

)

* Fix widths for literal values in Bundle literals (#4082)

* Fix widths for literal values in Bundle literals

Previously, the user-specified (or unspecified minimum width) of the
literal would be used in some operations like concatenation.

* Implicitly truncate too-wide literals in Bundle literals

This is to fix a bug in concatenation without breaking things relying on
implicit truncation. This will be a warning in newer versions of Chisel.

(cherry picked from commit 3939e57)

# Conflicts:
#	core/src/main/scala/chisel3/internal/firrtl/IR.scala
#	src/test/scala/chiselTests/BundleLiteralSpec.scala
#	src/test/scala/chiselTests/ConnectableSpec.scala
#	src/test/scala/chiselTests/VecLiteralSpec.scala

* Resolve backport conflicts and waive false MiMa failure.

---------

Co-authored-by: Jack Koenig <koenig@sifive.com>

build.sbt

@@ -1,5 +1,7 @@
 // See LICENSE for license details.
 
+import com.typesafe.tools.mima.core._
+
 enablePlugins(SiteScaladocPlugin)
 
 addCommandAlias("fmt", "; scalafmtAll ; scalafmtSbt")
@@ -19,8 +21,7 @@ ThisBuild / firtoolVersion := {
 
 // Previous versions are read from project/previous-versions.txt
 // If this file is empty or does not exist, no binary compatibility checking will be done
-// Add waivers to the directory defined by key `mimaFiltersDirectory` in files named: <since version>.backwards.excludes
-//   eg. unipublish/src/main/mima-filters/5.0.0.backwards.excludes
+// Add waivers to mimaBinaryIssueFilters in the correct project below.
 val previousVersions = settingKey[Set[String]]("Previous versions for binary compatibility checking")
 ThisBuild / previousVersions := {
   val file = new java.io.File("project", "previous-versions.txt")
@@ -253,7 +254,12 @@ lazy val core = (project in file("core"))
   .settings(
     mimaPreviousArtifacts := previousVersions.value.map { version =>
       organization.value %% name.value % version
-    }
+    },
+    // MiMa waivers
+    mimaBinaryIssueFilters ++= Seq(
+      // Technically users could extend LitArg but its in an internal package and there is no reason to.
+      ProblemFilters.exclude[ReversedMissingMethodProblem]("chisel3.internal.firrtl.LitArg.cloneWithValue")
+    )
   )
   .settings(warningSuppression: _*)
   .settings(fatalWarningsSettings: _*)

core/src/main/scala/chisel3/Aggregate.scala

@@ -1084,10 +1084,10 @@ abstract class Record(private[chisel3] implicit val compileOptions: CompileOptio
 
     requireIsChiselType(this, "bundle literal constructor model")
     val clone = cloneType
-    val cloneFields = getRecursiveFields(clone, "(bundle root)").toMap
+    val cloneFields = getRecursiveFields(clone, "_").toMap
 
     // Create the Bundle literal binding from litargs of arguments
-    val bundleLitMap = elems.map { fn => fn(clone) }.flatMap {
+    val bundleLitMapping = elems.map { fn => fn(clone) }.flatMap {
       case (field, value) =>
         val fieldName = cloneFields.getOrElse(
           field,
@@ -1169,12 +1169,31 @@ abstract class Record(private[chisel3] implicit val compileOptions: CompileOptio
     }
 
     // don't convert to a Map yet to preserve duplicate keys
-    val duplicates = bundleLitMap.map(_._1).groupBy(identity).collect { case (x, elts) if elts.size > 1 => x }
+    val duplicates = bundleLitMapping.map(_._1).groupBy(identity).collect { case (x, elts) if elts.size > 1 => x }
     if (!duplicates.isEmpty) {
       val duplicateNames = duplicates.map(cloneFields(_)).mkString(", ")
       throw new BundleLiteralException(s"duplicate fields $duplicateNames in Bundle literal constructor")
     }
-    clone.bind(BundleLitBinding(bundleLitMap.toMap))
+    // Check widths and sign extend as appropriate.
+    val bundleLitMap = bundleLitMapping.view.map {
+      case (field, value) =>
+        field.width match {
+          // If width is unknown, then it is set by the literal value.
+          case UnknownWidth()                 => field -> value
+          case width @ KnownWidth(widthValue) =>
+            // TODO make this a warning then an error, but for older versions, just truncate.
+            val valuex = if (widthValue < value.width.get) {
+              // Mask the value to the width of the field.
+              val mask = (BigInt(1) << widthValue) - 1
+              value.cloneWithValue(value.num & mask).cloneWithWidth(width)
+            } else if (widthValue > value.width.get) value.cloneWithWidth(width)
+            // Otherwise, ensure width is same as that of the field.
+            else value
+
+            field -> valuex
+        }
+    }.toMap
+    clone.bind(BundleLitBinding(bundleLitMap))
     clone
   }
 

core/src/main/scala/chisel3/internal/firrtl/IR.scala

@@ -117,13 +117,19 @@ abstract class LitArg(val num: BigInt, widthArg: Width) extends Arg {
   }
 
   /** Provides a mechanism that LitArgs can have their width adjusted
-    * to match other members of a VecLiteral
     *
     * @param newWidth the new width for this
     * @return
     */
   def cloneWithWidth(newWidth: Width): this.type
 
+  /** Provides a mechanism that LitArgs can have their value adjusted.
+    *
+    * @param newWidth the new width for this
+    * @return
+    */
+  def cloneWithValue(newValue: BigInt): this.type
+
   protected def minWidth: Int
   if (forcedWidth) {
     require(
@@ -147,6 +153,8 @@ case class ULit(n: BigInt, w: Width) extends LitArg(n, w) {
     ULit(n, newWidth).asInstanceOf[this.type]
   }
 
+  def cloneWithValue(newValue: BigInt): this.type = ULit(newValue, w).asInstanceOf[this.type]
+
   require(n >= 0, s"UInt literal ${n} is negative")
 }
 
@@ -161,6 +169,8 @@ case class SLit(n: BigInt, w: Width) extends LitArg(n, w) {
   def cloneWithWidth(newWidth: Width): this.type = {
     SLit(n, newWidth).asInstanceOf[this.type]
   }
+
+  def cloneWithValue(newValue: BigInt): this.type = SLit(newValue, w).asInstanceOf[this.type]
 }
 
 @deprecated(deprecatedPublicAPIMsg, "Chisel 3.6")
@@ -174,6 +184,8 @@ case class FPLit(n: BigInt, w: Width, binaryPoint: BinaryPoint) extends LitArg(n
   def cloneWithWidth(newWidth: Width): this.type = {
     FPLit(n, newWidth, binaryPoint).asInstanceOf[this.type]
   }
+
+  def cloneWithValue(newValue: BigInt): this.type = FPLit(newValue, w, binaryPoint).asInstanceOf[this.type]
 }
 
 @deprecated(deprecatedMFCMessage, "Chisel 3.6")
@@ -194,6 +206,8 @@ case class IntervalLit(n: BigInt, w: Width, binaryPoint: BinaryPoint) extends Li
   def cloneWithWidth(newWidth: Width): this.type = {
     IntervalLit(n, newWidth, binaryPoint).asInstanceOf[this.type]
   }
+
+  def cloneWithValue(newValue: BigInt): this.type = IntervalLit(newValue, w, binaryPoint).asInstanceOf[this.type]
 }
 
 @deprecated(deprecatedPublicAPIMsg, "Chisel 3.6")

src/test/scala/chiselTests/BundleLiteralSpec.scala

@@ -345,6 +345,18 @@ class BundleLiteralSpec extends ChiselFlatSpec with Utils {
     exc.getMessage should include(".c")
   }
 
+  "bundle literals with too-wide of literal values" should "truncate" in {
+    class SimpleBundle extends Bundle {
+      val a = UInt(4.W)
+      val b = UInt(4.W)
+    }
+    val chirrtl = ChiselStage.emitCHIRRTL(new RawModule {
+      val lit = (new SimpleBundle).Lit(_.a -> 0xde.U, _.b -> 0xad.U)
+      val x = lit.asUInt
+    })
+    chirrtl should include("""node x = cat(UInt<4>("he"), UInt<4>("hd"))""")
+  }
+
   "partial bundle literals" should "fail to pack" in {
     ChiselStage.elaborate {
       new RawModule {
@@ -353,4 +365,19 @@ class BundleLiteralSpec extends ChiselFlatSpec with Utils {
       }
     }
   }
+
+  "bundle literals" should "use the widths of the Bundle fields rather than the widths of the literals" in {
+    class SimpleBundle extends Bundle {
+      val a = UInt(4.W)
+      val b = UInt(4.W)
+    }
+    val chirrtl = ChiselStage.emitCHIRRTL(new RawModule {
+      // Whether the user specifies a width or not.
+      val lit = (new SimpleBundle).Lit(_.a -> 0x3.U, _.b -> 0x3.U(3.W))
+      lit.a.getWidth should be(4)
+      lit.b.getWidth should be(4)
+      val cat = lit.asUInt
+    })
+    chirrtl should include("""node cat = cat(UInt<4>("h3"), UInt<4>("h3"))""")
+  }
 }

src/test/scala/chiselTests/ConnectableSpec.scala

@@ -1060,7 +1060,7 @@ class ConnectableSpec extends ChiselFunSpec with Utils {
           "out.valid <= in.valid",
           "in.ready <= out.ready",
           "out.data.b <= in.data.b",
-          "out.data.c <= UInt<1>(\"h1\")"
+          "out.data.c <= UInt<2>(\"h1\")"
         ),
         Nil
       )
@@ -1517,11 +1517,11 @@ class ConnectableSpec extends ChiselFunSpec with Utils {
         out,
         Seq(
           """wire w0 : { foo : UInt<3>, flip bar : UInt<3>}""",
-          """w0.bar <= UInt<1>("h1")""",
-          """w0.foo <= UInt<1>("h0")""",
+          """w0.bar <= UInt<3>("h1")""",
+          """w0.foo <= UInt<3>("h0")""",
           """wire w1 : { foo : UInt<3>, flip bar : UInt<3>}""",
-          """w1.bar <= UInt<1>("h1")""",
-          """w1.foo <= UInt<1>("h0")""",
+          """w1.bar <= UInt<3>("h1")""",
+          """w1.foo <= UInt<3>("h0")""",
           """w1 <= w0"""
         ),
         Nil
@@ -1607,7 +1607,7 @@ class ConnectableSpec extends ChiselFunSpec with Utils {
       testCheck(
         out,
         Seq(
-          """out.data <= UInt<1>("h0")""",
+          """out.data <= UInt<32>("h0")""",
           """in.ready <= out.ready""",
           """out.valid <= in.valid"""
         ),

src/test/scala/chiselTests/VecLiteralSpec.scala

@@ -437,6 +437,7 @@ class VecLiteralSpec extends ChiselFreeSpec with Utils {
 
   class VecExample extends RawModule {
     val out = IO(Output(Vec(2, new SubBundle)))
+    // Note that 22.U is too wide for bar so gets truncated below.
     val bundle = Vec(2, new SubBundle).Lit(
       0 -> (new SubBundle).Lit(_.foo -> 42.U, _.bar -> 22.U),
       1 -> (new SubBundle).Lit(_.foo -> 7.U, _.bar -> 3.U)
@@ -446,10 +447,10 @@ class VecLiteralSpec extends ChiselFreeSpec with Utils {
 
   "vec literals can contain bundles and should not be bulk connected" in {
     val chirrtl = ChiselStage.emitCHIRRTL(new VecExample)
-    chirrtl should include("""out[0].bar <= UInt<5>("h16")""")
-    chirrtl should include("""out[0].foo <= UInt<6>("h2a")""")
-    chirrtl should include("""out[1].bar <= UInt<2>("h3")""")
-    chirrtl should include("""out[1].foo <= UInt<3>("h7")""")
+    chirrtl should include("""out[0].bar <= UInt<4>("h6")""")
+    chirrtl should include("""out[0].foo <= UInt<8>("h2a")""")
+    chirrtl should include("""out[1].bar <= UInt<4>("h3")""")
+    chirrtl should include("""out[1].foo <= UInt<8>("h7")""")
   }
 
   "vec literals can have bundle children" in {
@@ -507,4 +508,20 @@ class VecLiteralSpec extends ChiselFreeSpec with Utils {
     vec.getWidth should be(16 * 2)
     vec.litValue should be(BigInt("bbbb000a", 16))
   }
+
+  "Vec literals should use the width of the Vec element rather than the widths of the literals" in {
+    val chirrtl = ChiselStage.emitCHIRRTL(new RawModule {
+      // Whether the user specifies a width or not.
+      val lit0 = (Vec(2, UInt(4.W))).Lit(0 -> 0x3.U, 1 -> 0x2.U(3.W))
+      lit0(0).getWidth should be(4)
+      lit0(1).getWidth should be(4)
+      val uint0 = lit0.asUInt
+      val lit1 = Vec.Lit(0x3.U, 0x2.U(4.W))
+      lit1(0).getWidth should be(4)
+      lit1(1).getWidth should be(4)
+      val uint1 = lit1.asUInt
+    })
+    chirrtl should include("""node uint0 = cat(UInt<4>("h2"), UInt<4>("h3"))""")
+    chirrtl should include("""node uint1 = cat(UInt<4>("h2"), UInt<4>("h3"))""")
+  }
 }