-
Notifications
You must be signed in to change notification settings - Fork 63
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
Fix bugs in several Yosys cell implementations #1817
Conversation
Out of curiosity, what were the actual buggy parts of the code? (It's somewhat hard to tell due to all of the adjacent refactorings in the diff.) |
Specifically, the issues were:
|
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.
Ah, OK. Part of me wonders if we should always call flipEndianness
before calling each operation to avoid issues like this in the future, but then again, that would impose an additional simulation-time cost. In lieu of that, perhaps it is worth grouping up the operations in the code that are sensitive to the endianness so that is easier to tell them apart.
I don't have much insight into the rest of the code, so my approval here is mostly a formality :)
Yeah, I thought about this, but the term size increase scared me off (in addition to simulation time, I find myself reading parts of the resulting SAWCore terms quite often, and lots of redundant reverses make that obnoxious). There are probably some relatively easy solutions but for now Thanks for the review! |
Several SAW implementations of the semantics of Yosys cells did not match the expected behavior. This PR hopefully fixes a few instances of this (there are undoubtedly more). I also refactored out some duplicated code for e.g. handling bit order - this is a common source of error in the semantics, as the convention in SAWCore is opposite Yosys.
Longer-term, we should ensure that we have good reason to trust these cell definitions - if they're incorrect, we're not reasoning about anything useful! I think at least some randomized testing would be a good start, and I plan to implement something to address this in the future. #1816 tracks this need.