Last Comment Bug 714260 - Provide double bit-twiddling macros in MFBT
: Provide double bit-twiddling macros in MFBT
Status: RESOLVED FIXED
: dev-doc-complete
Product: Core
Classification: Components
Component: MFBT (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla14
Assigned To: Jeff Walden [:Waldo] (remove +bmo to email)
:
Mentors:
: 720096 (view as bug list)
Depends on: 746273
Blocks: 745089 692614 788108
  Show dependency treegraph
 
Reported: 2011-12-30 04:45 PST by :Ms2ger
Modified: 2012-09-04 01:33 PDT (History)
6 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Partial patch (6.18 KB, patch)
2012-01-23 03:50 PST, Jeff Walden [:Waldo] (remove +bmo to email)
no flags Details | Diff | Splinter Review
Patch (63.86 KB, patch)
2012-04-11 16:03 PDT, Jeff Walden [:Waldo] (remove +bmo to email)
Ms2ger: review+
dmandelin: review+
mh+mozilla: review-
Details | Diff | Splinter Review
Patch, v2 (68.19 KB, patch)
2012-04-12 12:03 PDT, Jeff Walden [:Waldo] (remove +bmo to email)
Ms2ger: review+
Details | Diff | Splinter Review

Description :Ms2ger 2011-12-30 04:45:37 PST
JSDOUBLE_IS_NaN / DOUBLE_IS_NaN and friends, in particular.
Comment 1 :Ms2ger 2012-01-21 00:51:26 PST
*** Bug 720096 has been marked as a duplicate of this bug. ***
Comment 2 Jeff Walden [:Waldo] (remove +bmo to email) 2012-01-23 03:50:05 PST
Created attachment 590671 [details] [diff] [review]
Partial patch

Putting off sleep, I decided to churn something out here.  This adds a header with inline methods for all the various double predicates (float predicates still need to be added), but it doesn't hook up any users yet.  That's a clear prerequisite to committing any sorts of tempting-looking implementations of such low-level queries.

This also needs DOUBLE_COMPARE at least added in.  But that macro currently has two versions, one for Windows and one for everything else, and as a result it requires specifying a value redundant on every platform but Windows.  I'd like to find out what still requires the Windows-specific version, because the extra argument used only on Windows seems like a great way to get something wrong in the future.  (I can't find a comment explaining why the macro bifurcates as it does, unfortunately.)  If nothing requires it, perhaps we could remove this macro entirely.
Comment 3 :Ms2ger 2012-01-23 04:35:21 PST
There's no way I'm going to remember the name of this file next time I need it ;)
Comment 4 Jeff Walden [:Waldo] (remove +bmo to email) 2012-01-23 09:57:12 PST
Fair.  Suggest some other name, then (keeping in mind it'd have both double-predicates and float-predicates when complete).
Comment 5 :Ms2ger 2012-01-23 10:22:00 PST
Fair too :). Something about reals/floats/StdNonInt/? I'm bad at naming things.
Comment 6 Jeff Walden [:Waldo] (remove +bmo to email) 2012-02-27 17:22:20 PST
I should probably take this so someone doesn't spend time updating my patch for me, as I expect I'll get this done in my spare time sometime soonish.
Comment 7 Jeff Walden [:Waldo] (remove +bmo to email) 2012-04-11 16:03:28 PDT
Created attachment 614208 [details] [diff] [review]
Patch

Ms2ger vaguely poked me about this today, so while I was waiting for jorendorff to show up to ask him a question, I polished off the bits of this I hadn't noodled into shape earlier.  dmandelin, feel free to just look at the js/src changes, I guess.

This doesn't remove all instances of jsdpun from js/src; as I recall (it's been awhile since I touched this other than to rebase it) there are uses for js_DoubleToECMAInt32.  But it's most of them, which is a good start.

I remember having posted previous iterations of this to try and getting back successful results.  I haven't done that with this yet, but I'll make sure to do so before pushing, should this pass reviews.
Comment 8 Jeff Walden [:Waldo] (remove +bmo to email) 2012-04-11 16:05:07 PDT
The NaN bit-pattern bits are a bit funky, to be sure.  Suggestions on that API appreciated.
Comment 9 :Ms2ger 2012-04-12 02:38:34 PDT
Comment on attachment 614208 [details] [diff] [review]
Patch

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

Looks good to me, but I'd like glandium to have a look at the static initializers.

I haven't carefully reviewed the implementations in FloatingPoint.h; I assume you can copy+paste.

::: content/xslt/src/base/txDouble.cpp
@@ +53,5 @@
>  
>  //-- Initialize Double related constants
> +const double txDouble::NaN = MOZ_DOUBLE_NaN();
> +const double txDouble::POSITIVE_INFINITY = MOZ_DOUBLE_POSITIVE_INFINITY();
> +const double txDouble::NEGATIVE_INFINITY = MOZ_DOUBLE_NEGATIVE_INFINITY();

Will this cause static initializers to run? See bug 569629.

Actually, we could probably call the MOZ_* functions where we currently use the txDouble::* constants.

::: mfbt/FloatingPoint.h
@@ +57,5 @@
> +MOZ_STATIC_ASSERT((MOZ_DOUBLE_EXPONENT_BITS & MOZ_DOUBLE_SIGNIFICAND_BITS) == 0,
> +                  "exponent bits don't overlap significand bits");
> +
> +MOZ_STATIC_ASSERT((MOZ_DOUBLE_SIGN_BIT | MOZ_DOUBLE_EXPONENT_BITS | MOZ_DOUBLE_SIGNIFICAND_BITS)
> +                  == (uint64_t)(-1),

UINT64_MAX?

@@ +117,5 @@
> +}
> +
> +/**
> + * Determines whether a double is negative.  It is an error to call this method
> + * on a double which is NaN.

I assume you checked this for the callers you added?

@@ +237,5 @@
> +{
> +  /*
> +   * XXX Casting a double that doesn't truncate to int32_t, to int32_t, induces
> +   *     undefined behavior.  We should definitely fix this, but as apparently
> +   *     it "works" in practice, it's not a pressing concern now.

Could you file a bug and mention it here?
Comment 10 Mike Hommey [:glandium] 2012-04-12 05:58:39 PDT
Comment on attachment 614208 [details] [diff] [review]
Patch

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

::: content/xslt/src/base/txDouble.cpp
@@ +53,5 @@
>  
>  //-- Initialize Double related constants
> +const double txDouble::NaN = MOZ_DOUBLE_NaN();
> +const double txDouble::POSITIVE_INFINITY = MOZ_DOUBLE_POSITIVE_INFINITY();
> +const double txDouble::NEGATIVE_INFINITY = MOZ_DOUBLE_NEGATIVE_INFINITY();

Most likely, it will add static initializers. I do agree with Ms2ger, there is no need to keep these consts, now.
Comment 11 Jeff Walden [:Waldo] (remove +bmo to email) 2012-04-12 12:03:28 PDT
Created attachment 614480 [details] [diff] [review]
Patch, v2

This hasn't fixed the double-int32_t casting comment yet to mention a bug.  I'll make sure to file one and update the comment before pushing this.
Comment 12 :Ms2ger 2012-04-12 12:31:52 PDT
Comment on attachment 614480 [details] [diff] [review]
Patch, v2

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

Ship it
Comment 13 Jeff Walden [:Waldo] (remove +bmo to email) 2012-04-12 22:27:30 PDT
After a bit of tryservering (including of PGO builds, fun fun fun), this appears good.

https://hg.mozilla.org/integration/mozilla-inbound/rev/2187cab0d2f6

I'll get a blog post about this out sometime in the next week or so, also update MDN as needed for it.
Comment 14 Marco Bonardo [::mak] 2012-04-14 06:20:22 PDT
https://hg.mozilla.org/mozilla-central/rev/2187cab0d2f6
Comment 15 Jeff Walden [:Waldo] (remove +bmo to email) 2012-04-19 19:09:32 PDT
Blog post:

http://whereswalden.com/2012/04/19/introducing-mozillafloatingpoint-h-methods-for-floating-point-types-and-values/

I also updated https://developer.mozilla.org/En/mfbt to minimally point to the header.  With those, I guess we're done on the documentation front.
Comment 16 Jeff Walden [:Waldo] (remove +bmo to email) 2012-06-05 10:02:55 PDT
Turns out MOZ_HASH_DOUBLE, modeled on JSDOUBLE_HASH or whatever, became unused sometime between when I first wrote this patch and when I landed it.  Also there were some extraneous #includes, for the same reason.  After a double-check on try I removed these bits here:

https://hg.mozilla.org/integration/mozilla-inbound/rev/4f44ffa3f9ff
Comment 17 Ed Morley [:emorley] 2012-06-06 08:45:20 PDT
https://hg.mozilla.org/mozilla-central/rev/4f44ffa3f9ff

Note You need to log in before you can comment on or make changes to this bug.