Math library cleanups

RESOLVED FIXED in mozilla27

Status

()

--
enhancement
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: sunfish, Assigned: sunfish)

Tracking

Trunk
mozilla27
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(4 attachments, 1 obsolete attachment)

(Assignee)

Description

5 years ago
Created attachment 797380 [details] [diff] [review]
math-isnan.patch

Looking at math library functions, I noticed a few things that could be tidied up.
Attachment #797380 - Flags: review?(jwalden+bmo)
(Assignee)

Comment 1

5 years ago
Created attachment 797382 [details] [diff] [review]
math-uncache-sqrt-sign.patch
Attachment #797382 - Flags: review?(terrence)
(Assignee)

Comment 2

5 years ago
Created attachment 797383 [details] [diff] [review]
math-no-errno.patch

Darwin libm never uses errno; one of the effects of this patch is to make the compiled code on Linux more consistent with it.
Attachment #797383 - Flags: review?(jwalden+bmo)
Comment on attachment 797382 [details] [diff] [review]
math-uncache-sqrt-sign.patch

I'm not the right reviewer for MathCache. I don't even know off hand who is. Forwarding to Waldo, since he's flagged on the rest of these.
Attachment #797382 - Flags: review?(terrence) → review?(jwalden+bmo)

Comment 4

5 years ago
Comment on attachment 797380 [details] [diff] [review]
math-isnan.patch

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

Always good to have more tests, although I'm not sure I'd go out of my way to tempt fate with the FloatingPoint.h change, myself.  I'm fine with either tests, or tests and code change landing -- just beware of non-obvious breakage in the second case.

::: mfbt/FloatingPoint.h
@@ +64,5 @@
>  {
>    /*
>     * A double is NaN if all exponent bits are 1 and the significand contains at
>     * least one non-zero bit.
>     */

This comment should be removed if you make this change.

@@ +65,5 @@
>    /*
>     * A double is NaN if all exponent bits are 1 and the significand contains at
>     * least one non-zero bit.
>     */
> +  return d != d;

There have been compilers in the past that didn't implement this correctly -- perhaps especially in concert with PGO.  Have we stopped using them?  It's not impossible we have, but I wouldn't bet on it.  If you do land this change (I'd leave well enough alone for now, myself, but I'm not opposed to people double-checking that it's still needed), keep an eye on tinderbox for issues, and then keep an eye on it for the next few days in case it causes any builds to bust with PGO enabled.  You can test PGO-enabled building with this patch:

diff --git a/build/mozconfig.common.override b/build/mozconfig.common.override
--- a/build/mozconfig.common.override
+++ b/build/mozconfig.common.override
@@ -4,8 +4,12 @@
 
 # Common mozconfig for all users
 #
 # Add options to this file that will be inherited by all in-tree mozconfigs.
 # This file is included at the *end* of the mozconfigs, and so may be used
 # to override anything done previously.
 #
 # The common expected usage is for try builds with nondefault options.
+
+# Enable PGO building.
+mk_add_options MOZ_PGO=1
+






I'm told the only lines that matter are the Windows optimized lines and the Ubuntu/Fedora optimized lines.

::: mfbt/tests/TestFloatingPoint.cpp
@@ +107,5 @@
> +{
> +  MOZ_ASSERT(IsNaN(UnspecifiedNaN()));
> +  MOZ_ASSERT(IsNaN(SpecificNaN(1, 17)));;
> +  MOZ_ASSERT(IsNaN(SpecificNaN(0, 0xfffffffffff0fULL)));
> +  MOZ_ASSERT(!IsNaN(0));

Add a !IsNaN(-0) test as well.

@@ +115,5 @@
> +
> +  MOZ_ASSERT(IsInfinite(PositiveInfinity()));
> +  MOZ_ASSERT(IsInfinite(NegativeInfinity()));
> +  MOZ_ASSERT(!IsInfinite(UnspecifiedNaN()));
> +  MOZ_ASSERT(!IsInfinite(0));

And -0 here too.

@@ +121,5 @@
> +
> +  MOZ_ASSERT(!IsFinite(PositiveInfinity()));
> +  MOZ_ASSERT(!IsFinite(NegativeInfinity()));
> +  MOZ_ASSERT(!IsFinite(UnspecifiedNaN()));
> +  MOZ_ASSERT(IsFinite(0));

And here.
Attachment #797380 - Flags: review?(jwalden+bmo) → review+

Comment 5

5 years ago
Comment on attachment 797383 [details] [diff] [review]
math-no-errno.patch

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

Punting, this is build stuff I really don't know much about.
Attachment #797383 - Flags: review?(jwalden+bmo) → review?(mh+mozilla)

Comment 6

5 years ago
Comment on attachment 797382 [details] [diff] [review]
math-uncache-sqrt-sign.patch

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

Seems fine.  I dunno about the implications of not-caching Math.sqrt, but as I'm a bit dubious of the value of Math.*-caching, it's good with me.  ;-)

::: js/src/jsmath.cpp
@@ +877,1 @@
>  {

template <...>
static bool
math_function(...)
{

@@ +1360,5 @@
>      return sign(x);
>  }
>  
>  bool
>  js::math_sign(JSContext *cx, unsigned argc, Value *vp)

Huh, I wonder when Math.sign became a thing...

This seems like something JITs would want to know about at some point, if they don't already, I'd think.
Attachment #797382 - Flags: review?(jwalden+bmo) → review+
Comment on attachment 797383 [details] [diff] [review]
math-no-errno.patch

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

::: js/src/build/autoconf/compiler-opts.m4
@@ +144,5 @@
>          CFLAGS="$CFLAGS -ffunction-sections -fdata-sections"
>          CXXFLAGS="$CXXFLAGS -ffunction-sections -fdata-sections"
>      fi
> +    CFLAGS="$CFLAGS -fno-math-errno"
> +    CXXFLAGS="$CXXFLAGS -fno-exceptions -fno-math-errno"

You don't need changes to android.m4 or configure.in if you add the argument here.

That being said, i don't know if none of our code is using errno for <math.h> functions. One thing that is sure is that we don't use the alternative fetestexcept. OTOH, if darwin never fills errno for those functions...
Attachment #797383 - Flags: review?(mh+mozilla) → review-
(Assignee)

Comment 8

5 years ago
(In reply to Mike Hommey [:glandium] from comment #7)
> Comment on attachment 797383 [details] [diff] [review]
> math-no-errno.patch
> 
> Review of attachment 797383 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: js/src/build/autoconf/compiler-opts.m4
> @@ +144,5 @@
> >          CFLAGS="$CFLAGS -ffunction-sections -fdata-sections"
> >          CXXFLAGS="$CXXFLAGS -ffunction-sections -fdata-sections"
> >      fi
> > +    CFLAGS="$CFLAGS -fno-math-errno"
> > +    CXXFLAGS="$CXXFLAGS -fno-exceptions -fno-math-errno"
> 
> You don't need changes to android.m4 or configure.in if you add the argument
> here.

Ok. What I actually did was just look for the places that add -fno-exceptions, so this may suggest that -fno-exceptions is added redundantly.

> That being said, i don't know if none of our code is using errno for
> <math.h> functions. One thing that is sure is that we don't use the
> alternative fetestexcept. OTOH, if darwin never fills errno for those
> functions...

Yep. And another data point is that there are only two standard math.h errno codes, EDOM and ERANGE, and nothing in mozilla-central is using EDOM, and the things using ERANGE aren't using math.h functions. Another is that no tests fail under -fno-math-errno.

On the dtoa change, I found some additional dtoa-related things to clean up, so I'll split that part out into a separate patch, since it's kind of a separate concern anyway.
(Assignee)

Comment 9

5 years ago
(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #4)
> Comment on attachment 797380 [details] [diff] [review]
> math-isnan.patch
> 
> Review of attachment 797380 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Always good to have more tests, although I'm not sure I'd go out of my way
> to tempt fate with the FloatingPoint.h change, myself.  I'm fine with either
> tests, or tests and code change landing -- just beware of non-obvious
> breakage in the second case.

Ok, I just landed the tests, with the changes you suggested. I did try a PGO build with the FloatingPoint.h change, and the Windows and Ubuntu/Fedora builds did pass, but I decided to stick with your sense of caution for now anyway.

https://hg.mozilla.org/integration/mozilla-inbound/rev/8d55e506ea72
Whiteboard: [leave open]
(Assignee)

Comment 10

5 years ago
Created attachment 798615 [details] [diff] [review]
just -fno-math-errno

This patch just has the -fno-math-errno changes, and just in the compiler-opts.m4 files.
Attachment #797383 - Attachment is obsolete: true
Attachment #798615 - Flags: review?(mh+mozilla)
(Assignee)

Comment 11

5 years ago
Created attachment 798616 [details] [diff] [review]
math-dtoa-overflow.patch

It turns out that js_strtod_harder wasn't actually using JS_DTOA_ERANGE, despite its comment. Fortunately, dtoa.c returns infinity on overflow, so it isn't actually necessary for its callers to check for overflow if they're just going to use infinity in that case. This patch removes the unnecessary code, and eliminates a use of the ambiguous HUGE_VAL macro.

(js_strtod_harder doesn't actually use JS_DTOA_ENOMEM either, but that's a different story.)
Attachment #798616 - Flags: review?(mh+mozilla)
Attachment #798615 - Flags: review?(mh+mozilla) → review+
Comment on attachment 798616 [details] [diff] [review]
math-dtoa-overflow.patch

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

Punting
Attachment #798616 - Flags: review?(mh+mozilla) → review?(jwalden+bmo)

Updated

5 years ago
Attachment #798616 - Flags: review?(jwalden+bmo) → review+
(Assignee)

Comment 16

5 years ago
(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #6)
> Comment on attachment 797382 [details] [diff] [review]
> math-uncache-sqrt-sign.patch
> 
> Review of attachment 797382 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Seems fine.  I dunno about the implications of not-caching Math.sqrt, but as
> I'm a bit dubious of the value of Math.*-caching, it's good with me.  ;-)

I'm dubious of the caching in general too, but I recently saw some data which suggested that cached Math.sign is faster than uncached Math.sign (assuming cachable inputs) on at least one ARM platform, so I'm holding off on this change for now.
(Assignee)

Comment 18

5 years ago
I don't currently plan to commit math-uncache-sqrt-sign.patch, and the other patches are checked in, so I'm closing this bug.
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Whiteboard: [leave open]
Target Milestone: --- → mozilla27
You need to log in before you can comment on or make changes to this bug.