Fix width inferencing issue #952
Conversation
| @@ -507,8 +507,8 @@ object Wire extends WireFactory | |||
| object WireInit { | |||
| def apply[T <: Data](init: T)(implicit sourceInfo: SourceInfo, compileOptions: CompileOptions): T = { | |||
| val model = (init match { | |||
| // For e.g. Wire(init=0.U(k.W)), fix the Reg's width to k | |||
There was a problem hiding this comment.
Thanks for updating the comment. One reason this was able to regress is that I only wrote the comment to cover the special case, and didn't explain the intended semantics for the other cases.
|
Can you write ScalaDoc that has a summary of the width inference rules for each function? Off the top of my head I don't see strong reasons for one being more intuitive than the other, so it makes sense to keep compatible behavior, but I also think that being able to express the logic succinctly and intuitively is a good sniff test and benchmark. |
|
This PR (and anything after chipsalliance/firrtl#952) will fail Jenkins build due to the new methods required for the FIRRTL |
|
@ducky64 ScalaDoc and more tests (namely for |
|
I'm open to better wording but AFAICT there really are the 3 cases. Literal Bits (set if width forced), Bits (inferred), and Aggregates (cloned from the Aggregate). I clarified the ScalaDoc a little bit, and added some to the actual |
|
Oh wait, I think I misunderstood. The "two cases" of Literals with defined width vs. other Bits could be collapsed into one and made more concise |
|
Yeah, I think it's nicer to have a very concise version, which you can always elaborate (eg edge cases) with illustrative examples. I'm more thinking of what gets a fixed width (only literals with defined width) or not (everything else it seems?). Should something on bundle literals also be included in the inference rules? |
|
I added a shorter summary for single-argument WireInit and RegInit. We should probably talk about bundle literals, but I'm thinking that can be follow on. I need this bug fix to bump chisel3 in rocket-chip so I would like to merge as soon as possible |
There was a problem hiding this comment.
Scaladoc looks good, it's pretty clear to me what the rules are.
I still have doubts on whether these are the semantics that should be - it seems like a code smell to have a unique special-case for non-literal-with-explicit-width Bits. But discussing later in the more general context of Bundle literals might be more useful.
|
Well, the time to reconsider those semantics was probably 2011 or 2012. |
* Added Foreachers * Changed CheckTypes to use foreach * Check widths now uses foreach * Finished merge, added foreachers to added stmts * Address reviewer feedback
As part of the Bundle literals framework PR, (#820) it appears a minor code refactor accidentally changed the width inference semantics of
WireInitandRegInit.For example
prior to #820, wire2 would have it's width inferred. After #820, wire2 gets its width set to 8.
This PR restores the previous behavior and adds some tests that pass prior to #820, fail on current master, and pass on this branch
I noticed this issue while trying to bump Chisel because it changed the functionality of the output FIRRTL ever so slightly but enough to make simulations fail.
Future work is to make similar tests that show and check the width inferencing semantics for other constructors (eg.
RegNext,RegEnable), as well as for aggregates (rather than justUInt).Related issue:
Type of change: bug report & fix
Impact: Undo API modification
Development Phase: implementation
Release Notes
Restore use of width inference to set width of RegInit and WireInit when initialized with a non-literal Bits