Simd: Shift semantics, what should they be?

[dtig]

In the current state, an i32 parameter specifies the number of bits to shift. I think this is inefficient for right shifts on ARM, as the vshr instruction only accepts immediates. Left shifts on ARM, and shift operations on Intel other than the ones for i8x16 (which will be slow irrespective of the semantics), will not be affected as these have instructions that map to variable shifts. I see a couple of options here -

• Change shift semantics to use immediates, potentially rename to differentiate them from variable shifts?
• No change, and incur the performance penalty for all right shifts on ARM

Is there an instruction on ARM that simplifies this that I've missed? Or other thoughts on the right way to approach this?

[binji]

No change, and incur the performance penalty for all right shifts on ARM

This only is a performance penalty if the compiler can't prove that the shift amount is a constant. It seems like compilers should be able to do this (they likely are already doing this for scalar shifts, for example).

[dtig]

My concern was that when the constant is matched the code-gen is identical to when it is an immediate, but if a local is used that would be a potential performance cliff because AFAIK there isn't an instruction that matches to that exactly, and would need to be emulated. Whereas most of the scalar shifts it wouldn't be very obvious because the the same instruction can be used either with an immediate or a register.
Forcing them to be immediates guarantees consistent performance, though that's probably non-ideal for some use cases.

[penzn]

I think a lot depends on use patterns in real code. Using two kinds of shifts via intrinsics would still handicap ARM when programmer picks a variable shift over a constant shift. And autovectorizers may or may not be able to produce enough constant shifts in place of variable shifts to make a difference.

[arunetm]

We could avoid limiting all use-cases by a semantic change unless the perf impact is significant in this case. Do we know how bad the problem is? The perf concern seems to be limited to a narrow scenario which could potentially go away in future.

[billbudge]

The Neon vshl instruction treats the register holding the shift amount as signed, so it's possible to implement the equivalent of vshr efficiently.

[tlively]

This is the only remaining encoding/semantic mismatch between the toolchain and V8 that I know of, so it would be great to get this resolved one way or another soonish.

[penzn]

Probably the easiest approach is to implement it as @billbudge suggested, which would take an extra register and an extra instruction, this can be revisited if that is not efficient enough.

[binji]

This sounds like similar overhead to what we currently have for scalar shifts, since we require the shift amount to be masked, which is done automatically on some architectures, and not on others.

[dtig]

Asking for clarification on this issue as I've been prototyping these - is there a reason we shouldn't just do vector shifts to map exactly to what the hardware does? What I mean is changing the operation to use two vector registers, each element in the vector is then shifted by a value from the least significant byte of the corresponding element of a second vector. This maps directly to the vector shift instructions on both Arm/Intel hardware.

Having the shifts map to the hardware eliminate an extra splat (on Intel), and builds in more flexibility to the shifts operations. Doing a true vector shift seems like it would be cumbersome if only this shift variant is provided.

[gnzlbg]

@dtig on Intel at least, lane-wise vector shifts can be performed by using an immediate to shift all lanes, using a run-time constant to shift all lanes, and by using values in a different vector (allowing each lane to be shifted by a different value).

In Rust, the primitive is the later (shift by vector of indices), and LLVM recognizes when the vector with the shift indices was generated by using splat on a run-time or compile-time constant and generates the right code.

If the WASM machine code generator fails to recognize that the shift argument comes with a vector splatted with a constant, it won't be able to generate machine code using the shift by immediate instruction. Is this something we expect all WASM machine code generators to do?

If not, then maybe we need three shift instruction variants, e.g., _imm, _scalar, _vec, so that WASM machine code generators can remain "dumb" and the responsibility of choosing the right instruction is delegated to optimizing compilers targeting WASM.

[dtig]

At the spec level, I think specifying just the _scalar version and not _vec as it is now is not great, because to pattern match the _vec one would be the most general case, which covers the _scalar case as well by using splat as you mentioned above.

It would be possible to pattern match for constants and generate the instructions, and that's more of an implementation detail. But that's an orthogonal discussion from not including the _vec version in the spec at all.

[penzn]

@dtig, why not do something like this (from a comment above):

The Neon vshl instruction treats the register holding the shift amount as signed, so it's possible to implement the equivalent of vshr efficiently.

An extra instruction would be needed to negate, but vector shifts are not a very frequently used operation. Would this be too big of an overhead?

[billbudge]

Many of the portable operations require multiple machine instructions, so this would be totally acceptable. Here's V8's implementation for x86-64:

https://cs.chromium.org/chromium/src/v8/src/compiler/backend/x64/code-generator-x64.cc?rcl=36585c1b6b1fd69d093706a6c4ad9775506a7885&l=2203

[dtig]

@dtig, why not do something like this (from a comment above):

The Neon vshl instruction treats the register holding the shift amount as signed, so it's possible to implement the equivalent of vshr efficiently.

An extra instruction would be needed to negate, but vector shifts are not a very frequently used operation. Would this be too big of an overhead?

It's not that this is too big of an overhead, mostly just that the shifts as they are specified not don't match exactly to any hardware, and need extra instructions. Now these can be generated as additional wasm opcodes (i.e. splat+shift) or just taken care of in codegen. Anyway, I gathered from this issue that vector shifts are not very frequently used, so our implementation matches the Spec. I'll close this issue, and if there is a need for Vector shuffles, we can re-evaluate then.