Last Comment Bug 772892 - IonMonkey: Optimize Math.pow
: IonMonkey: Optimize Math.pow
Status: RESOLVED FIXED
[ion:p1:fx18]
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: All All
: -- normal (vote)
: ---
Assigned To: Sean Stangl [:sstangl]
: general
: Jason Orendorff [:jorendorff]
Mentors:
: 713875 (view as bug list)
Depends on:
Blocks: IonSpeed 768745 777564
  Show dependency treegraph
 
Reported: 2012-07-11 08:51 PDT by Jan de Mooij [:jandem]
Modified: 2012-09-07 07:52 PDT (History)
7 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
x86/x64 implementation. (39.04 KB, patch)
2012-07-30 14:04 PDT, Sean Stangl [:sstangl]
jdemooij: review+
Details | Diff | Splinter Review
Full patch with working ARM. (39.62 KB, patch)
2012-07-31 15:57 PDT, Sean Stangl [:sstangl]
marty.rosenberg: review+
Details | Diff | Splinter Review

Description Jan de Mooij [:jandem] 2012-07-11 08:51:49 PDT
JM+TI inlines Math.pow(x, 0.5) and Math.pow(x, -0.5). These are easy to inline because they compile to sqrt(x) or 1/sqrt(x) (with some guards to handle some edge cases).

We can handle Math.pow(double, double | int) by calling a runtime function, like we already do for Math.log/sin/cos/tan.

Both of these should be a small SS partial-sums win.
Comment 1 Sean Stangl [:sstangl] 2012-07-19 16:17:03 PDT
Also affects kraken's darkroom.
Comment 2 Sean Stangl [:sstangl] 2012-07-30 14:04:22 PDT
Created attachment 647293 [details] [diff] [review]
x86/x64 implementation.

x86/x64 implementation. Kindly disregard the ARM implementation, as I don't currently have the ability to either build or test it. Once I do, I'll r? mjrosenb on the ARM-specific stuff.
Comment 3 Jan de Mooij [:jandem] 2012-07-31 10:13:11 PDT
Comment on attachment 647293 [details] [diff] [review]
x86/x64 implementation.

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

Looks good. r=me on the non-ARM changes, with nits addressed.

Can you also add some tests for the various cases we inline in MCallOptimize (assuming we don't have jit-test coverage for all of them)?

::: js/src/ion/MIR.h
@@ +2291,5 @@
> +    MPow(MDefinition *input, MDefinition *power, MIRType powerType)
> +      : PowPolicy(powerType)
> +    {
> +        initOperand(0, input);
> +        initOperand(1, power);

Nit: if MPow inherits from MBinaryInstruction you can pass these to its constructor.

@@ +2318,5 @@
> +    }
> +};
> +
> +// Inline implementation of Math.pow(x, 0.5), which subtly differs from Math.sqrt(x).
> +class MPowHalf

Nit: you can also override congruentTo (just "return congruentIfOperandsEqual(ins)")

::: js/src/ion/TypePolicy.h
@@ +149,5 @@
>  };
>  
> +// Expect an int or double for operand Op. The input may not be a Value.
> +template <unsigned Op>
> +class NumberPolicy : public TypePolicy

Nit: this policy is not used.

::: js/src/ion/shared/CodeGenerator-x86-shared.cpp
@@ +425,5 @@
> +CodeGeneratorX86Shared::visitPowHalfD(LPowHalfD *ins)
> +{
> +    FloatRegister input = ToFloatRegister(ins->input());
> +    FloatRegister xmm_scratch = ToFloatRegister(ins->tempFloat());
> +    Register scratch = ToRegister(ins->temp());

Can we use ScratchFloatReg here, like LFloor/LRound etc?

@@ +433,5 @@
> +    Label done, sqrt;
> +
> +    // Branch if not -Infinity.
> +    masm.move32(Imm32(negInfinityFloatBits), scratch);
> +    masm.loadFloatAsDouble(scratch, xmm_scratch);

Neat.
Comment 4 Sean Stangl [:sstangl] 2012-07-31 15:57:10 PDT
Created attachment 647731 [details] [diff] [review]
Full patch with working ARM.

Same patch as reviewed by Jan, with working ARM support. It's only necessary to look at CodeGeneratorARM::visitPowHalfD().
Comment 5 Marty Rosenberg [:mjrosenb] 2012-07-31 16:12:34 PDT
Comment on attachment 647731 [details] [diff] [review]
Full patch with working ARM.

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

::: js/src/ion/arm/CodeGenerator-arm.cpp
@@ +703,5 @@
> +    Label done;
> +
> +    // Masm.pow(-Infinity, 0.5) == Infinity.
> +    masm.ma_vimm(js_NegativeInfinity, ScratchFloatReg);
> +    masm.compareDouble(input, ScratchFloatReg);

I'll think if there is some way of doing this without both loading -Infinity and doing a comparison, but this will still be lots faster than what we have now.
Comment 6 Sean Stangl [:sstangl] 2012-07-31 20:38:50 PDT
http://hg.mozilla.org/projects/ionmonkey/rev/a2195dd78532
Comment 7 Sean Stangl [:sstangl] 2012-07-31 22:52:37 PDT
http://hg.mozilla.org/projects/ionmonkey/rev/a73cf1325796

Follow-up hack to work around regalloc bug exposed by this patch. Filed Bug 779411 for that.
Comment 8 Jan de Mooij [:jandem] 2012-09-07 07:52:48 PDT
*** Bug 713875 has been marked as a duplicate of this bug. ***

Note You need to log in before you can comment on or make changes to this bug.