1113445 - SIMD: handle -0 properly in signMask

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Dan Gohman [:sunfish]]

Attachment: signmask-negative-zero.patch

The SIMD.js signMask operation is defined to examine the sign bit of each value. -0 has a non-zero sign bit, but -0 < 0 is false, so a floating-point comparison isn't sufficient. The attached patch implements correct handling on -0, and adds tests.

Comment 1

[Benjamin Bouvier [:bbouvier]]

Comment on attachment 8538913 [details] [diff] [review]
signmask-negative-zero.patch

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

Nice catch, thanks! It will even scale nicely for Float64x2, but probably less nicely for int8x16 / int16x8...

::: js/src/tests/ecma_7/SIMD/float32x4signmask.js
@@ +1,2 @@
> +// |reftest| skip-if(!this.hasOwnProperty("SIMD"))
> +var BUGNUMBER = 946042;

You can remove BUGNUMBER and any reference to it in this file.

@@ +1,4 @@
> +// |reftest| skip-if(!this.hasOwnProperty("SIMD"))
> +var BUGNUMBER = 946042;
> +
> +var float32x4 = SIMD.float32x4;

Can you add tests for int32x4, and rename the file signmask.js, please?

@@ +5,5 @@
> +
> +function test() {
> +  print(BUGNUMBER + ": " + summary);
> +
> +  for ([v, w] of [[float32x4(-1, 20, 30, 4), 0b0001],

Technically, v and w should be declared somewhere, otherwise they'll be considered as globals and thus would get the GETGLOBAL and other deoptimized JSOP. Can you please declare them above the loop?

Comment 2

[Dan Gohman [:sunfish]]

Attachment: signmask-negative-zero.patch

Thanks. Here's a patch which should handle arbitrary SIMD types correctly, and which addresses the other review comments.

Comment 3

[Benjamin Bouvier [:bbouvier]]

Comment on attachment 8539323 [details] [diff] [review]
signmask-negative-zero.patch

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

Wow, even better, thanks!

Waldo, can you review changes made in mfbt/integerTypeTraits.h, please?

::: js/src/builtin/SIMD.cpp
@@ +12,5 @@
>   */
>  
>  #include "builtin/SIMD.h"
>  
> +#include "mozilla/IntegerTypeTraits.h"

Beware the style check: this include should be after js/Value.h, I think. To be tested with make check-style in the build dir, iirc.

@@ +151,5 @@
> +    // Load the data as integer so that we treat the sign bit consistently,
> +    // since -0.0 is not less than zero, but it still has the sign bit set.
> +    typedef typename mozilla::SignedStdintTypeForSize<sizeof(Elem)>::Type Int;
> +    const uint8_t *data = typedObj.typedMem();
> +    int32_t result = 0;

To be sure we don't read values out of limits, can we assert:

MOZ_ASSERT(SimdType::lanes * sizeof(Int) < jit::Simd128DataSize);

@@ +156,5 @@
> +    for (unsigned i = 0; i < SimdType::lanes; ++i) {
> +        Int x;
> +        memcpy(&x, &data[i * sizeof(Int)], sizeof(Int));
> +        result |= (x < 0) << i;
> +    }

nice!

::: mfbt/IntegerTypeTraits.h
@@ +81,5 @@
>  
> +template<size_t Size>
> +struct SignedStdintTypeForSize
> +  : detail::StdintTypeForSizeAndSignedness<Size, true>
> +{};

I am not a peer of mfbt, so might as well ask Waldo if that's fine to add.

Comment 4

[Jeff Walden [:Waldo]]

Comment on attachment 8539323 [details] [diff] [review]
signmask-negative-zero.patch

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

::: js/src/builtin/SIMD.cpp
@@ +150,5 @@
>  
> +    // Load the data as integer so that we treat the sign bit consistently,
> +    // since -0.0 is not less than zero, but it still has the sign bit set.
> +    typedef typename mozilla::SignedStdintTypeForSize<sizeof(Elem)>::Type Int;
> +    const uint8_t *data = typedObj.typedMem();

const Elem *elems = reinterpret_cast<const Elem*>(typedObj.typedMem());

@@ +151,5 @@
> +    // Load the data as integer so that we treat the sign bit consistently,
> +    // since -0.0 is not less than zero, but it still has the sign bit set.
> +    typedef typename mozilla::SignedStdintTypeForSize<sizeof(Elem)>::Type Int;
> +    const uint8_t *data = typedObj.typedMem();
> +    int32_t result = 0;

The suggested assertion can be a static_assert, right?

@@ +154,5 @@
> +    const uint8_t *data = typedObj.typedMem();
> +    int32_t result = 0;
> +    for (unsigned i = 0; i < SimdType::lanes; ++i) {
> +        Int x;
> +        memcpy(&x, &data[i * sizeof(Int)], sizeof(Int));

Int x = mozilla::BitwiseCast<Int>(elems[i]);

Comment 5

[Dan Gohman [:sunfish]]

Comments addressed. Thanks!

https://hg.mozilla.org/integration/mozilla-inbound/rev/25507fee09eb

Comment 6

[Mayank Bansal]

http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=145cfaf3c177&tochange=2f90deb1be15   
appears to have caused a 3.7% regression  on AWFY-Octane-EarlyBoyer on machine 28 http://arewefastyet.com/#machine=28&view=breakdown&suite=octane

Comment 7

[Benjamin Bouvier [:bbouvier]]

(In reply to mayankleoboy1 from comment #6)
> http://hg.mozilla.org/integration/mozilla-inbound/
> pushloghtml?fromchange=145cfaf3c177&tochange=2f90deb1be15   
> appears to have caused a 3.7% regression  on AWFY-Octane-EarlyBoyer on
> machine 28 http://arewefastyet.com/#machine=28&view=breakdown&suite=octane

It's more likely that it's due to bug 1096138, which I don't have access to. Opened bug 1115330 for tracking.

Comment 8

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/25507fee09eb