Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix widths for literal values in Bundle literals #4082

Merged
merged 3 commits into from
May 23, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 23 additions & 4 deletions core/src/main/scala/chisel3/Aggregate.scala
Original file line number Diff line number Diff line change
Expand Up @@ -1122,10 +1122,10 @@ abstract class Record extends Aggregate {

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,
Expand Down Expand Up @@ -1207,12 +1207,31 @@ abstract class Record extends Aggregate {
}

// 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
}

Expand Down
14 changes: 12 additions & 2 deletions core/src/main/scala/chisel3/internal/firrtl/IR.scala
Original file line number Diff line number Diff line change
Expand Up @@ -121,14 +121,20 @@ private[chisel3] object ir {
elem
}

/** Provides a mechanism that LitArgs can have their width adjusted
* to match other members of a VecLiteral
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Stray change.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not stray actually, I am calling the function from Bundle literals now lol.

/** Provides a mechanism that LitArgs can have their width adjusted.
*
* @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(
Expand All @@ -150,6 +156,8 @@ private[chisel3] object ir {
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")
}

Expand All @@ -163,6 +171,8 @@ private[chisel3] object ir {
def cloneWithWidth(newWidth: Width): this.type = {
SLit(n, newWidth).asInstanceOf[this.type]
}

def cloneWithValue(newValue: BigInt): this.type = SLit(newValue, w).asInstanceOf[this.type]
}

/** Literal property value.
Expand Down
27 changes: 27 additions & 0 deletions src/test/scala/chiselTests/BundleLiteralSpec.scala
Original file line number Diff line number Diff line change
Expand Up @@ -344,6 +344,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>(0he), UInt<4>(0hd))")
}

"partial bundle literals" should "fail to pack" in {
ChiselStage.emitCHIRRTL {
new RawModule {
Expand All @@ -367,4 +379,19 @@ class BundleLiteralSpec extends ChiselFlatSpec with Utils {
lit.litOption should equal(Some(0))
})
}

"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>(0h3), UInt<4>(0h3))")
}
}
12 changes: 6 additions & 6 deletions src/test/scala/chiselTests/ConnectableSpec.scala
Original file line number Diff line number Diff line change
Expand Up @@ -1060,7 +1060,7 @@ class ConnectableSpec extends ChiselFunSpec with Utils {
"connect out.valid, in.valid",
"connect in.ready, out.ready",
"connect out.data.b, in.data.b",
"connect out.data.c, UInt<1>(0h1)"
"connect out.data.c, UInt<2>(0h1)"
),
Nil
)
Expand Down Expand Up @@ -1535,11 +1535,11 @@ class ConnectableSpec extends ChiselFunSpec with Utils {
out,
Seq(
"""wire w0 : { foo : UInt<3>, flip bar : UInt<3>}""",
"""connect w0.bar, UInt<1>(0h1)""",
"""connect w0.foo, UInt<1>(0h0)""",
"""connect w0.bar, UInt<3>(0h1)""",
"""connect w0.foo, UInt<3>(0h0)""",
"""wire w1 : { foo : UInt<3>, flip bar : UInt<3>}""",
"""connect w1.bar, UInt<1>(0h1)""",
"""connect w1.foo, UInt<1>(0h0)""",
"""connect w1.bar, UInt<3>(0h1)""",
"""connect w1.foo, UInt<3>(0h0)""",
"""connect w1, w0"""
),
Nil
Expand Down Expand Up @@ -1625,7 +1625,7 @@ class ConnectableSpec extends ChiselFunSpec with Utils {
testCheck(
out,
Seq(
"""connect out.data, UInt<1>(0h0)""",
"""connect out.data, UInt<32>(0h0)""",
"""connect in.ready, out.ready""",
"""connect out.valid, in.valid"""
),
Expand Down
25 changes: 21 additions & 4 deletions src/test/scala/chiselTests/VecLiteralSpec.scala
Original file line number Diff line number Diff line change
Expand Up @@ -436,6 +436,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)
Expand All @@ -445,10 +446,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("""connect out[0].bar, UInt<5>(0h16)""")
chirrtl should include("""connect out[0].foo, UInt<6>(0h2a)""")
chirrtl should include("""connect out[1].bar, UInt<2>(0h3)""")
chirrtl should include("""connect out[1].foo, UInt<3>(0h7)""")
chirrtl should include("""connect out[0].bar, UInt<4>(0h6)""")
chirrtl should include("""connect out[0].foo, UInt<8>(0h2a)""")
chirrtl should include("""connect out[1].bar, UInt<4>(0h3)""")
chirrtl should include("""connect out[1].foo, UInt<8>(0h7)""")
}

"vec literals can have bundle children" in {
Expand Down Expand Up @@ -530,4 +531,20 @@ class VecLiteralSpec extends ChiselFreeSpec with Utils {
lit.litOption should equal(Some(0))
})
}

"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>(0h2), UInt<4>(0h3))")
chirrtl should include("node uint1 = cat(UInt<4>(0h2), UInt<4>(0h3))")
}
}
Loading