Avoid emitting 'const const' for const vecs of const elements #3393
Conversation
|
I realize now I need to handle vecs of vecs where the intermediate vec isn’t const |
There was a problem hiding this comment.
LGTM
The PR needs a label for the release generation flow. Could you do two things:
- Add the label. Probably "Codegen"
- Populate the "Release Notes" section of the PR template.
Jack wrote some cool automation to use these two things to automatically generate the release notes.
I added the release notes. I don't believe I'm able to add a label (I'm not a member of this repo) unless I'm missing something. |
mwachs5
added
the
Backend Code Generation
mwachs5
added
the
Please Merge
| case ConstType(underlying: Type) => { | ||
| // Avoid emitting multiple consecurive 'const', which can otherwise occur for const vectors of const elements | ||
| if (!lastEmittedConst) { | ||
| b ++= "const " | ||
| } | ||
| s(underlying, true)(b, indent) | ||
| } |
There was a problem hiding this comment.
I'm of two minds about this solution. I'm a bit concerned because this same issue is solved for directions in the Converter (
), and presumably we don't have it solved for probes. I'm worried that when we fold the Converter and Serializer together, we will lose this fix. I grant there is some elegance to the spot in that it ensures that any FIRRTL IR will be emitted properly whereas the Converter fix doesn't guarantee that. But we don't create FIRRTL IR any other way so I think I prefer the further upstream fix.Better yet would be if we could pull the fix further up into the actual construction of the Chisel types such that these fixups aren't necessary.
Contributor Checklist
docs/src?Type of Improvement
Desired Merge Strategy
Release Notes
Multiple consecutive 'const' modifiers are no longer emitted when emitting a const vector of const elements.
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.