Wasmtime GitHub notifications bot (Dec 08 2020 at 19:56): Wasmtime GitHub notifications bot (Dec 08 2020 at 19:56):cfallin requested abrown and julian-seward1 for a review on PR #2490.
Wasmtime GitHub notifications bot (Dec 08 2020 at 19:56):cfallin requested abrown and julian-seward1 for a review on PR #2490.
Wasmtime GitHub notifications bot (Dec 08 2020 at 19:56):cfallin opened PR #2490 from fix-popcnt-load-width to main:
As a subtle consequence of the recent load-op fusion, popcnt of a
value that came from a load.i32 was compiling into a 64-bit load. This
is a result of the way in which x86 infers the width of loads: it is a
consequence of the instruction containing the memory reference, not the
memory reference itself. So theinput_to_reg_mem()helper (convert an
instruction input into a register or memory reference) was providing the
appropriate memory reference for the result of a load.i32, but never
encoded the assumption that it would only be used in a 32-bit
instruction. It turns out that popcnt.i32 uses a 64-bit instruction to
load this RM op, hence widening a 32-bit to 64-bit load (which is
problematic when the offset is (memory_length - 4)).Separately, popcnt was using the RM operand twice, resulting in two
loads if we merged a load. This isn't a correctness bug in practice
because only a racy sequence (store interleaving between the loads)
would produce incorrect results, but we decided earlier to treat loads
as effectful for now, neither reordering nor duplicating them, to
deliberately reduce complexity.Because of the second issue, the fix is just to force the operand into a
register always, so any source load will not be merged.Discovered via fuzzing with oss-fuzz.
<!--
Please ensure that the following steps are all taken care of before submitting
the PR.
[ ] This has been discussed in issue #..., or if not, please tell us why
here.[ ] A short description of what this does, why it is needed; if the
description becomes long, the matter should probably be discussed in an issue
first.[ ] This PR contains test cases, if meaningful.
- [ ] A reviewer from the core maintainer team has been assigned for this PR.
If you don't know who could review this, please indicate so. The list of
suggested reviewers on the right can help you.Please ensure all communication adheres to the code of conduct.
-->
Wasmtime GitHub notifications bot (Dec 08 2020 at 20:18):bjorn3 submitted PR Review.
Wasmtime GitHub notifications bot (Dec 08 2020 at 20:18):bjorn3 created PR Review Comment:
// N.B.: explicitly put input in a reg here because the width of the instruction
Wasmtime GitHub notifications bot (Dec 08 2020 at 20:24):cfallin submitted PR Review.
Wasmtime GitHub notifications bot (Dec 08 2020 at 20:24):cfallin created PR Review Comment:
Thanks!
Wasmtime GitHub notifications bot (Dec 08 2020 at 20:24):cfallin updated PR #2490 from fix-popcnt-load-width to main:
As a subtle consequence of the recent load-op fusion, popcnt of a
value that came from a load.i32 was compiling into a 64-bit load. This
is a result of the way in which x86 infers the width of loads: it is a
consequence of the instruction containing the memory reference, not the
memory reference itself. So theinput_to_reg_mem()helper (convert an
instruction input into a register or memory reference) was providing the
appropriate memory reference for the result of a load.i32, but never
encoded the assumption that it would only be used in a 32-bit
instruction. It turns out that popcnt.i32 uses a 64-bit instruction to
load this RM op, hence widening a 32-bit to 64-bit load (which is
problematic when the offset is (memory_length - 4)).Separately, popcnt was using the RM operand twice, resulting in two
loads if we merged a load. This isn't a correctness bug in practice
because only a racy sequence (store interleaving between the loads)
would produce incorrect results, but we decided earlier to treat loads
as effectful for now, neither reordering nor duplicating them, to
deliberately reduce complexity.Because of the second issue, the fix is just to force the operand into a
register always, so any source load will not be merged.Discovered via fuzzing with oss-fuzz.
<!--
Please ensure that the following steps are all taken care of before submitting
the PR.
[ ] This has been discussed in issue #..., or if not, please tell us why
here.[ ] A short description of what this does, why it is needed; if the
description becomes long, the matter should probably be discussed in an issue
first.[ ] This PR contains test cases, if meaningful.
- [ ] A reviewer from the core maintainer team has been assigned for this PR.
If you don't know who could review this, please indicate so. The list of
suggested reviewers on the right can help you.Please ensure all communication adheres to the code of conduct.
-->
Wasmtime GitHub notifications bot (Dec 09 2020 at 05:18):julian-seward1 submitted PR Review.
Wasmtime GitHub notifications bot (Dec 09 2020 at 06:28):cfallin merged PR #2490.
Last updated: Dec 23 2024 at 13:07 UTC