Skip to content

opt_expr: fix sign extension for shifts #5066

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

Merged
merged 3 commits into from
Apr 28, 2025
Merged

Conversation

georgerennie
Copy link
Collaborator

@georgerennie georgerennie commented Apr 26, 2025

This fixes #5065. When optimizing a shift with constant shift amount, an optimization is used to limit the shift amount to the width of the input, but this limiting was done before potentially increasing the input's size by sign-extending it. This allowed the sign extended parts of the signal to be shifted into the output bits incorrectly. This PR defers the optimization until after the sign extension has happened to handle this and adds tests.

Could someone make sure that this doesn't break anything for $shift/$shiftx before merging please, I wasn't sure where they come from/what the difference is to the other shift cells.

@georgerennie georgerennie force-pushed the george/opt_expr_shr_sign branch from ebeef64 to 70a44f0 Compare April 26, 2025 10:40
@georgerennie
Copy link
Collaborator Author

Test failures seem to be that the testcase I added triggers UB in celledges

@georgerennie
Copy link
Collaborator Author

Have fixed that, seems this potential source of UB was known as there was b_width and b_width_capped but the former was still being used in some places. Switching to the latter everywhere fixes this (with the caveat in a comment above its description that if the shifted value is near 2**30 wide things will go wrong)

@georgerennie georgerennie changed the title opt_expr: only sign extend shift arguments for arithmetic right shift opt_expr: fix sign extension for shifts Apr 26, 2025
Copy link
Member

@KrystalDelusion KrystalDelusion left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @georgerennie ! Looks good to me.

@KrystalDelusion KrystalDelusion added the merge-soon Merge: PR will be merged at the end of the next work day unless concerns are raised label Apr 27, 2025
@KrystalDelusion KrystalDelusion merged commit bfe0596 into main Apr 28, 2025
43 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merge-soon Merge: PR will be merged at the end of the next work day unless concerns are raised
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Incorrect optimization after large constant shift (>> 32) in signed input
2 participants