Make ChiselEnum width inference consider known-width Value() literals
#4990
Conversation
jackkoenig
left a comment
jackkoenig
left a comment
There was a problem hiding this comment.
LGTM, thanks!
This isn't related to this PR, mostly a note to myself and @seldridge, but I noticed while reviewing:
You can still observe the smaller widths if you access them during the construction of the Enum object, e.g.
object EnumWithKnownWidth extends ChiselEnum {
val Member0 = Value(0.U(1.W))
println(Member0.getWith) // 1
val Member1 = Value(1.U(2.W))
println(Member0.getWith) // 2
val Member2 = Value(2.U(3.W))
println(Member0.getWith) // 3
val Member3 = Value(3.U(4.W))
println(Member0.getWith) // 4
This is a bit of a footgun but probably not that big of a deal in the grand scheme. It's not possible to give the right answer early on, maybe we could error but that would require being able to detect when the object is done being constructed which might be possible with runtime reflection. Unsure.
|
Thank you @CSharperMantle! |
I think the original implementation of ChiselEnum before this patch also behaves like this. Its width grows during its constructor as new members with longer bit length are added. So I guess a FIRRTL-level construct would be preferrable in terms of both aesthetics and "atomicity" like this. |
jackkoenig
added
API Modification
and removed
Bugfix
3 participants
Contributor Checklist
docs/src?Type of Improvement
Desired Merge Strategy
Release Notes
The inferred width for a
ChiselEnumusingValue()now correctly accommodates both known-width values and the largest specified literal integer. Closes #4989.Reviewer Checklist (only modified by reviewer)
3.6.x,5.x, or6.xdepending on impact, API modification or big change:7.0)?Enable auto-merge (squash)and clean up the commit message.Create a merge commit.