Bundle elements ordering fix #4226
Conversation
|
I want to clarify my statement that "The right order is the order of the fields in the definition of the Bundle". The order I'm talking about here is in the FIRRTL (and thus the Verilog). It is a well-known mild annoyance that the elements of a For fixing #4223 I think we should stick to just fixing the issue where object creation order sometimes violates field order. |
There was a problem hiding this comment.
make sure this cookbook is still valid whatever you do... my most bookmarked: https://www.chisel-lang.org/docs/cookbooks/cookbook#how-do-i-create-a-bundle-from-a-uint
| ) | ||
| } | ||
|
|
||
|
|
There was a problem hiding this comment.
seems like the cases we want to see are cross products of:
a. vals in the constructor args
b. vals in the body
c. vals in superclass/traits bodies
We should have a clear story on the order of these and the order within each of a, b, c and then show that as expected they go in order with a few test cases.
There was a problem hiding this comment.
before the PR, i think that b had expected behavior that we don't want to change in this PR except for weird interactions flagged by the bug
| @@ -410,7 +410,7 @@ private[chisel3] object Converter { | |||
| fir.Field(getRef(elt, info).name, fir.Flip, extractType(elt, false, info, checkProbe, true, typeAliases)) | |||
| } | |||
| if (!t._isOpaqueType) | |||
| fir.BundleType(t._elements.toIndexedSeq.reverse.map { case (_, e) => eltField(e) }) | |||
| fir.BundleType(t._elements.toIndexedSeq.map { case (_, e) => eltField(e) }) | |||
There was a problem hiding this comment.
you can't make this change here because it is affecting all Records, not just Bundles. So even if you undo the reversing for Bundles, this change is now messing with records so .asUint on a record would have different behavior.
YOu could show this by taking the cookbook here https://www.chisel-lang.org/docs/cookbooks/cookbook#how-do-i-create-a-bundle-from-a-uint and rewriting it with a record and convincing yourself it doesn't work the same after this change
| @@ -44,27 +44,27 @@ class BpipDecoupled extends BpipOneField { | |||
| class HasDecoupledBundleByInheritance extends Module { | |||
There was a problem hiding this comment.
add more tests that mess with object creation order like have static objects, call-by-name, gens, etc
|
it seems like part of the confusion here is that .asUInt is not actually documented in the firrtl spec in anyway how it is supposed to function. |
adkian-sifive
added
API Modification
Internal
jackkoenig
added
API Modification
and removed
Internal
| "Bundle elements should be ordered in order of declaration" in { | ||
| val chirrtl = ChiselStage.emitCHIRRTL(new SimpleBundleElemOrder) | ||
| chirrtl should include("input in1 : { a : UInt<1>, b : UInt<8>}") | ||
| chirrtl should include("input in2 : { a : UInt<1>, b : UInt<8>}") | ||
|
|
||
| val chirrtl2 = ChiselStage.emitCHIRRTL(new SimpleBundleElemOrderByName) | ||
| chirrtl2 should include("input in0 : { a : UInt<8>, b : UInt<8>, c : UInt<8>}") | ||
| chirrtl2 should include("input in1 : { a : UInt<8>, b : UInt<8>, c : UInt<8>}") | ||
| } | ||
|
|
There was a problem hiding this comment.
These are good, but it would also be nice to do some assertElementsMatchExpected as that's the thing that changed (i.e. that you fixed) in this PR.
Fixes #4223. Straighten out the reversing of bundle elements by removing sorting. Add reversals to fix ordering and fix tests
Contributor Checklist
docs/src?Type of Improvement
Desired Merge Strategy
Release Notes
All
Bundleelements will be ordered by the order of their declaration, including by inheritance.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), clean up the commit message, and label withPlease Merge.Create a merge commit.