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

Fix ConstantFieldPropagation signed packed field handling and improve Heap2Local's #6493

Merged
merged 3 commits into from
Apr 11, 2024

Conversation

kripken
Copy link
Member

@kripken kripken commented Apr 11, 2024

CFP already had logic for truncating but not for sign-extending, which this
fixes.

Use the new helper function in Heap2Local as well. This changes the model
there from "truncate on set, sign-extend on get" to "truncate or sign-extend
on get". That is both simpler by reusing the same logic as CFP but also more
optimal: the idea to truncate on sets made sense since sets are rarer, but if
we must then sign-extend on gets then we can end up doing more work
overall (as the truncations on sets are not needed if all gets are signed).

Found by #6486

Comment on lines +799 to +800
// general. However, signed gets make that more complicated, so leave this
// for other opts to handle.
Copy link
Member

Choose a reason for hiding this comment

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

How do signed gets make that more complicated? If you truncate the value on the set and sign extend it again on the get, you'll still get the correct value back. Can't we leave the redundant truncation and sign extension to be cleaned up by other ops instead?

Copy link
Member Author

Choose a reason for hiding this comment

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

It's more complicated in that it's hard to see what is best here. That is, correctness is easy but we'd need to count how many gets are signed and how many are unsigned, to know which model is more efficient. So it is simplest to do the simple thing (fix up reads in one place), and leave optimizing that further to another pass.

(Atm nothing optimizes this further, but we could do it - basically count the sign-ext operations on local.gets etc. and maybe decide to shift from fixups on gets to sets or vice versa. We actually do something similar for linear memory loads and stores, but not locals.)

Copy link
Member Author

Choose a reason for hiding this comment

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

Or, to put it another way, this PR simplifies Heap2Local code while also making it more efficient in at least one case (the case where there are many signed reads and many writes: we did the worst possible thing in that case before, which was to do a fixup on both those reads and those writes).

Copy link
Member

Choose a reason for hiding this comment

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

Makes sense 👍

@kripken kripken merged commit 0b0c338 into WebAssembly:main Apr 11, 2024
13 checks passed
@kripken kripken deleted the cfp.sign branch April 11, 2024 19:45
kripken added a commit that referenced this pull request Apr 11, 2024
GUFA already truncated packed fields on write, which is enough for unsigned gets,
but for signed gets we also need to sign them on reads.

Similar to #6493 but for GUFA. Also found by #6486
@gkdn gkdn mentioned this pull request Aug 31, 2024
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.

2 participants