Stream: git-wasmtime

Topic: wasmtime / PR #5147 cranelift: Add Bswap instruction (#1092)

Wasmtime GitHub notifications bot (Oct 27 2022 at 18:52):

11evan opened PR #5147 from bswap to main:

Adds Bswap to the Cranelift IR. Implements the Bswap instruction in the x64 and aarch64 codegen backends. Cranelift users can now:

builder.ins().bswap(value)

to get a native byteswap instruction.

• x64: implements the 32- and 64-bit bswap instruction, following the pattern set by similar unary instrutions (Neg and Not) - it only operates on a dst register, but is parameterized with both a src and dst which are expected to be the same register.

As x64 bswap instruction is only for 32- or 64-bit registers, the 16-bit swap is implemented as a rotate left by 8.

• aarch64: Bswap gets emitted as aarch64 rev16, rev32, or rev64 instruction as appropriate.

• s390x: Bswap not implemented

• For completeness, added bswap to the interpreter as well.

<!--

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 (Oct 27 2022 at 19:16):

cfallin created PR review comment:

Likewise here as above

Wasmtime GitHub notifications bot (Oct 27 2022 at 19:16):

cfallin submitted PR review.

Wasmtime GitHub notifications bot (Oct 27 2022 at 19:16):

cfallin submitted PR review.

Wasmtime GitHub notifications bot (Oct 27 2022 at 19:16):

cfallin created PR review comment:

The little-endian bit literal here is kind of confusing (it's little-endian but MSB first); if we need an explanatory comment can we convert the above to a u32 (0xdac00562) or maybe just remove this comment?

Wasmtime GitHub notifications bot (Oct 27 2022 at 19:16):

cfallin created PR review comment:

No need for the blank line and comment here, I think...

Wasmtime GitHub notifications bot (Oct 27 2022 at 19:16):

cfallin created PR review comment:

Can we use RexFlags here?

Wasmtime GitHub notifications bot (Oct 27 2022 at 19:34):

jameysharp submitted PR review.

Wasmtime GitHub notifications bot (Oct 27 2022 at 19:34):

jameysharp submitted PR review.

Wasmtime GitHub notifications bot (Oct 27 2022 at 19:34):

jameysharp created PR review comment:

I'm wondering if we should exclude 8-bit integers for this instruction. There's no sensible implementation of byte-swapping when there's only one byte.

Wasmtime GitHub notifications bot (Oct 28 2022 at 23:22):

11evan updated PR #5147 from bswap to main.

Wasmtime GitHub notifications bot (Oct 28 2022 at 23:24):

11evan submitted PR review.

Wasmtime GitHub notifications bot (Oct 28 2022 at 23:24):

11evan created PR review comment:

These comments were just to help me out when writing the test cases; I've removed them now

Wasmtime GitHub notifications bot (Oct 28 2022 at 23:25):

11evan submitted PR review.

Wasmtime GitHub notifications bot (Oct 28 2022 at 23:25):

11evan created PR review comment:

Yeah good idea, changed to use RexFlags in update, and gave it a new emit_one_op fn to go along with the existing emit_two_op and emit_three_op

Wasmtime GitHub notifications bot (Oct 28 2022 at 23:26):

11evan created PR review comment:

Good idea, excluded 8-bit and also 128-bit since the latter is unimplemented for now

Wasmtime GitHub notifications bot (Oct 28 2022 at 23:26):

11evan submitted PR review.

Wasmtime GitHub notifications bot (Oct 28 2022 at 23:32):

11evan updated PR #5147 from bswap to main.

Wasmtime GitHub notifications bot (Oct 28 2022 at 23:48):

jameysharp created PR review comment:

Thanks for removing the 8-bit case! We've generally kept support for types which make sense even if there's no backend support, though, so I would put the 128-bit case back in. If nothing else, that lets us have a runtests/i128-bswap.clif that validates the interpreter (test interpret) even though we can't yet do test run on any target.

There are quite a few instructions that don't have 128-bit support on some backends yet, but it's good to have things in place to show what those instructions are expected to do for whenever somebody gets around to implementing them.

Wasmtime GitHub notifications bot (Oct 28 2022 at 23:48):

jameysharp submitted PR review.

Wasmtime GitHub notifications bot (Oct 29 2022 at 10:30):

afonso360 submitted PR review.

Wasmtime GitHub notifications bot (Oct 29 2022 at 14:49):

afonso360 submitted PR review.

Wasmtime GitHub notifications bot (Oct 31 2022 at 16:20):

11evan updated PR #5147 from bswap to main.

Wasmtime GitHub notifications bot (Oct 31 2022 at 16:24):

11evan submitted PR review.

Wasmtime GitHub notifications bot (Oct 31 2022 at 16:24):

11evan created PR review comment:

Ok sounds good! Added back 128-bit and created an interpreter test. I didn't actually implement any 128-bit swaps in the backends, left that for the future.

Wasmtime GitHub notifications bot (Oct 31 2022 at 17:27):

jameysharp submitted PR review.

Wasmtime GitHub notifications bot (Oct 31 2022 at 17:28):

jameysharp has enabled auto merge for PR #5147.

Wasmtime GitHub notifications bot (Oct 31 2022 at 19:30):

jameysharp merged PR #5147.

Last updated: Oct 23 2024 at 20:03 UTC