Closed Bug 931328 Opened 6 years ago Closed 6 years ago

IonMonkey: Optimize Math.hypot

Categories

(Core :: JavaScript Engine: JIT, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla28

People

(Reporter: jorendorff, Assigned: sankha)

Details

Attachments

(1 file, 7 obsolete files)

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.
Attached patch patch v0 (obsolete) — Splinter Review
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?
Attachment #825236 - Flags: feedback?(jorendorff)
Attached patch patch v1 (obsolete) — Splinter Review
This patch compiles properly.
Attachment #825236 - Attachment is obsolete: true
Attachment #825236 - Flags: feedback?(jorendorff)
Attachment #825294 - Flags: review?(jorendorff)
This uses the system's hypot function.
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.
Attachment #825294 - Flags: review?(jorendorff)
Assignee: general → sankha93
Attached patch patch v2 (obsolete) — Splinter Review
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
Attachment #825294 - Attachment is obsolete: true
Attachment #827321 - Flags: review?(jdemooij)
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
Attachment #827321 - Flags: review?(jdemooij)
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.
Flags: needinfo?(jorendorff)
Yes, that's fine.
Flags: needinfo?(jorendorff)
Attached patch patch v3 (obsolete) — Splinter Review
Attachment #827321 - Attachment is obsolete: true
Attachment #827566 - Flags: review?(jdemooij)
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.
Attachment #827566 - Flags: review?(jdemooij) → review+
Attached patch patch v4 (obsolete) — Splinter Review
Attachment #827566 - Attachment is obsolete: true
Keywords: checkin-needed
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 :)
Attached patch patch v5 (obsolete) — Splinter Review
Added a wrapper function to the system hypot, and Math.hypot will be inlined with that. Tested on Windows and Linux, both are passing.
Attachment #827984 - Attachment is obsolete: true
Attachment #829155 - Flags: review?(jdemooij)
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 { }
Attachment #829155 - Flags: review?(jdemooij) → review?(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).
Attachment #829155 - Flags: review?(jorendorff) → review+
Attached patch patch v6 (obsolete) — Splinter Review
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?
Attachment #829155 - Attachment is obsolete: true
Attachment #829200 - Flags: review?(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.)
Attachment #829200 - Flags: review?(jorendorff) → review+
Attached patch final patchSplinter Review
Attachment #829200 - Attachment is obsolete: true
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/85be8ab85fbf
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla28
You need to log in before you can comment on or make changes to this bug.