1246800 - SIMD: Implement and test masked shift-by-scalar amounts

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jakob Stoklund Olesen [:jolesen]]

The SIMD.js functions for shifting a vector by a scalar are now masking the scalar to the size if the vector lanes: https://github.com/tc39/ecmascript_simd/pull/320

Make sure that we implement these semantics, and that we have tests.

Comment 1

[Jakob Stoklund Olesen [:jolesen]]

Attachment: Masked shift-by-scalar amounts.

Modulo-reduce SIMD shift amounts by the size of the lane instead of saturating
shift amounts larger than the number of bits in a lane.

Comment 2

[Jakob Stoklund Olesen [:jolesen]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=346133511ce5

Comment 3

[Dan Gohman [:sunfish]]

Comment on attachment 8721047 [details] [diff] [review]
Masked shift-by-scalar amounts.

Review of attachment 8721047 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good!

::: js/src/builtin/SIMD.cpp
@@ +718,5 @@
>  template<typename T>
>  struct ShiftLeft {
>      static T apply(T v, int32_t bits) {
> +        uint32_t maskedBits = uint32_t(bits) % (sizeof(T) * 8);
> +        return  v << maskedBits;

nit: extra space after `return`.

Also, C++'s left shift on signed types has a bunch of undefined behavior, so this should cast v to MakeUnsigned<T>::Type before doing the shift.

@@ +733,5 @@
>  template<typename T>
>  struct ShiftRightLogical {
>      static T apply(T v, int32_t bits) {
> +        uint32_t maskedBits = uint32_t(bits) % (sizeof(T) * 8);
> +        return uint32_t(v) >> maskedBits;

For consistency, and correctness if this code ever gets used for 64-bit shifts, it'd be good to use MakeUnsigned<T>::Type instead of uint32_t for this shift too.

Comment 4

[Jakob Stoklund Olesen [:jolesen]]

Ugh, there is no ScratchRegisterScope on 32-bit x86. I'll need to insert that masking `and` instruction before register allocation. I guess LIRGenerator::visitSimdShift() can do it.

Comment 5

[Jan de Mooij [:jandem]]

(In reply to Jakob Stoklund Olesen [:jolesen] from comment #4)
> Ugh, there is no ScratchRegisterScope on 32-bit x86. I'll need to insert
> that masking `and` instruction before register allocation. I guess
> LIRGenerator::visitSimdShift() can do it.

Just curious, we can't add a temp register to LSimdShift (and LDefinition::BogusTemp() when we know we don't need it)?

Comment 6

[Jakob Stoklund Olesen [:jolesen]]

Attachment: Masked shift-by-scalar amounts.

Dan, would you mind giving this one a second look?

I used MakeUnsigned<T> as you suggested to avoid undefined behavior in the
left-shifting function.

I added a temp register to LSimdShift as Jan suggested. It seems to work fine,
but please double-check.

Everything else is unchanged.

Comment 7

[Jakob Stoklund Olesen [:jolesen]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=dd3508bcede4

Comment 8

[Jakob Stoklund Olesen [:jolesen]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/cfaec1c77ae38d071284c61294ea70a59a3b7e09
Bug 1246800 - Masked shift-by-scalar amounts. r=sunfish

Comment 9

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/cfaec1c77ae3