1142668 - SIMD: fix float to int32 conversions in the JIT

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[Benjamin Bouvier [:bbouvier] (inactive)]

If the input isn't in the int32 range, throw.

Hello, new way of throwing from asm.js

Comment 1

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: Ion

Comment 2

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: asm.js

Comment 3

[Dan Gohman [:sunfish]]

Comment on attachment 8580152 [details] [diff] [review]
Ion

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

::: js/src/jit/MIR.h
@@ +1522,5 @@
>  class MSimdConvert
>    : public MUnaryInstruction,
>      public SimdPolicy<0>::Data
>  {
> +    Label *onImpreciseConversion_;

"onImpreciseConversion" is an imprecise name here ;-). Imprecision is ok; what's not ok are overflow or invalid. How about "onConversionError" and a comment somewhere mentioning what an error is?

@@ +1534,5 @@
>          setResultType(toType);
>          specialization_ = fromType; // expects fromType as input
> +
> +        setMovable();
> +        if (fromType == MIRType_Float32x4 && toType == MIRType_Int32x4) {

This is a good moment to add isFloatingPointSIMDType and isIntegerSIMDType helper functions.

::: js/src/jit/shared/CodeGenerator-x86-shared.cpp
@@ +2199,1 @@
>      masm.convertFloat32x4ToInt32x4(in, out);

This conversion is actually slightly incorrect, because values which are slightly greater than INT32_MAX should round to INT32_MAX. It looks like I made the same mistake in the polyfill; I'll fix that.

I think the best thing to do on x86 is just do the convert and test whether any value in the result is 0x80000000, and if so, handle it in out-of-line code. That way it'll only be one movmsk+compare+branch in the common case. The out-of-line code will be a little tricky because -0x80000000 could also be the result of a valid conversion, but the theory is that that won't be a commonly taken code path.

Comment 4

[Nicolas B. Pierron [:nbp]]

Comment on attachment 8580152 [details] [diff] [review]
Ion

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

::: js/src/jit/MIR.h
@@ +1522,5 @@
>  class MSimdConvert
>    : public MUnaryInstruction,
>      public SimdPolicy<0>::Data
>  {
> +    Label *onImpreciseConversion_;

What is a Label doing as part of the MIR ?!

Comment 5

[Dan Gohman [:sunfish]]

(In reply to Nicolas B. Pierron [:nbp] from comment #4)
> > +    Label *onImpreciseConversion_;
> 
> What is a Label doing as part of the MIR ?!

That's a good point. We should also move the Label to the MIRGenerator, as we did with the outOfBoundsLabel (unless someone knows of an even better place).

Comment 6

[Benjamin Bouvier [:bbouvier] (inactive)]

Note to self: there is a CGC timeout failure on try probably due to the |test-also-noasmjs| annotation.

Comment 7

[Benjamin Bouvier [:bbouvier] (inactive)]

(In reply to Dan Gohman [:sunfish] from comment #3)
> ::: js/src/jit/shared/CodeGenerator-x86-shared.cpp
> @@ +2199,1 @@
> >      masm.convertFloat32x4ToInt32x4(in, out);
> 
> This conversion is actually slightly incorrect, because values which are
> slightly greater than INT32_MAX should round to INT32_MAX. It looks like I
> made the same mistake in the polyfill; I'll fix that.

For what it's worth, I remember we discussed this on IRC and the conclusion was that this case (values slightly greater than INT32_MAX and which should round to INT32_MAX) could never happen for Float32x4 inputs (as the next representable float32 is way bigger than INT32_MAX anyways).

Comment 8

[Benjamin Bouvier [:bbouvier] (inactive)]

To test the hypothesis that testSIMD is taking too long in test-also-noasmjs mode:
Try build without test-also-noasmjs: https://treeherder.mozilla.org/#/jobs?repo=try&revision=4cbe5a680c0c
Try build with test-also-noasmjs: https://treeherder.mozilla.org/#/jobs?repo=try&revision=70226176f949

Comment 9

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: simdconvert-ion.patch

Addressed comments from the previous review. The ool path does the input check, so we need to keep the input check alive (and thus not use useAtStart anymore, in case the scratch or output reg was the input reg).

Comment 10

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: simdconvert-asm.patch

fwiw, the new asm.js patch which becomes way simpler, as the onConversionError label becomes part of the mirgen (no needs to pass it along to the AsmJS MIR generator functions, thus we can keep on using the templated variants).

Comment 11

[Dan Gohman [:sunfish]]

Comment on attachment 8591702 [details] [diff] [review]
simdconvert-ion.patch

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

::: js/src/jit-test/tests/SIMD/convert.js
@@ +21,5 @@
>  function f() {
>      var f4 = SIMD.float32x4(1, 2, 3, 4);
>      var i4 = SIMD.int32x4(1, 2, 3, 4);
>      var BitOrZero = (x) => x | 0;
> +    var bigint = Math.pow(2, 31);

Please also add cases for converting from NaN and from some negative finite value outside the range.

::: js/src/jit/Lowering.cpp
@@ +3894,4 @@
>      if (ins->type() == MIRType_Int32x4) {
>          MOZ_ASSERT(input->type() == MIRType_Float32x4);
> +        LFloat32x4ToInt32x4* lir = new(alloc()) LFloat32x4ToInt32x4(use, temp());
> +        if (!mir()->compilingAsmJS())

Instead of testing compilingAsmJS() here, could this test for gen->conversionErrorLabel() == null? It's nice to avoid testing for compilingAsmJS() when more specific tests can work.

::: js/src/jit/x86-shared/CodeGenerator-x86-shared.cpp
@@ +2184,5 @@
> +
> +    masm.loadConstantInt32x4(InvalidResult, ScratchSimdReg);
> +    masm.packedEqualInt32x4(Operand(out), ScratchSimdReg);
> +    masm.vmovmskps(ScratchSimdReg, temp);
> +    masm.cmp32(temp, Imm32(0));

With SSE4.1 we could do this with vptest instead of vmovmskps+cmp and I think it'd be faster. Would you mind adding a comment about this here?

@@ +2188,5 @@
> +    masm.cmp32(temp, Imm32(0));
> +    masm.j(Assembler::NotEqual, &moreChecks);
> +
> +    Label join;
> +    masm.jump(&join);

The following code block is likely to be cold enough that we should do an OutOfLineCode for it, instead of putting it inline here and jumping over it.

@@ +2211,5 @@
> +        masm.vmovmskps(ScratchSimdReg, temp);
> +        masm.cmp32(temp, Imm32(0));
> +        masm.j(Assembler::NotEqual, onConversionError);
> +
> +        if (bail.used()) {

Shouldn't there be a branch at this point to branch to join, so that we don't bail out if the extra checks pass?

::: js/src/jit/x86-shared/MacroAssembler-x86-shared.h
@@ -937,5 @@
> -        // TODO: Note that if the conversion failed (because the converted
> -        // result is larger than the maximum signed int32, or less than the
> -        // least signed int32, or NaN), this will return the undefined integer
> -        // value (0x8000000). Spec should define what to do in such cases. See
> -        // also bug 1068020.

I'd like to leave the first sentence of this comment in place, minus the TODO, since this aspect of x86 is non-obvious.

Comment 12

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: simdconvert-ion.patch

(In reply to Dan Gohman [:sunfish] from comment #11)

Thanks for the review!

> Comment on attachment 8591702 [details] [diff] [review]
> simdconvert-ion.patch
> 
> Review of attachment 8591702 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: js/src/jit-test/tests/SIMD/convert.js
> @@ +21,5 @@
> >  function f() {
> >      var f4 = SIMD.float32x4(1, 2, 3, 4);
> >      var i4 = SIMD.int32x4(1, 2, 3, 4);
> >      var BitOrZero = (x) => x | 0;
> > +    var bigint = Math.pow(2, 31);
> 
> Please also add cases for converting from NaN and from some negative finite
> value outside the range.
Done. I've also added a test case for manually checking that we don't bail out in the case where 0x80000000 is the correct result (with IONFLAGS=bailouts).

> 
> ::: js/src/jit/x86-shared/CodeGenerator-x86-shared.cpp
> @@ +2184,5 @@
> > +
> > +    masm.loadConstantInt32x4(InvalidResult, ScratchSimdReg);
> > +    masm.packedEqualInt32x4(Operand(out), ScratchSimdReg);
> > +    masm.vmovmskps(ScratchSimdReg, temp);
> > +    masm.cmp32(temp, Imm32(0));
> 
> With SSE4.1 we could do this with vptest instead of vmovmskps+cmp and I
> think it'd be faster. Would you mind adding a comment about this here?

Done and opened a follow up, referenced in the comment.

> 
> @@ +2188,5 @@
> > +    masm.cmp32(temp, Imm32(0));
> > +    masm.j(Assembler::NotEqual, &moreChecks);
> > +
> > +    Label join;
> > +    masm.jump(&join);
> 
> The following code block is likely to be cold enough that we should do an
> OutOfLineCode for it, instead of putting it inline here and jumping over it.

Done.

> 
> @@ +2211,5 @@
> > +        masm.vmovmskps(ScratchSimdReg, temp);
> > +        masm.cmp32(temp, Imm32(0));
> > +        masm.j(Assembler::NotEqual, onConversionError);
> > +
> > +        if (bail.used()) {
> 
> Shouldn't there be a branch at this point to branch to join, so that we
> don't bail out if the extra checks pass?

Good catch! As stated below, I've added a test case for this in the test.

Other comments addressed.

Comment 13

[Benjamin Bouvier [:bbouvier] (inactive)]

(try push to confirm that asm.js/testSIMD.js might timeout)

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

Comment 14

[Benjamin Bouvier [:bbouvier] (inactive)]

(same as comment 13, with the interpreter patches rebased to avoid jsreftests crashes everywhere)

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

Comment 15

[Dan Gohman [:sunfish]]

Comment on attachment 8594712 [details] [diff] [review]
simdconvert-ion.patch

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

Looks good!

::: js/src/jit/x86-shared/CodeGenerator-x86-shared.h
@@ +68,5 @@
>              codegen->visitOffsetBoundsCheck(this);
>          }
>      };
>  
> +    // Additional bounds check for vectorial Float to Int conversion, when the

"vectorial" makes me wonder if it means something special. I think you can just say "vector" here.

Comment 16

[Benjamin Bouvier [:bbouvier] (inactive)]

Thanks! One last try run to make sure the cgc timeout is caused by the --noasmjs test on testSIMD, which can be split in the future...

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

Comment 17

[Benjamin Bouvier [:bbouvier] (inactive)]

Try looks green, without the --no-asmjs annotation in testSIMD.js (which needs to get split, for not timeouting in cgc).

Comment 18

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/232963129589
https://hg.mozilla.org/integration/mozilla-inbound/rev/53e0f191ddee

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/232963129589
https://hg.mozilla.org/mozilla-central/rev/53e0f191ddee