IonMonkey: Optimize Math.pow

RESOLVED FIXED

Status

()

Core
JavaScript Engine
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: jandem, Assigned: sstangl)

Tracking

(Blocks: 1 bug)

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [ion:p1:fx18])

Attachments

(2 attachments)

(Reporter)

Description

5 years ago
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.
(Reporter)

Updated

5 years ago
Blocks: 768745
(Assignee)

Comment 1

5 years ago
Also affects kraken's darkroom.
(Assignee)

Updated

5 years ago
Assignee: general → sstangl
Blocks: 777564
Whiteboard: [ion:p1:fx18]
(Assignee)

Comment 2

5 years ago
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.
Attachment #647293 - Flags: review?(jdemooij)
(Reporter)

Comment 3

5 years ago
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.
Attachment #647293 - Flags: review?(jdemooij) → review+
(Assignee)

Comment 4

5 years ago
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().
Attachment #647731 - Flags: review?(mrosenberg)
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.
Attachment #647731 - Flags: review?(mrosenberg) → review+
(Assignee)

Comment 6

5 years ago
http://hg.mozilla.org/projects/ionmonkey/rev/a2195dd78532
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
(Assignee)

Comment 7

5 years ago
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.
(Reporter)

Updated

5 years ago
Duplicate of this bug: 713875
You need to log in before you can comment on or make changes to this bug.