-
Notifications
You must be signed in to change notification settings - Fork 603
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Don't use MuxLookup default for full mapping #1201
Conversation
c2bb542
to
2a2546f
Compare
e650ba9
to
8aed5a0
Compare
8aed5a0
to
7726e2f
Compare
@jackkoenig I would review this, but I wrote some of the code, so you should take a look. The literal exhaustiveness check handles the bitwidth corner case oddities, too. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
1 question/concern, otherwise, LGTM!
This modifies MuxLookup to not use the 'default' mapping argument if a "full" mapping is provided. A "full" mapping enumerates all possible cases for a 'key' argument of a known size. This will check literal values to ensure exhaustiveness holds. Signed-off-by: Schuyler Eldridge <schuyler.eldridge@ibm.com> Co-authored-by: Schuyler Eldridge <schuyler.eldridge@ibm.com> Co-authored-by: Albert Magyar <albert.magyar@gmail.com>
Signed-off-by: Schuyler Eldridge <schuyler.eldridge@ibm.com> Co-authored-by: Schuyler Eldridge <schuyler.eldridge@ibm.com> Co-authored-by: Albert Magyar <albert.magyar@gmail.com>
34e4817
to
1e1c9f8
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
If you use
MuxLookup
with a full mapping, i.e., you fully enumerate all possible values for a key, you still wind up with unused hardware being generated in the case of aMux
failure case that is never used and which FIRRTL does not optimize away.This PR modifies
MuxLookup
to examine if the user has provided a full mapping and to not emit the additional failure case for the initialMux
. In effect, thedefault
is immediately discarded and the first mapping becomes the default.This generally improves Verilog generation for users that are using
MuxLookup
like a Verilog case statement.This would benefit from tests, but there weren't any tests already written. 😭 I can write them (read: I have not fully tested this). I just figured I would drop the PR first.
Example
For the following Chisel:
Without this PR:
With this PR:
Related issue:
Type of change: other enhancement
Impact: API modification (in terms of generating different Verilog)
Development Phase: implementation
Release Notes
MuxLookup
code generation