Support zero-width integer literals. #2932
Conversation
|
|
|
At least some of the CI failures are because current firtool used by CI is not new enough, PR bumping to latest submitted: #2933 . Looks like it's indeed important for |
Can't create -1.S(0.W), special-case zero-width.
These are in the FIRRTL language specification now and supported by both SFC + MFC. For SInt, special-case handle zero-width as not needing a sign bit. Zero-width literals must be explicitly zero-width (`0.U(0.W)`); literals such as `0.U` are (still) at least 1-bit.
dtzSiFive
force-pushed
the
feature/zero-width-literals
branch
from
2c8c68f to
3f11979
Compare
| it should "infer width as 1-bit if the template type has no width and is initialized to zero-width literal" in { | ||
| assertInferredWidth(1) { | ||
| val w = builder(UInt()) | ||
| w := 0.U(0.W) | ||
| w | ||
| } | ||
| assertInferredWidth(1) { | ||
| val w = builder(new ZeroWidthBundle) | ||
| w := ZeroWidthBundle.intoWire() | ||
| w.y | ||
| } | ||
| assertInferredWidth(1) { | ||
| val w = builder(Vec(1, UInt())) | ||
| w(0) := 0.U(0.W) | ||
| w(0) | ||
| } | ||
| } |
There was a problem hiding this comment.
This seems wrong to me (and super kudos for writing these tests because I would not have noticed this behavior without them).
I would expect Cat(1.U, 0.U(0.W), 1.U) to be isomorphic to Cat(1.U, WireInit(UInt(), 0.U(0.W)), 1.U), but this suggests otherwise, why?
Both MFC and SFC seem to be doing what I expect here (the result being 2'h3 in the Verilog), so I'm confused why the wire would infer width == 1?
Is it perhaps a test setup issue? Determining inferred width from Chisel is tricky and maybe it has a bug around zero-width.
There was a problem hiding this comment.
This seems wrong to me
Me too, honestly, thanks for bringing it up. I agree with your suggestion it may be a test setup issue (definitely was not 100% on those changes when I made them) and indeed it seems to have been! Will push a fix soon, happily it was actually really dumb with fresh eyes -- was using a 0.U which this PR went out of the way to make NOT zero-width (min 1-bit) which was why it wouldn't pass things that actually were zero-width. Making it 0.U(0.W) fixes that 👍 .
I would expect
Cat(1.U, 0.U(0.W), 1.U)to be isomorphic toCat(1.U, WireInit(UInt(), 0.U(0.W)), 1.U), but this suggests otherwise, why?
Fantastic test case, thanks!
Is this something suitable for a test as well? Not sure where / how that might work, especially without assuming constant propagation in the compiler and such, but for I really liked that (immediately seemed clear what you were trying to check/determine/say) and so would be great to check moving forward. If that's not really suitable, the width checks already present are probably enough. Who tests the testers, indeed.
( I've been testing various behavior using an increasingly modified blink example, so I can replicate your findings re:2'h3 for both (no surprise, for my own sanity) )
There was a problem hiding this comment.
Is this something suitable for a test as well? Not sure where / how that might work, especially without assuming constant propagation in the compiler and such, but for I really liked that (immediately seemed clear what you were trying to check/determine/say) and so would be great to check moving forward. If that's not really suitable, the width checks already present are probably enough.
Yeah I have the same thoughts/questions/concerns. I think it would be really handy to be able to express things like this, and perhaps a suite of formal checks for certain properties we expect to hold would be good. I think such a thing is beyond the scope of this PR though--I think the tests you've written are more than sufficient. Nice work 🙂
jackkoenig
left a comment
jackkoenig
left a comment
There was a problem hiding this comment.
This looks really good and I especially appreciate all of the tests. There is 1 test that isn't making sense to me but otherwise this LGTM.
Specifically, fix the inferred-width check. Start from zero-width literal, otherwise 0.U will need 1 bit.
jackkoenig
left a comment
jackkoenig
left a comment
There was a problem hiding this comment.
LGTM! Nice work and great tests!
* assert{Inferred,Known}Width: tweak to support width of zero.
Can't create -1.S(0.W), special-case zero-width.
* Bits: nicer error messages for zero-width
* Support zero-width integer literals.
These are in the FIRRTL language specification now
and supported by both SFC + MFC.
For SInt, special-case handle zero-width as not needing a sign bit.
Zero-width literals must be explicitly zero-width (`0.U(0.W)`);
literals such as `0.U` are (still) at least 1-bit.
* check zero-width to bool conversion
* onehot test
* UIntOps: test a few operations.
* SIntOps: test a few operations
* {S,U}IntOps: touchup test description, now includes head/tail/pad.
* ChiselSpec: Fix assert{Known,Inferred}Width for zero-width.
Specifically, fix the inferred-width check.
Start from zero-width literal, otherwise 0.U will need 1 bit.
* WidthSpec: update inferred zero-width test
(cherry picked from commit fc3b4a0)
* assert{Inferred,Known}Width: tweak to support width of zero.
Can't create -1.S(0.W), special-case zero-width.
* Bits: nicer error messages for zero-width
* Support zero-width integer literals.
These are in the FIRRTL language specification now
and supported by both SFC + MFC.
For SInt, special-case handle zero-width as not needing a sign bit.
Zero-width literals must be explicitly zero-width (`0.U(0.W)`);
literals such as `0.U` are (still) at least 1-bit.
* check zero-width to bool conversion
* onehot test
* UIntOps: test a few operations.
* SIntOps: test a few operations
* {S,U}IntOps: touchup test description, now includes head/tail/pad.
* ChiselSpec: Fix assert{Known,Inferred}Width for zero-width.
Specifically, fix the inferred-width check.
Start from zero-width literal, otherwise 0.U will need 1 bit.
* WidthSpec: update inferred zero-width test
(cherry picked from commit fc3b4a0)
Co-authored-by: Will Dietz <will.dietz@sifive.com>
3 participants
These are in the FIRRTL language specification now and supported by both SFC + MFC.
If integer literal width is specified, support zero-width (
0.U(0.W)), otherwise at least 1-bit (0.U), for compatibility with existing code.Modified
assertKnownWidth/assertInferredWidthto allow checking for width of zero, avoid attempting to use-1S(0.W). Used these to check basic zero-width behavior of wires/regs being initialized with zero-width integer literals.Add a few tests to ensure zero-width literals and a few operations/constructs work as expected, and that their behavior is preserved.
Contributor Checklist
docs/src?Type of Improvement
API Impact
Backend Code Generation Impact
Desired Merge Strategy
Release Notes
Add support for zero-width integer literals, which must be explicitly requested (
0.U(0.W)), for compatibility reasons0.Uwill still be (minimum) 1-bit.Reviewer Checklist (only modified by reviewer)
3.4.x, [small] API extension:3.5.x, API modification or big change:3.6.0)?Enable auto-merge (squash), clean up the commit message, and label withPlease Merge.Create a merge commit.