939843 - Unify FloatingPoint.h's Float32 / Double operations

Status: RESOLVED FIXED

(Core :: MFBT, defect)

Description

[Benjamin Bouvier [:bbouvier] (inactive)]

See also bug 930477 comment 17. The idea is to unify Float32 and Double operations by using traits and generic operations.

This bug will make this change and also update all uses of FloatingPoint.h.

Waldo, do you think all operations can be made generic, as you suggested in the mentioned comment?

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #17)
> I want tests for this method.  Including for specific interesting values like -1, -0.5, -0.25, -FLT_MIN, -0, +0, +FLT_MIN, +0.25, +0.5, (1 << 22), (1 << 23) - 1, (1 << 23) -0.5, (1 << 23), (1 << 23) + 0.5 (mostly for sanity), (1 << 23) + 1.0, (1 << 23) + 1.5, FLT_MAX, -FLT_MAX, the number just smaller than FLT_MAX, Infinity, -Infinity, and for a sampling of NaN bit patterns.

Also, do you want some C++ tests instead of JS tests, to make sure that all cases are effectively tested?

That being done, it could help for bug 745089.

Comment 1

[Jeff Walden [:Waldo]]

C++ tests would be nice.  I would be inclined to copy-paste the double tests to do float tests, because it seems a little dangerous having *too* much genericity in both tests and implementation.  :-)

Comment 2

[Nathan Froyd [:froydnj]]

+1 for doing this!

Comment 3

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: 1 - Base - WIP

A first taste of what it would look like. A few comments:
- There were methods called DoubleEqualsInt32 / MinDoubleValue, etc. To be more generic, I called them NumberEqualsInt32 / MinNumberValue, etc. I was hesitating with using "FloatingPoint" instead of "Double" (but that's kinda long), or "FP" (shorter, but less explicit). What do you think would be a good name?
- Some interfaces changed for functions that return a floating point value: as C++11 is not capable of inferring returned template values (should be available in C++14 though, also see [1]), instantiations have to precise the template type in the call. For instance, we have to use PositiveInfinity<double>(), NegativeInfinity<double>(), etc. Do you think it could be worth it to create shortcuts that don't need the type, e.g. DoublePositiveInfinity?

[1] https://twitter.com/kropp/status/402365010498043904

Comment 4

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: 2 - Needed modifications in Spidermonkey - WIP

With this patch applied, the JS shell compiles on x64 and x86 (and jit tests pass at least for x64). I'll take care of the rest of the browser once FloatingPoint.h interfaces are stable.

This gives a good preview of how the user code would look like. Intentionally not asking for review or feedback as this also waits for FloatingPoint's interfaces to be stable.

Comment 5

[Benjamin Bouvier [:bbouvier] (inactive)]

Needless to say that I also plan on adding mfbt/tests for floats. I tried to look for some information on the mozilla wiki but didn't find anything pertinent. Is there a way to compile the tests in a standalone way or do they need to be compiled with the whole browser? How can I run them?

Comment 6

[Jeff Walden [:Waldo]]

Comment on attachment 8335196 [details] [diff] [review]
1 - Base - WIP

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

::: mfbt/FloatingPoint.h
@@ +225,1 @@
>                                   significand);

This bit needs realigning the way it was before.

@@ +287,5 @@
>     * since it's cheap to materialize on common platforms (such as x64, where
>     * this value can be represented in a 32-bit signed immediate field, allowing
>     * it to be stored to memory in a single instruction).
>     */
> +  return SpecificNaN<double>(1, 0xfffffffffffffULL);

I'd just have one method for both, and pass in Traits::SignificandBits as the second argument.  (By definition it's all ones, so it fits the comment just above perfectly.)

Comment 7

[Jeff Walden [:Waldo]]

(In reply to Benjamin Bouvier [:bbouvier] from comment #5)
> Needless to say that I also plan on adding mfbt/tests for floats. I tried to
> look for some information on the mozilla wiki but didn't find anything
> pertinent. Is there a way to compile the tests in a standalone way or do
> they need to be compiled with the whole browser? How can I run them?

You have to compile a debug browser to get them...sort of.  mfbt is built so early in the build process, tho, that you don't really need to wait around that long (maybe a few minutes or so) for them to finish.  Once TestFloatingPoint is built -- it gets built as a standalone executable during mfbt's building, which happens either nearly first in the build process, or just after NSPR and maybe NSS -- you can just run it and see whether it works.  The executable will be in $objdir/dist/bin/TestFloatingPoint.

I don't *think* you need it, but just in case: you may need to prefix the execution command with $objdir/dist/bin/run-mozilla.sh to set up library paths just so to run it.

It would be nice for the tests to be built as part of the JS shell build process, too.  That requires library-ification work on mfbt that nobody with the knowledge to do it has been quite exercised enough to do.

(In reply to Benjamin Bouvier [:bbouvier] from comment #3)
> A first taste of what it would look like. A few comments:
> - There were methods called DoubleEqualsInt32 / MinDoubleValue, etc. To be
> more generic, I called them NumberEqualsInt32 / MinNumberValue, etc. I was
> hesitating with using "FloatingPoint" instead of "Double" (but that's kinda
> long), or "FP" (shorter, but less explicit). What do you think would be a
> good name?

NumberBlah seems fine enough; FP isn't worth it, FloatingPoint is a bit much.  (Worth noting the standard way to do this, and most of the methods here, would be std::numeric_limits<double>::min_value or similar, which is way more typing and all, but which we'd switch to if it weren't broken with compilers we care about.)

> For instance, we have to use PositiveInfinity<double>(),
> NegativeInfinity<double>(), etc. Do you think it could be worth it to
> create shortcuts that don't need the type, e.g. DoublePositiveInfinity?

I'd say no.  You're only saving two characters; it's not worth the trouble.

Comment 8

[Benjamin Bouvier [:bbouvier] (inactive)]

As it might be needed by bug 948984, I will work on that as soon as I get the opportunity, which won't be before next week, if not the following week.

Comment 9

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: 1 - Base - WIP

This patch allows to compile TestFloatingPoint.cpp correctly (not the rest of the Spidermonkey engine yet) and gives a nice outlook of what the user code will look like.

The only new things I've added are helpers to write NumberExponentBias<double>() instead of mozilla::detail::DoubleTypeTraits::ExponentBias (the "detail" namespace sounds like it is a blackbox). I dislike the name a bit, but couldn't find any better that's still easy to understand (FPExponentBias? FloatingPointExponentBias? TraitExponentBias?). I'll change it happily if you have something better in mind.

I'd love feedback on this patch, if not a review, so that I can move on rewriting all the uses of this set of functions (and maybe that will help bug 948984). Thanks!

Comment 10

[Jeff Walden [:Waldo]]

Comment on attachment 8366690 [details] [diff] [review]
1 - Base - WIP

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

::: mfbt/FloatingPoint.h
@@ +65,3 @@
>  
> +template<typename T>
> +struct FPTraits : public SelectTrait<T>

What do you think about un-detailing this and naming it FloatingPoint?  Then you'd just have mozilla::FloatingPoint<double>::SignBit which doesn't require inline methods, which then you have to worry about inlining, constexpr, etc.  (If we do that we *will* need a /** ... */ documentation comment by it to describe what's included in it, and probably to clarify it only works at present with float/double.)

@@ +81,5 @@
> +                  "all bits accounted for");
> +
> +    /*
> +     * These implementations assume float/double are 32/64-bit single/double format
> +     * number types compatible with the IEEE-754 standard.  C/C++ don't require this

Let's change to "C++ doesn't" since this header's non-C now.

@@ +114,5 @@
> +}
> +
> +template<typename T>
> +static MOZ_ALWAYS_INLINE const typename detail::FPTraits<T>::Bits
> +

Stray blank line.

@@ +287,3 @@
>  {
>    /*
>     * XXX Casting a double that doesn't truncate to int32_t, to int32_t, induces

s/double/floating-point value/

@@ +294,4 @@
>  }
>  
>  /**
>   * If d can be converted to int32_t and back to an identical double value,

s/d/t/ and s/double/floating-point/ in the right places here.

@@ +296,5 @@
>  /**
>   * If d can be converted to int32_t and back to an identical double value,
>   * set *i to that value and return true; otherwise return false.
>   *
>   * The difference between this and DoubleEqualsInt32 is that this method returns

s/Double/Number/

Comment 11

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: unify-fp-base.patch

The FloatingPoint struct is a nice idea. I've added a few comments for the members of this struct: I think this is the minimum that one would need to use them, as long as they're aware of the IEEE754 spec. What do you think / what would you add?

Comment 12

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: Float32 Tests

Added FloatingPoint tests for the floats too, based on what the doubles do. I was hesitating to make the tests templatized too and then I stopped being a C++ mental.

The only things worth saying about these tests is that INT32_MAX can't be expressed as a float32 without a loss of precision, so instead we use a big int32 constant that can be converted with no loss. INT32_MIN doesn't suffer from that issue. Also, a few constants (1e300f overflows the float32 range a few times and the compiler complains about it) and significands for NaN values change.

I haven't run them yet (for that, I need them compiled, but for that I need at least a compiled JS shell, but for that I need to search and replace for all the calls to these mfbt functions, but for that I need the first patch with a r+ :)), but I would like you to check the sanity of these tests.

Comment 13

[Jeff Walden [:Waldo]]

Comment on attachment 8367924 [details] [diff] [review]
unify-fp-base.patch

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

::: mfbt/FloatingPoint.h
@@ +38,1 @@
>   */

No need for this comment, I think, should be basically evident from its uses.

@@ +77,5 @@
> + *
> + *  SignBit contains a bits mask. Bit-and-ing with this mask will result in
> + *  obtaining the sign bit.
> + *  ExponentBits contains the mask needed for obtaining the exponent bits and
> + *  SignificandBits contains the mask needed for obtaining the significand bits.

The nested typedef |Bits| is the unsigned integral type with the same size as T: uint32_t for float and uint64_t for double (static assertions double-check these assumptions).

Also maybe add a paragraph like so:

Full details of how floating point number formats are encoded are beyond the scope of this comment.  For more information, see

http://en.wikipedia.org/wiki/IEEE_floating_point
http://en.wikipedia.org/wiki/Floating_point#IEEE_754:_floating_point_in_modern_computers

...or something along those lines.  The point is, we're not really attempting to explain these concepts here -- if you're looking, you pretty much should already know what you need, you just need to figure out which terminology picks it.

@@ +257,4 @@
>   * otherwise return false.
>   *
>   * Note that negative zero is "equal" to zero here. To test whether a value can
> + * be losslessly converted to int32_t and back, use IsInt32 instead.

NumberIsInt32

Comment 14

[Jeff Walden [:Waldo]]

Comment on attachment 8367987 [details] [diff] [review]
Float32 Tests

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

::: mfbt/tests/TestFloatingPoint.cpp
@@ +202,5 @@
> +  MOZ_ASSERT(ExponentComponent(2.0f) == 1);
> +  MOZ_ASSERT(ExponentComponent(7.0f) == 2);
> +  MOZ_ASSERT(ExponentComponent(PositiveInfinity<float>()) == FloatingPoint<float>::ExponentBias + 1);
> +  MOZ_ASSERT(ExponentComponent(NegativeInfinity<float>()) == FloatingPoint<float>::ExponentBias + 1);
> +  MOZ_ASSERT(ExponentComponent(UnspecifiedNaN<float>()) == FloatingPoint<float>::ExponentBias + 1);

Hmm, could you split these into two methods Test{Float,Double}ExponentComponent and have TestExponentComponent call them both?

@@ +346,5 @@
>  int
>  main()
>  {
>    TestDoublesAreIdentical();
> +  TestFloatsAreIdentical();

As mentioned, minor nitpick to have this method as

  TestAreIdentical();
  TestExponentComponent();
  TestPredicates();

with each method then calling the double-testing method, and the float-testing method.

Comment 15

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: 1 - Base

Thanks for your comments! Seems like a good day to ask for a review.

Comment 16

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: 2 - Tests

Comment 17

[Jeff Walden [:Waldo]]

Comment on attachment 8370082 [details] [diff] [review]
1 - Base

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

::: mfbt/tests/TestFloatingPoint.cpp
@@ +9,5 @@
>  
> +using mozilla::FloatingPoint;
> +using mozilla::NumbersAreIdentical;
> +using mozilla::NumberEqualsInt32;
> +using mozilla::NumberIsInt32;

Alphabetize these with the rest here.

Comment 18

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: 1 - FloatingPoint.h

That was a terrible rebase.

Comment 19

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: 2 - tests/TestFloatingPoint

Carrying forward r+ from Waldo.

I have locally all needed modifications in the tree, need try run and will ask for r? then.

Comment 20

[Benjamin Bouvier [:bbouvier] (inactive)]

Try looks green, except for a few unrelated intermittent failures: https://tbpl.mozilla.org/?tree=Try&rev=444b6d640ed5
Let's go.

Comment 21

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: 3 - Required changes in Spidermonkey

Comment 22

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: 4 - Required changes in the tree

Mostly search and replace, as I said :)

Comment 23

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 8374739 [details] [diff] [review]
4 - Required changes in the tree

Attemptively asking froydnj for review, as Waldo is in PTO till early March and this kind of patch tends to get old quickly. Feel free to throw it back to waldo ;)

Comment 24

[Nathan Froyd [:froydnj]]

Comment on attachment 8374739 [details] [diff] [review]
4 - Required changes in the tree

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

r=me with the changes below and assuming the compiler and tests are happy.  Thanks for doing this!

::: dom/bindings/Codegen.py
@@ +2617,5 @@
>      IDLType.Tags.double: ''
>  }
>  
>  def numericValue(t, v):
> +    if t == IDLType.Tags.unrestricted_double:

How about we instead say:

if (t == IDLType.Tags.unrestricted_double or
    t == IDLType.Tags.unrestricted_float):
    typeName = builtinNames[t]
    if v == float("inf"):
        return "mozilla::PositiveInfinity<%s>()" % typeName
    ...and so on...

instead.

::: gfx/2d/BaseRect.h
@@ +62,5 @@
>    {
> +    return (mozilla::IsFinite(double(x)) &&
> +            mozilla::IsFinite(double(y)) &&
> +            mozilla::IsFinite(double(width)) &&
> +            mozilla::IsFinite(double(height)));

I think it is technically more correct and slightly more efficient to say something like:

typedef typename mozilla::Conditional<mozilla::IsSame<T,float>::value, float, double>::Type FloatType;
mozilla::IsFinite(FloatType(x)) // and so forth

here, since we have specializations of IsFinite for float now.  Don't forget to #include "mozilla/TypeTraits.h" for those new templates.  And rs+ on fixing the mozilla/ #includes in this file to use "" instead of <>.

Comment 25

[Benjamin Bouvier [:bbouvier] (inactive)]

Thanks for the quick review! Try run for sanity.

https://tbpl.mozilla.org/?tree=Try&rev=03c4a63d7cfc

Comment 26

[Jeff Walden [:Waldo]]

Comment on attachment 8374739 [details] [diff] [review]
4 - Required changes in the tree

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

::: gfx/2d/BaseRect.h
@@ +62,5 @@
>    {
> +    return (mozilla::IsFinite(double(x)) &&
> +            mozilla::IsFinite(double(y)) &&
> +            mozilla::IsFinite(double(width)) &&
> +            mozilla::IsFinite(double(height)));

If T isn't float or double, then it's more or less a category error to ask if a value of the type is finite, because it *always* is.  (Unless it's long double, but I'm guessing that's not what's showing up here.)  Do whatever you need to to land this in the short run.  But in the longer run, casting x/y/width/height to a floating point type should never be necessary -- if it's not floating point to start (as judged by mozilla::IsFloatingPoint), then IsFinite should return false without doing any tests at all.

Unfortunately that gets into messy because you need to use template specialization, *and* you need it with access to private members.  Not so five-liner as the current code.  Oh well.

Comment 27

[Benjamin Bouvier [:bbouvier] (inactive)]

I think I will need to improve this part. The reason why I haven't landed the patches yet is that there is a leak that showed up on the last try build, with some "System JS: Error (null):0". Will investigate more.

Comment 28

[Benjamin Bouvier [:bbouvier] (inactive)]

After a few days of trying to find out what's wrong with my patches, seems like the "System JS" error is actually a known problem (see also bug 957666) and the leak was due to some other changeset as I can't reproduce it in try these days, so I'll land all of this now.

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/74196538e7a2
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/5de01e2586e9
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/98cd7d0d2a9b
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/192efb36136a

Comment 29

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/74196538e7a2
https://hg.mozilla.org/mozilla-central/rev/5de01e2586e9
https://hg.mozilla.org/mozilla-central/rev/98cd7d0d2a9b
https://hg.mozilla.org/mozilla-central/rev/192efb36136a