1497955 - Fix wasm saturating conversions on x86 et al.

Status: RESOLVED FIXED

(Core :: JavaScript: WebAssembly, defect)

Comment 1

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: x86-fix.patch

This code is a bit hard to read and reason about, so it's actually quite easy to make mistakes. The OOL paths have totally different behavior for the non-saturating case (just check the inputs and trap in this case) vs the saturating case (compute the actual result in the unsigned case, fix up the truncated result in the signed case).

I've ran out of time for today, will investigate other platforms tomorrow.

Comment 2

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: fix.patch

Well, it was an incorrect comparison in the callout for ARM and ARM64; with this patch, all the test pass on all platforms, with Luke's patch applied.

Comment 3

[Dan Gohman [:sunfish]]

Comment on attachment 9015992 [details] [diff] [review]
fix.patch

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

I thought this code had been tested on other platforms, but apparently I was wrong :-/.

::: js/src/jit/x86-shared/MacroAssembler-x86-shared.cpp
@@ +982,5 @@
>              // Negative overflow and NaN both are converted to 0, and the only other case
>              // is positive overflow which is converted to UINT64_MAX.
>              Label nonNegative;
>              loadConstantDouble(0.0, ScratchDoubleReg);
> +            branchDouble(Assembler::DoubleGreaterThan, input, ScratchDoubleReg, &nonNegative);

Ah, this is quite subtle. branchDoubleNotInUInt64Range considers -0 an out-of-range case, even though in some sense it is in range.

::: js/src/jit/x86/MacroAssembler-x86.cpp
@@ +1000,5 @@
>      bind(&fail);
> +    if (isSaturating) {
> +        // The OOL path fixes up issues after we've done the conversion.
> +        truncateDoubleToInt64(Address(esp, 0), Address(esp, 0), temp);
> +        load64(Address(esp, 0), output);

Are these two lines needed? The ool handler code appears to read from input and write to output, so we don't need to precompute output for it.

@@ +1002,5 @@
> +        // The OOL path fixes up issues after we've done the conversion.
> +        truncateDoubleToInt64(Address(esp, 0), Address(esp, 0), temp);
> +        load64(Address(esp, 0), output);
> +        freeStack(2 * sizeof(int32_t));
> +        jump(oolEntry);

And if not, these two lines can be factored out with the else body below. (And ditto for the other instances of this code below).

Comment 4

[Luke Wagner [:luke]]

Thanks Benjamin!

Comment 5

[Benjamin Bouvier [:bbouvier] (inactive)]

(In reply to Dan Gohman [:sunfish] from comment #3)
> Comment on attachment 9015992 [details] [diff] [review]
> fix.patch
> 
> Review of attachment 9015992 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I thought this code had been tested on other platforms, but apparently I was
> wrong :-/.

We had tests, but testing only the very edge cases and simple values (I decided to just keep the spec tests, and not duplicate them into our test suite): https://searchfox.org/mozilla-central/source/js/src/jit-test/tests/wasm/conversion.js#243-298

> ::: js/src/jit/x86/MacroAssembler-x86.cpp
> @@ +1000,5 @@
> >      bind(&fail);
> > +    if (isSaturating) {
> > +        // The OOL path fixes up issues after we've done the conversion.
> > +        truncateDoubleToInt64(Address(esp, 0), Address(esp, 0), temp);
> > +        load64(Address(esp, 0), output);
> 
> Are these two lines needed? The ool handler code appears to read from input
> and write to output, so we don't need to precompute output for it.

Yes, they are needed, in a case of a positive overflow we adjust the output value: https://searchfox.org/mozilla-central/source/js/src/jit/x86-shared/MacroAssembler-x86-shared.cpp#1063

Maybe the OOL functions could be split, for clarity. Maybe the OOL path could even contain this second truncate.

Comment 6

[Dan Gohman [:sunfish]]

Comment on attachment 9015992 [details] [diff] [review]
fix.patch

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

::: js/src/jit/x86-shared/MacroAssembler-x86-shared.cpp
@@ +995,5 @@
>              Label notNaN;
>              branchDouble(Assembler::DoubleOrdered, input, input, &notNaN);
>              move64(Imm64(0), output);
>              jump(rejoin);
> +

Please make this same newline change in the *ToI32 versions of these functions.

Comment 7

[Dan Gohman [:sunfish]]

Attachment: x86-toint64

Fwiw, here's a patch that simplifies the x86 *ToInt64 functions. This makes the x86 versions of these routines work essentially the same way their x64 counterparts do, which makes the codebase simpler, since they already share the OOL path. It also reduces the overall code size for both saturating and non-saturating cases, though it does make the non-saturating case do two load+conditional branches rather than one in the fast path.

Comment 8

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: sunfish-fix.patch

Ah, much better, thanks! I've rejiggered the commits locally to apply yours first (since it was undoing changes I've added), and rebased mine onto yours. Consider r=me for your changes ;)

Comment 9

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: 2.bbouvier-fix.patch

Remaining changes on top of your commit.

Comment 10

[Dan Gohman [:sunfish]]

Comment on attachment 9016223 [details] [diff] [review]
2.bbouvier-fix.patch

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

Looks good!

::: js/src/jit/x86-shared/MacroAssembler-x86-shared.cpp
@@ +979,5 @@
>  
>      if (isSaturating) {
>          if (isUnsigned) {
>              // Negative overflow and NaN both are converted to 0, and the only other case
>              // is positive overflow which is converted to UINT64_MAX.

Please line-wrap this comment to be consistent with the F32 version.

@@ +982,5 @@
>              // Negative overflow and NaN both are converted to 0, and the only other case
>              // is positive overflow which is converted to UINT64_MAX.
>              Label nonNegative;
>              loadConstantDouble(0.0, ScratchDoubleReg);
> +            branchDouble(Assembler::DoubleGreaterThan, input, ScratchDoubleReg, &nonNegative);

One last nitpick: the name "nonNegative" for this label corresponded to testing `>= 0`. Now that we're using `> 0` we should call the label "positive" (here and in the F32 version).

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d9951c225add
Simplify code generation for wasm truncations to int64 on x86; r=bbouvier
https://hg.mozilla.org/integration/mozilla-inbound/rev/ee4f12cc7136
Fix wasm saturating conversions codegen; r=sunfish

Comment 12

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/d9951c225add
https://hg.mozilla.org/mozilla-central/rev/ee4f12cc7136