1946618 - wasm: SimdOp::{F32x4,F64x2}Relaxed{Min,Max} incorrectly marked as commutative

Status: RESOLVED FIXED

(Core :: JavaScript: WebAssembly, defect)

Description

[xiangwei1895]

Steps to reproduce:

My env:
Ubuntu 22.04 TLS
gecko-dev e85232b4b28ecc970240d39203e417d1c320623c

Build args:
../configure --disable-jemalloc --enable-fuzzing --enable-gczeal --enable-debug --enable-optimize --disable-shared-js --enable-address-sanitizer

Execution:
./dist/bin/js poc.js

Actual results:

Inconsistent results before and after jit optimization.
-13 2

Expected results:

-13 -13

Comment 1

[xiangwei1895]

Attachment: poc.js

Comment 2

[Julian Seward [:jseward]]

Attachment: Reduced test case

Comment 3

[Julian Seward [:jseward]]

Baseline:

[Codegen] xorl       %eax, %eax
[Codegen] vmovd      %eax, %xmm0
[Codegen] vbroadcastb%xmm0, %xmm0
[Codegen] xorl       %eax, %eax
[Codegen] vmovd      %eax, %xmm1
[Codegen] vbroadcastb%xmm1, %xmm1
[Codegen] xorl       %eax, %eax
[Codegen] vmovd      %eax, %xmm2
[Codegen] vbroadcastb%xmm2, %xmm2
[Codegen] pcmpeqd    %xmm2, %xmm1
[Codegen] maxps      %xmm1, %xmm0
[Codegen] vmovmskps  %xmm0, %eax

Ion:

[Codegen] pxor       %xmm0, %xmm0
[Codegen] vpcmpeqd   %xmm0, %xmm0, %xmm1
[Codegen] vmaxps     %xmm0, %xmm1, %xmm0
[Codegen] vmovmskps  %xmm0, %eax

Comment 4

[Julian Seward [:jseward]]

I am unsure whether there is a bug here. In both cases we end up doing
f32x4.relaxed_max with one operand being 0x0000'0000 x 4 and the other
being 0xFFFF'FFFF x 4 -- that is, a negative NaN. The relaxed-simd spec
overview [1] says:

Relaxed min and max
[..]
Return the lane-wise minimum or maximum of two values. If either values is
NaN, or the values are -0.0 and +0.0, the return value is
implementation-defined

and in this case our two implementations produce different results.

Looking at the Intel spec for (v)pcmpeqd, the result depends on which of the
operand registers contains the NaN(s).

So we wind up with the resulting reg being either 0x0000'0000 x 4 or
0xFFFF'FFFF x 4, which, after applying i32x4.bitmask (movmskps) becomes
either 0 or 15, as noted in the reduced test case.

[1] https://github.com/WebAssembly/relaxed-simd/blob/main/proposals/relaxed-simd/Overview.md

Comment 5

[Yury Delendik (:yury)]

and in this case our two implementations produce different results.
Looking at the Intel spec for (v)pcmpeqd, the result depends on which of the
operand registers contains the NaN(s).

The result has to match between baseline and ion. We probably need to align "dest" operands in maxFloatXXXRelaxed calls, so they produce the similar results on the same platform (x86 with avx support).

Comment 6

[Julian Seward [:jseward]]

Attachment: Bug 1946618 - wasm: SimdOp::{F32x4,F64x2}Relaxed{Min,Max} incorrectly marked as commutative. r=yury.

When compiling wasm via Ion, SimdOp::{F32x4,F64x2}Relaxed{Min,Max}, binary
Simd128 nodes with operation SimdOp::{F32x4,F64x2}Relaxed{Min,Max} are marked
as commutative. That means the MIR-to-LIR translation can swap the arguments
as it sees fit. Unfortunately, the underlying x86 minps/maxps instructions
are asymmetrical when one of the input arguments has NaNs in its lanes.

That does not lead to a violation of the wasm Relaxed Simd spec, since in the
case of NaNs the result is implementation-defined. However, it does make it
impossible to guarantee that Ion and baseline produce the same results. We can
(and do) fix the argument order in baseline, but Ion can swap them arbitrarily,
leading to differences in behaviour.

The simple and obvious fix is just to mark such nodes in Ion as
non-commutative.

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/61d37082088e
wasm: SimdOp::{F32x4,F64x2}Relaxed{Min,Max} incorrectly marked as commutative.  r=yury.

Comment 8

[agoloman]

https://hg.mozilla.org/mozilla-central/rev/61d37082088e