Stream: git-wasmtime

Topic: wasmtime / PR #2230 [machinst x64]: add lane operations

view this post on Zulip Wasmtime GitHub notifications bot (Sep 25 2020 at 17:36):

abrown opened PR #2230 from lane to main:

This change adds x64 implementations for extract_lane, replace_lane, shuffle, and swizzle as well as some saturating addition (necessary for swizzle). I cannot yet enable the simd_lane.wast test because it also uses other instructions like splat, all_true, and any_true but I did verify that the assertions in simd_lane.wast that do not use these instructions pass.

view this post on Zulip Wasmtime GitHub notifications bot (Sep 25 2020 at 17:40):

bjorn3 submitted PR Review.

view this post on Zulip Wasmtime GitHub notifications bot (Sep 25 2020 at 17:40):

bjorn3 created PR Review Comment:

The frontend could have incorrectly passed for example 2 as lane. Maybe add a debug_assert! that lane < src_ty.lane_count()?

view this post on Zulip Wasmtime GitHub notifications bot (Sep 25 2020 at 17:40):

abrown submitted PR Review.

view this post on Zulip Wasmtime GitHub notifications bot (Sep 25 2020 at 17:40):

abrown created PR Review Comment:

These assertions were only valid for CMPP*; with the addition of PINSR* and PEXTR* the reg classes can come from I64 in either slot (perhaps we could keep the imm assertion with imm < 16).

view this post on Zulip Wasmtime GitHub notifications bot (Sep 25 2020 at 23:57):

cfallin submitted PR Review.

view this post on Zulip Wasmtime GitHub notifications bot (Sep 25 2020 at 23:57):

cfallin submitted PR Review.

view this post on Zulip Wasmtime GitHub notifications bot (Sep 25 2020 at 23:57):

cfallin created PR Review Comment:

Little style preference mostly, but maybe do a let swapped = match *op { ... }; and then let (dst, src) = if swapped { ... } else { ... };? This keeps the code duplication down in case we add more logic around the emit_...() call later.

view this post on Zulip Wasmtime GitHub notifications bot (Sep 25 2020 at 23:57):

cfallin created PR Review Comment:

I'd prefer an assert!() here to ensure we avoid those opcodes, then avoid the extra nesting level of the if/else.

view this post on Zulip Wasmtime GitHub notifications bot (Sep 25 2020 at 23:57):

cfallin created PR Review Comment:

Doc comment and/or better name for this field perhaps? All the others are self-explanatory but I'm not sure what just w means without context...

view this post on Zulip Wasmtime GitHub notifications bot (Sep 25 2020 at 23:57):

cfallin created PR Review Comment:

Also, on preview below, the raw bool params to the encoding functions are somewhat bothersome; can we make this an enum instead for more self-documenting goodness?

view this post on Zulip Wasmtime GitHub notifications bot (Sep 25 2020 at 23:57):

cfallin created PR Review Comment:

comment clarification request: somewhat subtle, but let's note that the CLIF instruction spec also specifies this behavior. In theory we might have chosen a simpler behavior at the CLIF level and done the Wasm-required magic in the translation; the swizzle op in CLIF just happens to be aligned with Wasm. (It probably doesn't matter much and is unlikely to change, but precision doesn't hurt...).

view this post on Zulip Wasmtime GitHub notifications bot (Sep 28 2020 at 10:14):

bnjbvr submitted PR Review.

view this post on Zulip Wasmtime GitHub notifications bot (Sep 28 2020 at 11:41):

bnjbvr submitted PR Review.

view this post on Zulip Wasmtime GitHub notifications bot (Sep 28 2020 at 11:41):

bnjbvr submitted PR Review.

view this post on Zulip Wasmtime GitHub notifications bot (Sep 28 2020 at 11:41):

bnjbvr created PR Review Comment:

Even better, for static goodness: use a different Vcode instruction for these two opcodes, so that the error becomes a compile time error.

view this post on Zulip Wasmtime GitHub notifications bot (Sep 28 2020 at 11:41):

bnjbvr created PR Review Comment:

While this may map the intel manual more closely, it's unusual and quite disturbing to see an input named dst0 here, and it could lead to mental mapping errors (e.g. this can be written to without any issue). Can you name this in_vec or something like this?

view this post on Zulip Wasmtime GitHub notifications bot (Sep 28 2020 at 11:41):

bnjbvr created PR Review Comment:

Also, the tradition has to name this is64 to indicate a 64-bits operation or something along those lines.

view this post on Zulip Wasmtime GitHub notifications bot (Sep 28 2020 at 17:06):

abrown submitted PR Review.

view this post on Zulip Wasmtime GitHub notifications bot (Sep 28 2020 at 17:06):

abrown created PR Review Comment:

I went with the assert! here; it's a bit burdensome to create new Inst variants and creating new ones just adds to the chaos of deciding which variant to pick during lowering.

view this post on Zulip Wasmtime GitHub notifications bot (Sep 28 2020 at 17:11):

abrown submitted PR Review.

view this post on Zulip Wasmtime GitHub notifications bot (Sep 28 2020 at 17:11):

abrown created PR Review Comment:

Added additional documentation here. But note that we shouldn't translate the semantics at the Wasm-to-CLIF level because that would disadvantage ARM since the semantics of their instruction matches the Wasm SIMD semantics. (If you look at issues in the Wasm SIMD repository I've brought up several times how the Wasm SIMD semantics are skewed towards matching ARM semantics).

view this post on Zulip Wasmtime GitHub notifications bot (Sep 28 2020 at 19:24):

abrown updated PR #2230 from lane to main:

This change adds x64 implementations for extract_lane, replace_lane, shuffle, and swizzle as well as some saturating addition (necessary for swizzle). I cannot yet enable the simd_lane.wast test because it also uses other instructions like splat, all_true, and any_true but I did verify that the assertions in simd_lane.wast that do not use these instructions pass.

view this post on Zulip Wasmtime GitHub notifications bot (Sep 29 2020 at 15:45):

abrown merged PR #2230.

Last updated: Nov 22 2024 at 17:03 UTC