772892 - IonMonkey: Optimize Math.pow

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jan de Mooij [:jandem]]

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]]

Also affects kraken's darkroom.

Comment 2

[Sean Stangl [:sstangl]]

Attachment: 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]]

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]]

Attachment: 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]]

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]]

http://hg.mozilla.org/projects/ionmonkey/rev/a2195dd78532

Comment 7

[Sean Stangl [:sstangl]]

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.