919838 - IonMonkey: specialize ToInt32 for Float32

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: to-int32.patch

ToInt32 needs a Float32 specialization, too.
Tested locally: it gets activated and results look correct (couldn't reproduce the -0 case locally though).

Bonus: ARM implementation. That's the first time I touch the arm/ subfolder for real, so please let me know if I've done anything wrong.

Asking r? to Sean for the general parts and to Marty for the ARM parts.

Comment 1

[Sean Stangl [:sstangl]]

Comment on attachment 808918 [details] [diff] [review]
to-int32.patch

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

Looks good!

::: js/src/jit/shared/MacroAssembler-x86-shared.h
@@ +457,5 @@
> +    void convertFloat32ToInt32(FloatRegister src, Register dest, Label *fail,
> +                              bool negativeZeroCheck = true)
> +    {
> +        cvttss2si(src, dest);
> +        cvtsi2ss(dest, ScratchFloatReg);

Suggest using convertInt32ToFloat32() -- see Bug 916167.

@@ +466,5 @@
> +        // Check for -0
> +        if (negativeZeroCheck) {
> +            Label notZero;
> +            testl(dest, dest);
> +            j(Assembler::NonZero, &notZero);

|testl(dest, dest); j(Assembler::NonZero, &notZero);| is better written as |branchTest32(Assembler::NonZero, dest, dest, &notZero);|.

@@ +473,5 @@
> +                ptest(src, src);
> +                j(Assembler::NonZero, fail);
> +            } else {
> +                // bit 0 = sign of low double
> +                // bit 1 = sign of high double

This should now read "single" instead of "double".

@@ +476,5 @@
> +                // bit 0 = sign of low double
> +                // bit 1 = sign of high double
> +                movmskps(src, dest);
> +                andl(Imm32(1), dest);
> +                j(Assembler::NonZero, fail);

Using |andl| here is strange. Since we know that the upper 3 singles must be zero, only the lower bit is significant. Then, since a bailout occurs if this lower bit is 1 and we want to pass through a zero, it seems that this could just be written as: |branchTest32(Assembler::NonZero, dest, dest, fail)|, saving a dependency.

Comment 2

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: addresses reviewer comments

Also, the andl is still needed as the higher floats could be garbage (the non-zero tests are carried out only on the lowest float).

Comment 3

[Sean Stangl [:sstangl]]

Comment on attachment 810784 [details] [diff] [review]
addresses reviewer comments

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

Stealing review due to priority.

::: js/src/jit/arm/MacroAssembler-arm.cpp
@@ +129,5 @@
> +    // move the value into the dest register.
> +    ma_vxfer(ScratchFloatReg, dest);
> +    ma_vcvt_I32_F32(ScratchFloatReg, ScratchFloatReg);
> +    ma_vcmp_f32(src, ScratchFloatReg);
> +    as_vmrs(pc);

I have no idea what this particular line does. The ARM ISR states that "Rd must not be r15 (pc)", which makes sense, since you probably don't want to jump to a value stored in an fpreg. And shouldn't it be taking two arguments?

http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.dui0204h/Bcfbdihi.html

In any case, it works in the double path, so I'll just assume it's right.

@@ +137,5 @@
> +        ma_cmp(dest, Imm32(0));
> +        // Test and bail for -0.0, when integer result is 0
> +        // Move the top word of the double into the output reg, if it is non-zero,
> +        // then the original value was -0.0
> +        as_vxfer(dest, InvalidReg, src, FloatToCore, Assembler::Equal, 1);

as_vxfer() does not have a case for where the src register isSingle(), which will trigger an assertion. It goes through all this hardship because it wants to only read the top 32 bits of the double.

But floats are already 32 bits. So it looks like we can fix this by changing the last argument from "1" to "0".

Comment 4

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: fixed parts of ARM

Carrying forward r+ from sstangl.

https://hg.mozilla.org/integration/mozilla-inbound/rev/3c21fdf48606

Comment 5

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: Test for negative zero bailout + jsop_in_dense support

Sean, this is the test case I had to make for the negative zero check. Almost all ToInt32 don't need the negative check (because of MToInt32::analyzeEdgeCasesBackward that sets it to false almost all the time), so I had to introduce Float32 support for jsop_in_dense so that I have a test case working.

That being said, it looks like there's actually no need for the IN operator to have this negative check, as in the interpreter, |-0 in array| is correct. So, we might want to modify the NeedNegativeZeroCheck in MIR.cpp so that it doesn't need a negative check for MInArray, but this test case wouldn't bail out anymore :(

Should I land it too?

Comment 6

[Douglas Crosher [:dougc]]

(In reply to Sean Stangl [:sstangl] from comment #3)
> Comment on attachment 810784 [details] [diff] [review]
> addresses reviewer comments
> 
> Review of attachment 810784 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Stealing review due to priority.
> 
> ::: js/src/jit/arm/MacroAssembler-arm.cpp
> @@ +129,5 @@
> > +    // move the value into the dest register.
> > +    ma_vxfer(ScratchFloatReg, dest);
> > +    ma_vcvt_I32_F32(ScratchFloatReg, ScratchFloatReg);
> > +    ma_vcmp_f32(src, ScratchFloatReg);
> > +    as_vmrs(pc);
> 
> I have no idea what this particular line does. The ARM ISR states that "Rd
> must not be r15 (pc)", which makes sense, since you probably don't want to
> jump to a value stored in an fpreg. And shouldn't it be taking two arguments?
> 
> http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.dui0204h/
> Bcfbdihi.html

The FPSCR is transferred to the condition flags when the target register
is 0b1111 (the pc here), so this looks ok.
 
> In any case, it works in the double path, so I'll just assume it's right.
> 
> @@ +137,5 @@
> > +        ma_cmp(dest, Imm32(0));
> > +        // Test and bail for -0.0, when integer result is 0
> > +        // Move the top word of the double into the output reg, if it is non-zero,
> > +        // then the original value was -0.0
> > +        as_vxfer(dest, InvalidReg, src, FloatToCore, Assembler::Equal, 1);
> 
> as_vxfer() does not have a case for where the src register isSingle(), which
> will trigger an assertion. It goes through all this hardship because it
> wants to only read the top 32 bits of the double.
> 
> But floats are already 32 bits. So it looks like we can fix this by changing
> the last argument from "1" to "0".

Changing the last argument from 1 to 0 looks right, and it will then emit
a VMOV from a single float to a core register.  It might also be
necessary to convert the src to a VFPRegister and flag it as a single
with an appropriate code:

        as_vxfer(dest, InvalidReg, VFPRegister(src).singleOverlay(), FloatToCore, Assembler::Equal, 0);

Comment 7

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/3c21fdf48606

Comment 8

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 810890 [details] [diff] [review]
fixed parts of ARM

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 888109
User impact if declined: Crashes / hangs on various websites that use Float32 (Google Maps, Boon,...)
Testing completed (on m-c, etc.): on m-c central, all tests passed
Risk to taking this patch (and alternatives if risky): low
String or IDL/UUID changes made by this patch: N/A

Comment 9

[Sean Stangl [:sstangl]]

Comment on attachment 810896 [details] [diff] [review]
Test for negative zero bailout + jsop_in_dense support

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

It's not necessary to land this test.

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/47cb296e2627

Comment 11

[Ryan VanderMeulen [:RyanVM]]

Backed out for Android bustage. Please attached a branch-specific patch or request approval for any other bugs this patch depends on.
https://hg.mozilla.org/releases/mozilla-aurora/rev/5626e0382661

https://tbpl.mozilla.org/php/getParsedLog.php?id=28802553&tree=Mozilla-Aurora

Comment 12

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: bug919838-branch.patch

[Approval Request Comment]
Same approval request as comment 8.

Branch patch as requested.
Carried forward r+ from sstangl.

Comment 13

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 817752 [details] [diff] [review]
bug919838-branch.patch

Rebases don't need new approval. Thanks for the patch :)

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/d204b52c966d

Comment 15

[Ryan VanderMeulen [:RyanVM]]

Backed out for jit-test failures.
https://hg.mozilla.org/releases/mozilla-aurora/rev/d14d91e9fca4

https://tbpl.mozilla.org/php/getParsedLog.php?id=29193997&tree=Mozilla-Aurora

Comment 16

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: bug919838-branch.patch

Doh! Sorry about that. Didn't take the time to test locally as it sounded inoffensive.

I had to integrate the patch " Better check coherency function " from bug 913282, r+ by sstangl and tested locally. This time, I checked that all tests passed with --tbpl on x64 and --ion on x86.

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/9ca74e12ff65