Fix wasm saturating conversions on x86 et al.

RESOLVED FIXED in Firefox 64

Status

()

defect
RESOLVED FIXED
7 months ago
7 months ago

People

(Reporter: bbouvier, Assigned: bbouvier)

Tracking

Trunk
mozilla64
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox64 fixed)

Details

Attachments

(2 attachments, 3 obsolete attachments)

Assignee

Description

7 months ago
No description provided.
Assignee

Comment 1

7 months ago
Posted patch x86-fix.patch (obsolete) — Splinter Review
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.
Assignee: nobody → bbouvier
Status: NEW → ASSIGNED
Attachment #9015988 - Flags: review?(sunfish)
Assignee

Comment 2

7 months ago
Posted patch fix.patch (obsolete) — Splinter Review
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.
Attachment #9015988 - Attachment is obsolete: true
Attachment #9015988 - Flags: review?(sunfish)
Attachment #9015992 - Flags: review?(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).
Thanks Benjamin!
Assignee

Comment 5

7 months ago
(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 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.
Posted patch x86-toint64 (obsolete) — Splinter Review
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.
Assignee

Comment 8

7 months ago
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 ;)
Attachment #9016103 - Attachment is obsolete: true
Attachment #9016222 - Flags: review+
Assignee

Comment 9

7 months ago
Remaining changes on top of your commit.
Attachment #9015992 - Attachment is obsolete: true
Attachment #9015992 - Flags: review?(sunfish)
Attachment #9016223 - Flags: review?(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).
Attachment #9016223 - Flags: review?(sunfish) → review+

Comment 11

7 months ago
Pushed by bbouvier@mozilla.com:
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

7 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/d9951c225add
https://hg.mozilla.org/mozilla-central/rev/ee4f12cc7136
Status: ASSIGNED → RESOLVED
Last Resolved: 7 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla64
You need to log in before you can comment on or make changes to this bug.