Closed Bug 728852 Opened 9 years ago Closed 9 years ago

IonMonkey: miscellaneous improvements to the Double->Int32 handling on ARM

Categories

(Core :: JavaScript Engine, defect)

ARM
Linux
defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: mjrosenb, Unassigned)

Details

Attachments

(1 file)

I found two things that were slowing us down on Sunspider:
1) the implementation of round called emitDoubleToInt32, rather than emitTruncateDouble
2) ValueToInt32 bails out if emitTruncateDouble fails

The attached patch should address both of these issues (not in the most elegant manner, but...)
Attachment #598836 - Flags: review?(Jacob.Bramley)
Comment on attachment 598836 [details] [diff] [review]
/home/mrosenberg/patches/optimize_double_to_int-r1.patch

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

There's one actual (but trivial) bug to be fixed, and a set of comments which are mostly tidying and other such things.

::: js/src/ion/arm/CodeGenerator-arm.cpp
@@ +859,5 @@
>      //              +2  +1.5  +1  +0.5  +0  -0.5  -1  -1.5  -2
>      // vcvt:          }-------> }-------><-------{ <-------{
>      // floor:         }-------> }-------> }-------> }------->
>  
> +    //masm.ma_vsub(ScratchFloatReg, ScratchFloatReg, ScratchFloatReg);

This line can go.

@@ +883,5 @@
>      // In case of impossible convertion, INT_MIN is stored in output, which
>      // cause an overflow.
>      if (!bailoutIf(Assembler::Overflow, lir->snapshot()))
>          return false;
> +#endif

Delete the #if 0 block, or make it a run-time assertion (if we can do those).

@@ +925,5 @@
> +    masm.ma_b(fail, Assembler::Equal);
> +}
> +
> +static uint32
> +js_DoubleToECMAInt32NonInline(jsdouble d)

There's an optimized version of this in jsnum.h ("js_DoubleToECMAInt32"). The function is (C++) inline, but you could simply call that function from here to get a non-inline version of it.

The implementation in this function is the same as the fall-back (slow) case from js_DoubleToECMAInt32.

@@ +959,5 @@
>      masm.ma_cmp(dest, Imm32(0x7fffffff));
>      masm.ma_cmp(dest, Imm32(0x80000000), Assembler::NotEqual);
> +    Label join;
> +    masm.ma_b(&join, Assembler::NotEqual);
> +    // oh god, i'm a bad person, this is so the label is referenced :(

Did you do it to avoid a warning? I don't understand why it's needed, since you reference the label at the end anyway. Is this development code?

::: js/src/ion/arm/MacroAssembler-arm.cpp
@@ +967,5 @@
>  
>  void
> +MacroAssemblerARM::ma_vneg(FloatRegister src, FloatRegister dest)
> +{
> +    as_vmov(dest, src);

s/as_vmov/as_vneg/

@@ +1019,5 @@
>  void
>  MacroAssemblerARM::ma_vxfer(FloatRegister src, Register dest)
>  {
>      as_vxfer(dest, InvalidReg, VFPRegister(src).singleOverlay(), FloatToCore);
> +}void

You definitely need some white-space there!

@@ +1585,5 @@
>  }
>  
> +// This functon almost certainly should not be called.
> +// It has two uses in indep code, loading NaN, and loading 0.0
> +// loading 1.0, then subtracting from itself will almost certainly be faster.

Why not make this function do just that for those special-cases?
Attachment #598836 - Flags: review?(Jacob.Bramley) → review+
landed: http://hg.mozilla.org/projects/ionmonkey/rev/2c61c3d67b0f
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.