-
Notifications
You must be signed in to change notification settings - Fork 178
Make CheckWidths accept zero-width UIntLiterals #530
base: master-deprecated
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
| module Top : | ||
| output x: UInt<3> | ||
| x <= UInt<0>(0)""".stripMargin | ||
val check = | ||
"""module Top( | ||
| output [2:0] x |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
shouldn't you check if x is assigned to be zero?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This diff is messed up, this check for this test is identical to the one above so git strangely put the new code in the middle, but if you look below there is more to the new test.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, that's super weird, if I view the whole file it looks fine.
This change seems to lead to a zero-width Vec issue in the Chisel tests. Looking into it |
(Well I think this one combined with the one where Chisel emits zero width UInts) |
@colinschmidt Can you confirm this works? |
Here's the Chisel branch, looking in to the Chisel test failure: https://github.com/ucb-bar/chisel3/tree/one-entry-enum |
Thanks @jackkoenig I'll test with that branch and this firrtl branch |
Hmm I got a bunch of these errors:
|
It seems like there might be some hidden zero-width wire problems in Firrtl, perhaps we should just patch Chisel by not emitting zero-width for 1-entry Enums and at least fix THAT problem |
I added a testcase that currently fails so that a fix for it (and possibly other issues) can accompany this change. |
#530) * Revert "Change Vec creation to check if gen is lit (and hence needs to be declared)" This reverts commit dc86e7e1734d6abacb739b488df1de231e6b41b2. This may address #522 - using chiselCloneType (instead of cloneType) to preserve directionality. * Add missing implicits to Vec.apply() signature. * Use correct macro (CompileOptionsTransform) for indexWhere.
Any updates on this? Is this dated? |
No updates, I think it's a lingering problem |
All tests (FIRRTL) currently pass. I'm running regression tests with the rest of the BIG6. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks ok to me (weird diff not withstanding)
Should get branch updated
* Fixes #508 Co-authored-by: Jack Koenig <[email protected]>
fd3f0c1
to
f0cd6a8
Compare
I rebased and cleaned up. The previously failing Vec test works now; should we consider merging this now @jackkoenig? |
Fixes #508
Release notes:
Zero-width literals with value zero are now allowed.