-
Notifications
You must be signed in to change notification settings - Fork 78
Add support for $_BUF_ cell type in Yosys import.
#2832
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
base: master
Are you sure you want to change the base?
Conversation
1889d71 to
6925c2a
Compare
RyanGlScott
left a comment
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.
Is it reasonable to add a test case for this?
| , ("$bmux" , CellTypeBmux) | ||
| , ("$dff" , CellTypeDff) | ||
| , ("$ff" , CellTypeFf) | ||
| , ("$_BUF_" , CellTypeBUF) |
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.
This adds just one of the cell types listed here. Do you foresee the need to add support for the others? (If so, I'd be fine with deferring that to a separate issue.)
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.
It really depends on what kinds of output we see from our synthesis tools. We could go ahead and implement all of the gate types listed in the Yosys cell library, but if we don't have any generated circuits that use a particular gate type, then we can't really test the implementation properly. It might be the case that $_BUF_ is the only additional gate type that TabbyCAD ever adds, and if so it might be best to hold off on implementing the others.
|
Yes, we should definitely add a test case for this. The thread for #2828 doesn't include a proof script, but it says it's based on an example from a tutorial, so I should be able to put something together. |
| Map.empty | ||
| sorted | ||
| convertYosysIR sc ir = | ||
| do let mg = yosysIRModgraph ir |
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.
FWIW, I rather prefer the do positioning the other way. Which is not to say you shouldn't change it here. But maybe we should talk about it offline...
|
With this patch, I can indeed load the TabbyCAD generated |
|
For what it's worth, the already-existing |
This greatly reduces the number of needed uses of liftIO.
For readability, 'do' and 'case' go on the left.
6925c2a to
93505e7
Compare
|
I added a new test case that loads both .json files and proves that the |
Also do a bunch of code style changes and refactorings.
Fixes #2828.
Note that because this branch does a lot of code reformatting and reindentation that touches a huge number of lines of code, it is probably easiest to review by looking at individual commits.