Make it possible to illegalize .asUInt on OpaqueTypes#3344
Conversation
| * | ||
| * Users can override this to increase the "opacity" of their type. | ||
| */ | ||
| protected def errorOnAsUInt: Boolean = false |
There was a problem hiding this comment.
perhaps they could override it with a string message...? so you could say something like
There was a problem hiding this comment.
I think you lost the second half of that sentence.
We could make the String message customizable, I'm curious what API you have in mind. I think the current error message is pretty good, eg. from the tests (admittedly hard to see since I also moved a bunch, but see the end of OpaqueTypeSpec.scala): "Field '_.bits' of type MaybeNoAsUInt does not support .asUInt.". That Arg.earlyLocalName trick is very much a private API and IMO you wouldn't want to lose that "coordinate" information when overriding the error message.
jackkoenig
force-pushed
the
jackkoenig/opaque-type-asuint
branch
from
6913848 to
0c41a39
Compare
| behavior.of("Empty Aggregates") | ||
|
|
||
| they should "have a width of 0" in { | ||
| assertKnownWidth(0) { | ||
| Wire(Vec(0, UInt(8.W))) | ||
| } | ||
| assertKnownWidth(0) { | ||
| Wire(new EmptyBundle) | ||
| } | ||
| } | ||
|
|
||
| // This is a bug that has existed for basically forever | ||
| // This really should be assertKnownWidth(0) | ||
| they should "result in a 1-bit UInt when calling .asUInt" in { | ||
| assertInferredWidth(1) { | ||
| val x = Wire(Vec(0, UInt(8.W))).asUInt | ||
| WireInit(x) | ||
| } | ||
| assertInferredWidth(1) { | ||
| val x = Wire(new EmptyBundle).asUInt | ||
| WireInit(x) | ||
| } | ||
| } |
There was a problem hiding this comment.
My original change resulted in some surprising test failures and exposed some concerning buggy behavior. As shown here, if you call .asUInt on an empty Aggregate, you get 0.U which is 0.U(1.W). Fortunately, when these empty Aggregates are part of larger Aggregates, they are flattened out (as zero-width wires), but in the special case of just .asUInt on a empty Aggregate, this is what happens.
This is definitely a bug, obviously it should hold that x.getWidth == x.asUInt.getWidth, but such width issues are tricky to change. This one derives from calling SeqUtils.asUInt which is the same reason why Cat(Seq()) returns 0.U (which again is 0.U(1.W)).
I think it's reasonable to fix this particular bug while leaving the Cat and other similar cases alone, but such changes are a little sketchy because it can result in width changes for downstream things and thus silently change behavior. In particular, it's scary if generator code is concatenating the result of .asUInt on a Vec that can be parameterized all the way down to size 0, or similarly with a Record or Bundle that could be empty.
Thoughts @aswaterman @azidar @seldridge @mwachs5?
There was a problem hiding this comment.
In any case, this PR is not really intended to be concerned with the concerning behavior and just documents and maintains the existing behavior.
There was a problem hiding this comment.
obviously it should hold that
x.getWidth == x.asUInt.getWidth
I think this is the overriding concern and fixing it is worth the pain.
jackkoenig
dismissed
mwachs5’s stale review
This PR has changed a little, not functionally but it does highlight some weird behavior.
jackkoenig
force-pushed
the
jackkoenig/opaque-type-asuint
branch
from
0c41a39 to
b69fa37
Compare
Subclasses of OpaqueType can override errorOnAsUInt to make it an elaboration time error if .asUInt is called on an instance of the particular type (including when nested inside of an Aggregate). This closes a large loophole in the OpaqueType API.
jackkoenig
force-pushed
the
jackkoenig/opaque-type-asuint
branch
from
b69fa37 to
d57ae63
Compare
3 participants
I also split the
OpaqueTypetests out ofRecordSpecsince we would like to make OpaqueTypes "opaquer" which ultimately means making them no longer be Records.Contributor Checklist
docs/src?Type of Improvement
Desired Merge Strategy
Release Notes
Subclasses of OpaqueType can override
errorOnAsUIntto make it an elaboration time error if .asUInt is called on an instance of the particular type (including when nested inside of an Aggregate). This closes a large loophole in the OpaqueType API.Reviewer Checklist (only modified by reviewer)
3.5.xor3.6.xdepending on impact, API modification or big change:5.0.0)?Enable auto-merge (squash), clean up the commit message, and label withPlease Merge.Create a merge commit.