Skip to content
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

Merged
merged 5 commits into from
Nov 7, 2023

Conversation

phsauter
Copy link
Contributor

@phsauter phsauter commented Aug 8, 2023

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:

  • Test more synthetic cases for correctness and QoR
  • Test effects and correctness on a large module
  • Document somewhere

Copy link
Member

@povik povik left a 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.

passes/pmgen/peepopt_shiftadd.pmg Show resolved Hide resolved
passes/pmgen/peepopt_shiftadd.pmg Outdated Show resolved Hide resolved
passes/pmgen/peepopt_shiftadd.pmg Outdated Show resolved Hide resolved
passes/pmgen/peepopt_shiftadd.pmg Outdated Show resolved Hide resolved
passes/pmgen/peepopt_shiftadd.pmg Show resolved Hide resolved
passes/pmgen/peepopt_shiftadd.pmg Show resolved Hide resolved
passes/pmgen/peepopt_shiftadd.pmg Outdated Show resolved Hide resolved
passes/pmgen/peepopt_shiftadd.pmg Outdated Show resolved Hide resolved
@phsauter
Copy link
Contributor Author

phsauter commented Aug 9, 2023

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.

BTW we could make this work for var signed if we padded the output instead, that is, if inserted a dummy wire on the bottom offset bits
I think this should work? Initially I rejected the possibility of fiddling with the output because of concerns it may increase the number of MUX cells again. Now I realize that this isn't necessary, we can just pad it after the cell so it shouldn't change anything there.

@phsauter
Copy link
Contributor Author

phsauter commented Aug 9, 2023

This new version just passed the yosys tests on ubuntu.

I think its about time to try shiftmul and shiftadd together on pulp-platform/iguana.

@phsauter phsauter marked this pull request as ready for review October 16, 2023 15:15
@phsauter phsauter force-pushed the peepopt-shiftadd branch 2 times, most recently from a9915d6 to 963b017 Compare November 6, 2023 13:00
- 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.
@phsauter
Copy link
Contributor Author

phsauter commented Nov 6, 2023

As discussed in the dev-meeting:
I will add the pattern-matching tests for this in a future PR, this one here is considered complete.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

part-selects ($shift, $shiftx) create huge MUX-trees
3 participants