Stream: git-wasmtime

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


view this post on Zulip 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.

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

<!--

Please ensure that the following steps are all taken care of before submitting
the PR.

Please ensure all communication adheres to the code of conduct.
-->

view this post on Zulip Wasmtime GitHub notifications bot (Oct 27 2022 at 19:16):

cfallin created PR review comment:

Likewise here as above

view this post on Zulip Wasmtime GitHub notifications bot (Oct 27 2022 at 19:16):

cfallin submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 27 2022 at 19:16):

cfallin submitted PR review.

view this post on Zulip 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?

view this post on Zulip 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...

view this post on Zulip Wasmtime GitHub notifications bot (Oct 27 2022 at 19:16):

cfallin created PR review comment:

Can we use RexFlags here?

view this post on Zulip Wasmtime GitHub notifications bot (Oct 27 2022 at 19:34):

jameysharp submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 27 2022 at 19:34):

jameysharp submitted PR review.

view this post on Zulip 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.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 28 2022 at 23:22):

11evan updated PR #5147 from bswap to main.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 28 2022 at 23:24):

11evan submitted PR review.

view this post on Zulip 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

view this post on Zulip Wasmtime GitHub notifications bot (Oct 28 2022 at 23:25):

11evan submitted PR review.

view this post on Zulip 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

view this post on Zulip 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

view this post on Zulip Wasmtime GitHub notifications bot (Oct 28 2022 at 23:26):

11evan submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 28 2022 at 23:32):

11evan updated PR #5147 from bswap to main.

view this post on Zulip 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.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 28 2022 at 23:48):

jameysharp submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 29 2022 at 10:30):

afonso360 submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 29 2022 at 14:49):

afonso360 submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 31 2022 at 16:20):

11evan updated PR #5147 from bswap to main.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 31 2022 at 16:24):

11evan submitted PR review.

view this post on Zulip 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.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 31 2022 at 17:27):

jameysharp submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 31 2022 at 17:28):

jameysharp has enabled auto merge for PR #5147.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 31 2022 at 19:30):

jameysharp merged PR #5147.


Last updated: Dec 23 2024 at 12:05 UTC