931328 - IonMonkey: Optimize Math.hypot

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[Jason Orendorff [:jorendorff]]

There's magic in MIR.h and MCallOptimize.cpp for all the other math functions, but hypot isn't there. It could be inlined for the common 2-argument case.

Comment 1

[Sankha Narayan Guria [:sankha]]

Attachment: patch v0

This patch doesn't compile as of yet. It still needs |math_hypot_impl| and |math_hypot_uncached| in jsmath.cpp.

Looks like I will need to implement the hypot function such that it returns a double value only, and |math_hypot_impl| and |math_hypot_uncached| will call it then. Then how do I go about returning the Infinite and NaN values from a double only hypot function?

Comment 2

[Sankha Narayan Guria [:sankha]]

Attachment: patch v1

This patch compiles properly.

Comment 3

[Sankha Narayan Guria [:sankha]]

This uses the system's hypot function.

Comment 4

[Jason Orendorff [:jorendorff]]

Comment on attachment 825294 [details] [diff] [review]
patch v1

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

I don't think hypot() exists on Windows (?) so please steal an implementation from fdlibm, put it in jsmath.cpp, and declare it in jsmath.h, under #ifndef HAVE_HYPOT.

Please also make sure that the implementation in jsmath.cpp also uses hypot() when exactly two arguments are given. This way, the result of Math.hypot will not vary depending on what kind of code called it.

When you've done that, please ask :jandem to review the rest. Thanks.

::: js/src/jit/Lowering.cpp
@@ +1214,5 @@
>  
>  bool
> +LIRGenerator::visitHypot(MHypot *ins)
> +{
> +    

Nit: Delete this blank line, please.

Comment 5

[Sankha Narayan Guria [:sankha]]

Attachment: patch v2

This inlines the Math.hypot() call for 2 arguments with the system hypot function. Also I changed the jsmath.cpp implementation to use the system's hypot function in case of two arguments.

Pushed to Try: https://tbpl.mozilla.org/?tree=Try&rev=501318e3293a

Comment 6

[Jan de Mooij [:jandem]]

Comment on attachment 827321 [details] [diff] [review]
patch v2

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

Excellent, thanks! Just some style nits, if you address these I will r+.

::: js/src/jit/Lowering.cpp
@@ +1223,5 @@
>  
>  bool
> +LIRGenerator::visitHypot(MHypot *ins)
> +{
> +    

Nit: delete this blank line

::: js/src/jsmath.cpp
@@ +1278,5 @@
>  {
>      CallArgs args = CallArgsFromVp(argc, vp);
>  
>      bool isInfinite = false;
>      bool isNaN = false;

Nit: you can move these two lines after the |if|

@@ +1287,5 @@
> +            return false;
> +        if (!ToNumber(cx, args[1], &y))
> +            return false;
> +        isInfinite = mozilla::IsInfinite(x) || mozilla::IsInfinite(y);
> +        isNaN = mozilla::IsNaN(x) || mozilla::IsNaN(y);

... and remove these two lines.

@@ +1296,5 @@
> +        result = isInfinite ? PositiveInfinity() :
> +                 isNaN ? GenericNaN() :
> +                 result;
> +        args.rval().setNumber(result);
> +        return true;

And here just do:

double result = hypot(x, y);
args.rval.setNumber(result);
return true;

IonMonkey is going to call "hypot" directly from JIT code so that should work here too without the extra checks :)

@@ +1297,5 @@
> +                 isNaN ? GenericNaN() :
> +                 result;
> +        args.rval().setNumber(result);
> +        return true;
> +    } else {

Style nit: "no else after return", so remove the else branch and unindent the code

Comment 7

[Jan de Mooij [:jandem]]

Jason, should we move tests/ecma_6/Math/hypot-{exact,approx}.js to jit-tests? We will have to port assertNear somehow but other than that it should Just Work I think and will get us --ion-eager testing etc for free.

Comment 8

[Jason Orendorff [:jorendorff]]

Yes, that's fine.

Comment 9

[Sankha Narayan Guria [:sankha]]

Attachment: patch v3

Comment 10

[Jan de Mooij [:jandem]]

Comment on attachment 827566 [details] [diff] [review]
patch v3

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

Looks great, thanks for moving the tests. r=me with nits addressed.

::: js/src/jit-test/tests/basic/hypot-approx.js
@@ +1,1 @@
> +load(libdir + "../../tests/ecma_6/Math/shell.js");

You can use loadRelativeToScript("../../tests/ecma_6/Math/shell.js")

::: js/src/jsmath.cpp
@@ +1277,5 @@
>  js::math_hypot(JSContext *cx, unsigned argc, Value *vp)
>  {
>      CallArgs args = CallArgsFromVp(argc, vp);
>  
> +    if (args.length() == 2) {

Nit: sorry I forgot to mention this earlier, but add a comment here like 

    // IonMonkey calls the system hypot function directly if two arguments are
    // given. Do that here as well to get the same results.

Comment 11

[Sankha Narayan Guria [:sankha]]

Attachment: patch v4

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/727ea6823984

Comment 13

[Ryan VanderMeulen [:RyanVM]]

Backed out for Windows jit-test failures.
https://hg.mozilla.org/integration/mozilla-inbound/rev/dc6beafe17c9

https://tbpl.mozilla.org/php/getParsedLog.php?id=30210318&tree=Mozilla-Inbound

Comment 14

[Jan de Mooij [:jandem]]

It also fails with the JITs disabled, but the test passes on other platforms. hypot-approx.js passes too. Maybe the hypot function on windows returns slightly different numbers?

Sankha, you can download Windows browser and shell builds from TBPL, if you have a Windows machine it should be easy to find out why the test fails :)

Comment 15

[Sankha Narayan Guria [:sankha]]

Attachment: patch v5

Added a wrapper function to the system hypot, and Math.hypot will be inlined with that. Tested on Windows and Linux, both are passing.

Comment 16

[Jan de Mooij [:jandem]]

Comment on attachment 829155 [details] [diff] [review]
patch v5

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

Looks good but I think jorendorff is a better reviewer for this part (see jslibmath.h) :)

::: js/src/jslibmath.h
@@ +51,5 @@
> +    /*
> +     * Workaround MS hypot bug, where hypot(Infinity, NaN or Math.MIN_VALUE)
> +     * is NaN, not Infinity.
> +     */
> +    if (mozilla::IsInfinite(x) || mozilla::IsInfinite(y)) {

Nit: no { }

Comment 17

[Jason Orendorff [:jorendorff]]

Comment on attachment 829155 [details] [diff] [review]
patch v5

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

r+=me, but does js_hypot really need to be inlined anywhere outside jsmath.cpp? If not, please move it to jsmath.cpp. The usual place for compatibility wrappers like this is jsmath.cpp -- again, like atan2. The reason js_fmod is in jslibmath.h is that NumberMod is called directly from the interpreter (JSOP_MOD).

Comment 18

[Sankha Narayan Guria [:sankha]]

Attachment: patch v6

Addressed the above comments. I also took the opportunity to remove few lines from jsmath.h, that should have been removed as a part of bug 896264.

Jason can you see if I am correct?

Comment 19

[Jason Orendorff [:jorendorff]]

Comment on attachment 829200 [details] [diff] [review]
patch v6

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

Looks good. Thanks.

::: js/src/jsmath.h
@@ +302,5 @@
>  
>  extern double 
>  math_atanh_impl(MathCache *cache, double x);
>  
>  extern double 

While you're here, please delete trailing whitespace throughout this file. (In emacs, M-x delete-trailing-whitespace.)

Comment 20

[Sankha Narayan Guria [:sankha]]

Attachment: final patch

Comment 21

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/85be8ab85fbf

Comment 22

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/85be8ab85fbf