IonMonkey: specialize ToInt32 for Float32

RESOLVED FIXED in Firefox 26

Status

()

RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: bbouvier, Assigned: bbouvier)

Tracking

unspecified
mozilla27
Points:
---

Firefox Tracking Flags

(firefox26 fixed, firefox27 fixed)

Details

(Whiteboard: [qa-])

Attachments

(3 attachments, 3 obsolete attachments)

(Assignee)

Description

5 years ago
Created attachment 808918 [details] [diff] [review]
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.
Attachment #808918 - Flags: review?(sstangl)
Attachment #808918 - Flags: review?(mrosenberg)
(Assignee)

Updated

5 years ago
Summary: IonMonkey: specialize ToInt32 → IonMonkey: specialize ToInt32 for Float32
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.
Attachment #808918 - Flags: review?(sstangl) → review+
(Assignee)

Comment 2

5 years ago
Created attachment 810784 [details] [diff] [review]
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).
Attachment #808918 - Attachment is obsolete: true
Attachment #808918 - Flags: review?(mrosenberg)
Attachment #810784 - Flags: review?(mrosenberg)
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".
Attachment #810784 - Flags: review?(mrosenberg) → review+
(Assignee)

Comment 4

5 years ago
Created attachment 810890 [details] [diff] [review]
fixed parts of ARM

Carrying forward r+ from sstangl.

https://hg.mozilla.org/integration/mozilla-inbound/rev/3c21fdf48606
Attachment #810784 - Attachment is obsolete: true
Attachment #810890 - Flags: review+
(Assignee)

Comment 5

5 years ago
Created attachment 810896 [details] [diff] [review]
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?
Attachment #810896 - Flags: feedback?(sstangl)
(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);
https://hg.mozilla.org/mozilla-central/rev/3c21fdf48606
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla27
Depends on: 921490
(Assignee)

Comment 8

5 years ago
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
Attachment #810890 - Flags: approval-mozilla-aurora?
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.
Attachment #810896 - Flags: feedback?(sstangl)
Attachment #810890 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
https://hg.mozilla.org/releases/mozilla-aurora/rev/47cb296e2627
status-firefox26: --- → fixed
status-firefox27: --- → fixed
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
status-firefox26: fixed → affected
Keywords: branch-patch-needed
Flags: needinfo?(benj)
(Assignee)

Comment 12

5 years ago
Created attachment 817752 [details] [diff] [review]
bug919838-branch.patch

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

Branch patch as requested.
Carried forward r+ from sstangl.
Attachment #817752 - Flags: review+
Attachment #817752 - Flags: approval-mozilla-aurora?
Flags: needinfo?(benj)
Comment on attachment 817752 [details] [diff] [review]
bug919838-branch.patch

Rebases don't need new approval. Thanks for the patch :)
Attachment #817752 - Flags: approval-mozilla-aurora?
Flags: needinfo?(benj)
(Assignee)

Comment 16

5 years ago
Created attachment 818257 [details] [diff] [review]
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.
Attachment #817752 - Attachment is obsolete: true
Attachment #818257 - Flags: review+
Flags: needinfo?(benj)
https://hg.mozilla.org/releases/mozilla-aurora/rev/9ca74e12ff65
status-firefox26: affected → fixed
Keywords: branch-patch-needed

Updated

5 years ago
Whiteboard: [qa-]
You need to log in before you can comment on or make changes to this bug.