-
Notifications
You must be signed in to change notification settings - Fork 903
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
peepopt: Add shiftadd
pattern
#3883
Conversation
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.
I went over it, looks good, dropping the log2scale
logic and adding a wraparound check are the only points I consider important.
I applied most of the suggestions, I may have gone overboard with moving everything to the match-block. Personally I would leave the log2scale code as I think it isn't too expensive and makes it just a little more versatile, you never know what RTLIL this may encounter.
|
This new version just passed the yosys tests on ubuntu. I think its about time to try |
f2911b3
to
232eee5
Compare
9522556
to
bd3aec9
Compare
bd3aec9
to
fb9dcbc
Compare
a9915d6
to
963b017
Compare
- moved all selection and filtering logic to the match block - applied less-verbose code suggestions - removed constraint on number of bits in shift-amount - added check for possible wrap-arround in the operation
This expands the part-select tests with one additional module. It specifically tests the different variants of the `peepopt` optimizations `shiftadd` and `shiftmul`. Not all these cases are actually transformed using `shiftadd`, including them also checks if the correct variants are rejected.
963b017
to
c3b8de5
Compare
As discussed in the dev-meeting: |
This has been lightly discussed in #3873.
Together with #3873 and #3880 it should fix #3833.
Another approach to fix #3833 using attributes is shown in #3875 and #3877.
This transformation has been tested a bit but not completely and never on larger modules. Additionally the benefits to Quality-of-Results are as of now non-obvious and lack empirical evidence (though since #3880 does show benefits and this should allow more of these transformations, QoR should improve).
ToDo: