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

Add rotate left/right by constant amount #339

Closed
Ravenslofty opened this issue Mar 23, 2020 · 5 comments · Fixed by #352
Closed

Add rotate left/right by constant amount #339

Ravenslofty opened this issue Mar 23, 2020 · 5 comments · Fixed by #352
Labels
Milestone

Comments

@Ravenslofty
Copy link
Contributor

While it's not hard to roll your own in a function, it seems a bit more ergonomic to have Value.rotate_{left,right}() instead.

@whitequark
Copy link
Member

Variable rotates or constant rotates?

@Ravenslofty
Copy link
Contributor Author

Currently I only need constant rotates, but since you can build a rotate out of shifts, it shouldn't be difficult to also handle variable rotates.

@whitequark
Copy link
Member

Constant rotates are easy and uncontroversial. Variable rotates not so much because if the LHS is not a power of 2 and the RHS can represent an absolute value larger than the length of LHS, you get a (possibly signed) modulus operator in the expansion and that's likely going to result in a nasty synthesized netlist.

@Ravenslofty
Copy link
Contributor Author

Excellent point, I'd forgotten about that (and this is why I filed an issue first). I suppose it would have to be constant right hand side then.

@whitequark
Copy link
Member

whitequark commented Mar 23, 2020

We can always extend it to variable rotates later if someone comes up with a good way to handle this issue. So let's stick to constant rotates for now.

@whitequark whitequark changed the title Add rotate left/right Add rotate left/right by constant amount Mar 23, 2020
@whitequark whitequark added this to the 0.3 milestone Apr 14, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

Successfully merging a pull request may close this issue.

2 participants