930477 - IonMonkey: Specialize floor and round for float32

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[Jan de Mooij [:jandem]]

Attachment: Testcase

For the attached testcase I get the following output:

floor d 24
floor f 249
round d 38
round f 269
pow d 118
pow f 336

Especially Math.floor we can't regress. Benjamin, do you think we can get this fixed for Firefox 27? We have a few days until the next merge.

Comment 1

[Benjamin Bouvier [:bbouvier] (inactive)]

Challenge accepted. Let me give it a try

Comment 2

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: Floor + tests

This implements Math.floor specialization for Float32.

Try: (I am not sure that floorf is available on all platforms / compilers)
https://tbpl.mozilla.org/?tree=Try&rev=7ecc7d5392b7

Asking r? for jonco for the ARM parts, jandem for the rest.

Comment 3

[Jon Coppeard (:jonco)]

Comment on attachment 821722 [details] [diff] [review]
Floor + tests

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

ARM parts look good to me.

Comment 4

[Jan de Mooij [:jandem]]

Comment on attachment 821722 [details] [diff] [review]
Floor + tests

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

Nice, thanks! r=me with comments below addressed.

::: js/src/assembler/assembler/X86Assembler.h
@@ +1273,5 @@
>              m_formatter.immediate32(imm);
>          }
>      }
>  
> +    void cmpd_ir(int imm, RegisterID dst)

You can remove this method (see other comments).

@@ +1450,5 @@
>               nameIReg(8,src), nameIReg(8,dst));
>          m_formatter.oneByteOp64(OP_TEST_EvGv, src, dst);
>      }
>  
> +    void testd_rr(RegisterID src, RegisterID dst)

And this one.

::: js/src/jit/LIR-Common.h
@@ +4364,5 @@
>      LFloor(const LAllocation &num) {
>          setOperand(0, num);
>      }
>  
>      MRound *mir() const {

Pre-existing but this looks wrong (MRound instead of MFloor), can you just remove this method?

@@ +4379,5 @@
> +    LFloorF(const LAllocation &num) {
> +        setOperand(0, num);
> +    }
> +
> +    MRound *mir() const {

Same here.

::: js/src/jit/MIR.cpp
@@ +676,5 @@
> +            ConvertDefinitionToDouble<0>(input(), this);
> +        return;
> +    }
> +
> +    if (type() == MIRType_Double)

I think you can remove this and JS_ASSERT(type() == MIRType_Int32); ?

::: js/src/jit/x64/Assembler-x64.h
@@ +601,5 @@
>      }
>      void cmpq(const Register &lhs, Imm32 rhs) {
>          masm.cmpq_ir(rhs.value, lhs.code());
>      }
> +    void cmpd(const Register &lhs, Imm32 rhs) {

You can remove this method, see other comment.

::: js/src/jit/x64/MacroAssembler-x64.cpp
@@ +368,5 @@
> +Assembler::Condition
> +MacroAssemblerX64::testNegativeZeroFloat32(const FloatRegister &reg, const Register &scratch)
> +{
> +    movd(reg, scratch);
> +    cmpd(scratch, Imm32(1));

s/cmpd/cmpl

::: js/src/jit/x86/MacroAssembler-x86.cpp
@@ +402,5 @@
>      return Zero;
>  }
> +
> +Assembler::Condition
> +MacroAssemblerX86::testNegativeZeroFloat32(const FloatRegister &reg, const Register &scratch)

I think we can do better for float32 and use what we have for x64:

movd(reg, scratch);
cmpl(scratch, Imm32(1));
return Overflow;

Comment 5

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: Floor + tests + updated x86 code

Carrying forward r+. Ready to land, as soon as jandem approves the diff.

Comment 6

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: diff.patch

Not asking for review but just for feedback for this part. This diff contains all the changes I had to add for the x86 backend to work too. Basically, it just consists in using fstp for 32 bits instead of 64 bits when retrieving the result of the called function.

Comment 7

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: Floor + tests + updated x86 code

Forgot to qref for compilation on x64.

Carrying forward r+

Comment 8

[Jan de Mooij [:jandem]]

Comment on attachment 821891 [details] [diff] [review]
diff.patch

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

Nice find.

Comment 9

[Benjamin Bouvier [:bbouvier] (inactive)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f55bec181790

Comment 10

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/f55bec181790

Comment 11

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: Round + tests

This patch specializes Math.round for Float32 inputs. It makes round on par with doubles for the given test case.

Fixed a bug in ARM by the way: result of a MMathFunction call wasn't moved in the return result if the Result kind was FLOAT. Curious that this didn't show up last time I ran the tests.

Funny that libc's round() function rounds -0.5 to -1 while Math.round(-0.5) rounds it to -0. I launched a try run to be sure that all platforms have roundf:
https://tbpl.mozilla.org/?tree=Try&rev=c8933f55d03d

Asking for review to Marty for the ARM parts and Jan for the rest.

Comment 12

[Jan de Mooij [:jandem]]

Comment on attachment 828344 [details] [diff] [review]
Round + tests

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

Thanks,

::: js/src/jit/MIR.cpp
@@ +685,5 @@
>  
> +void
> +MRound::trySpecializeFloat32()
> +{
> +    // No need to look at the output, as it's an integer (unique way to have this instruction in IonBuilder::inlineMathRound)

Nit: comment doesn't fit in 79 characters. Also add a JS_ASSERT(type() == MIRType_Int32).

Comment 13

[Marty Rosenberg [:mjrosenb]]

Comment on attachment 828344 [details] [diff] [review]
Round + tests

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

::: js/src/jit/arm/MacroAssembler-arm.cpp
@@ +3613,5 @@
>  {
>      if (secondScratchReg_ != lr)
>          ma_mov(secondScratchReg_, lr);
>  
> +    if (result == DOUBLE || result == FLOAT) {

This doesn't look correct.  When the result is a float, it will be returned in s0, not d0, so the two possible transfers should both be changed to the floating point equivalents.

Comment 14

[Jan de Mooij [:jandem]]

Comment on attachment 828344 [details] [diff] [review]
Round + tests

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

Restore r+ flag.

Comment 15

[Benjamin Bouvier [:bbouvier] (inactive)]

Thanks for the reviews! As the try run shows, the build didn't work on Win XP, which doesn't implement roundf, so I have slightly more work to do here...

Comment 16

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: roundf impl for all platforms

I wish I had something more interesting to have you review. This is almost only copy and paste, with s/double/float and the right values at the right places.

The jit-tests in the previous patch pass, thus the function is correct.

Comment 17

[Jeff Walden [:Waldo]]

Comment on attachment 829552 [details] [diff] [review]
roundf impl for all platforms

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

I didn't look at the FloatingPoint.h stuff, as I'll just need to reread it all super-carefully to verify all the algorithms in a unified world anyway.

::: js/src/jsmath.cpp
@@ +47,2 @@
>  using mozilla::ExponentComponent;
> +using mozilla::FloatExponentComponent;

Keep these alphabetical, not enough value in pairing stuff up, I think.

@@ +789,5 @@
>      return js_copysign(floor(x + 0.5), x);
>  }
>  
> +float
> +js::math_roundf_impl(float x)

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.

This is totally ripe for all sorts of errors, especially fenceposting ones, without a bunch of tests.  :-)

@@ +793,5 @@
> +js::math_roundf_impl(float x)
> +{
> +    int32_t i;
> +    if (FloatIsInt32(x, &i))
> +        return float(i);

Seems like

    int32_t ignored;
    if (FloatIsInt32(x, &ignored))
        return x;

would be better wrt returning its input unchanged.

@@ +797,5 @@
> +        return float(i);
> +
> +    /* Some numbers are so big that adding 0.5 would give the wrong number. */
> +    if (FloatExponentComponent(x) >= mozilla::FloatExponentShift)
> +        return x;

ExponentComponent is signed.  ExponentShift is unsigned.  By the usual arithmetic conversions, the signed value will be promoted to an unsigned value, using the normal modular arithmetic.  So ExponentComponent, on sufficiently small numbers (I think anything 0.5 or lower in magnitude) won't be rounded by this method!  Unless I'm horribly misreading, but I'm pretty sure I'm not.

Did I mention tests yet?

@@ +799,5 @@
> +    /* Some numbers are so big that adding 0.5 would give the wrong number. */
> +    if (FloatExponentComponent(x) >= mozilla::FloatExponentShift)
> +        return x;
> +
> +    return js_copysign(floorf(x + 0.5f), x);

We should move js_copysign into FloatingPoint.h at some point.  Separate patch, separate bug if you prefer, should be quick to do.

::: mfbt/FloatingPoint.h
@@ +289,5 @@
>    MOZ_ASSERT(IsFloatNaN(f));
>    return f;
>  }
>  
>  /**

I think we're at the point with FloatingPoint.h that we should condense the float/double implementations into one, to avoid typos and the like.  Something like:

namespace mozilla {

namespace detail {

struct FloatTypeTraits
{
    typedef uint32_t Bits;

    const unsigned FloatExponentBias = 127;
    const unsigned FloatExponentShift = 23;

    static const Bits SignBit         = 0x80000000UL;
    static const Bits ExponentBits    = 0x7F800000UL;
    static const Bits SignificandBits = 0x007FFFFFUL;
};

struct DoubleTypeTraits
{
    typedef uint64_t Bits;

    static const unsigned ExponentBias = 1023;
    static const unsigned ExponentShift = 52;

    static const Bits SignBit         = 0x8000000000000000ULL;
    static const Bits ExponentBits    = 0x7ff0000000000000ULL;
    static const Bits SignificandBits = 0x000fffffffffffffULL;
};

template<typename T> struct SelectTrait;
template<float> struct SelectTrait : FloatTypeTraits {};
template<double> struct SelectTrait : DoubleTypeTraits {};

template<typename T>
struct FPTraits : SelectTrait<T>
{
    typedef typename SelectTrait<T> Base;

    static_assert((Base::SignBit & Base::ExponentBits) == 0,
                  "sign bit shouldn't overlap exponent bits");
    static_assert((Base::SignBit & Base::SignificandBits) == 0,
                  "sign bit shouldn't overlap significand bits");
    static_assert((Base::ExponentBits & Base::SignificandBits) == 0,
                  "exponent bits shouldn't overlap significand bits");

    static_assert((Base::SignBit | Base::ExponentBits | Base::SignificandBits) ==
                  ~Base::Bits(0),
                  "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
     * to be the case.  But we required this in implementations of these algorithms
     * that preceded this header, so we shouldn't break anything if we keep doing so.
     */
    static_assert(sizeof(T) == sizeof(Base::Bits), "Bits must be same size as T");
};

} // namespace detail

template<typename T>
static MOZ_ALWAYS_INLINE bool
IsNaN(T t)
{
  typedef typename FPTraits<T> Traits;
  Traits::Bits bits = BitwiseCast<Traits::Bits>(t);
  return (bits & Traits::ExponentBits) == Traits::ExponentBits &&
         (bits & Traits::SignificandBits) != 0;
}

// ...and so on

} // namespace mozilla

Comment 18

[Jeff Walden [:Waldo]]

Ah, you had tests in a separate patch.  Eyeballing quickly, it looks like you had double tests where |x| < 0.5 but not any float ones.  Err on the side of more tests!  :-)

Comment 19

[Benjamin Bouvier [:bbouvier] (inactive)]

Thanks for the careful review! Making FloatingPoint.h more generic seems a good idea, and as I wouldn't want to spam this thread (and there might also be some uses of FloatingPoint.h that would need to be updated), I filed bug 939843.

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #18)
> Ah, you had tests in a separate patch.  Eyeballing quickly, it looks like
> you had double tests where |x| < 0.5 but not any float ones.  Err on the
> side of more tests!  :-)

Heh, the "double tests" denomination is kinda wrong, this deserves a comment in the tests I added to explain the difference between testRound and testDoubleRound. Actually, when creating the MIR nodes, the baseline can conclude that the return value of a Math.round can be an Integer (first round (pun not intended) of tests) or a Double (second round, which tests < 0.5 values, etc.). The above roundf function is actually called if and only if the returned value is expected to be a Double, so this was actually tested and passing on my machine, but not for all special cases you mentioned above.

Comment 20

[Jan de Mooij [:jandem]]

Benjamin, what's the next step here? Can we land these patches?

Comment 21

[Benjamin Bouvier [:bbouvier] (inactive)]

Not yet, bug 939843 has to be implemented and finished before, so that we have a nice implementation of roundf for all platforms (win xp doesn't have one). I'll work on bug 939843 again soon.

Comment 22

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: Roundf for all platforms

Long time no see, bug 930477! Here is a new patch based on the last API from bug 939843, fixing the related other comments.

Waldo, there is no C++ tests for the JS maths so far. To avoid the unsigned / signed comparison, I have used the same trick as in the equivalent double function, which is to write directly the value here (dirty)(but it works).
Should we begin some C++ js_math tests too?

Comment 23

[Benjamin Bouvier [:bbouvier] (inactive)]

FWIW, try is green. https://tbpl.mozilla.org/?tree=Try&rev=f47ad07d3df8

Comment 24

[Benjamin Bouvier [:bbouvier] (inactive)]

Pow doesn't strictly commute -> can't specialize it.

Comment 25

[Jan de Mooij [:jandem]]

(In reply to Benjamin Bouvier [:bbouvier] from comment #24)
> Pow doesn't strictly commute -> can't specialize it.

OK, but can you make sure we get the same numbers for "pow d" and "pow f" for the attached testcase? When I filed this bug, "pow f" was almost 3x slower (see comment 0), it would be good to fix that by converting to double or something.

Comment 26

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: Parity for Pow

Sure thing. This inlines the MPow or MPowHalf even if the input is a Float32, but the DoublePolicy (or PowPolicy) will convert these to a double before applying the instruction.

Comment 27

[Jan de Mooij [:jandem]]

Comment on attachment 8379100 [details] [diff] [review]
Parity for Pow

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

Thanks!

::: js/src/jit/MCallOptimize.cpp
@@ +779,2 @@
>          return InliningStatus_NotInlined;
> +    if (powerType != MIRType_Int32 && !IsFloatingPointType(powerType))

Nit: if (!IsNumberType(powerType)), same for baseType.

Comment 28

[Benjamin Bouvier [:bbouvier] (inactive)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7a5cbe4dadf8

Comment 29

[Niko Matsakis [:nmatsakis]]

I already reported this to bbouvier on IRC, but I thought I'd add a note to bugzilla. This patch seems to have caused a slowdown in Octane 2 Mandreel of ~50% (or at least this is what I saw when running tests manually).

Comment 30

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/7a5cbe4dadf8

Comment 31

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: Parity for Pow - Followup

Got r+ from h4writer over IRC. Fixes bug 975290 and the regression. Added test case from bug 975920.

https://hg.mozilla.org/integration/mozilla-inbound/rev/9ac8d347e6c6

Comment 32

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/9ac8d347e6c6

Comment 33

[Jeff Walden [:Waldo]]

Comment on attachment 8374165 [details] [diff] [review]
Roundf for all platforms

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

::: js/src/jit/CodeGenerator.cpp
@@ +4326,5 @@
> +      case MMathFunction::ASin:  funptr = JS_FUNC_TO_DATA_PTR(void *, asinf);            break;
> +      case MMathFunction::ACos:  funptr = JS_FUNC_TO_DATA_PTR(void *, acosf);            break;
> +      case MMathFunction::Floor: funptr = JS_FUNC_TO_DATA_PTR(void *, floorf);           break;
> +      case MMathFunction::Round: funptr = JS_FUNC_TO_DATA_PTR(void *, math_roundf_impl); break;
> +      case MMathFunction::Ceil:  funptr = JS_FUNC_TO_DATA_PTR(void *, ceilf);            break;

Blugh, this is why I dislike |expr; break;|, with attempted break-alignment, on the same line as a case.  I would favor expanding this into the normal multiple-line thing, myself.

::: js/src/jsmath.cpp
@@ +751,3 @@
>  
>      /* Some numbers are so big that adding 0.5 would give the wrong number. */
>      if (ExponentComponent(x) >= 52)

After you land bug 939843, could you make a followup that changes this to mozilla::FloatingPoint<double>::ExponentShift?

@@ +763,5 @@
> +    if (NumberIsInt32(x, &ignored))
> +        return x;
> +
> +    /* Some numbers are so big that adding 0.5 would give the wrong number. */
> +    if (ExponentComponent(x) >= 23)

And this to mozilla::FloatingPoint<float>::ExponentShift.

@@ +766,5 @@
> +    /* Some numbers are so big that adding 0.5 would give the wrong number. */
> +    if (ExponentComponent(x) >= 23)
> +        return x;
> +
> +    return js_copysign(floorf(x + 0.5f), x);

Probably a mozilla::CopySign method would be useful, perhaps mozilla::Floor as well, given the C-required floor/floorf/floorl(?) mess, and the messiness of defining js_copysign across compilers.  Separate bug.

Comment 34

[Jeff Walden [:Waldo]]

(In reply to Benjamin Bouvier [:bbouvier] from comment #22)
> Waldo, there is no C++ tests for the JS maths so far. To avoid the unsigned
> / signed comparison, I have used the same trick as in the equivalent double
> function, which is to write directly the value here (dirty)(but it works).
> Should we begin some C++ js_math tests too?

I don't quite follow what's being commented on, or asked about, here.

Comment 35

[Benjamin Bouvier [:bbouvier] (inactive)]

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #34)
> I don't quite follow what's being commented on, or asked about, here.

Waldo, may I quote what Waldo-from-the-past said about using ExponentShift in this condition.

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #17)
> @@ +797,5 @@
> > +        return float(i);
> > +
> > +    /* Some numbers are so big that adding 0.5 would give the wrong number. */
> > +    if (FloatExponentComponent(x) >= mozilla::FloatExponentShift)
> > +        return x;
> 
> ExponentComponent is signed.  ExponentShift is unsigned.  By the usual
> arithmetic conversions, the signed value will be promoted to an unsigned
> value, using the normal modular arithmetic.  So ExponentComponent, on
> sufficiently small numbers (I think anything 0.5 or lower in magnitude)
> won't be rounded by this method!  Unless I'm horribly misreading, but I'm
> pretty sure I'm not.
> 
> Did I mention tests yet?

That's what I was referring to. You were asking at some points for tests, but didn't precise whether you wanted tests for the Floating point generic MFBT operations OR the js maths functions. So I was asking whether you wanted tests for js maths, and simply observed that there weren't any I could see so far.

Comment 36

[Benjamin Bouvier [:bbouvier] (inactive)]

Try looks good. Added a coercion to int16_fast_t in the comparisons (same return type as ExponentComponent()) and everything is fine.

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/a1b396e1f1dd
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/a07b45e32644

Comment 37

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/a1b396e1f1dd
https://hg.mozilla.org/mozilla-central/rev/a07b45e32644

Comment 38

[Jeff Walden [:Waldo]]

(In reply to Benjamin Bouvier [:bbouvier] from comment #35)
> (In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #34)
> > I don't quite follow what's being commented on, or asked about, here.
> 
> Waldo, may I quote what Waldo-from-the-past said about using ExponentShift
> in this condition.
> 
> (In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #17)
> > @@ +797,5 @@
> > > +        return float(i);
> > > +
> > > +    /* Some numbers are so big that adding 0.5 would give the wrong number. */
> > > +    if (FloatExponentComponent(x) >= mozilla::FloatExponentShift)
> > > +        return x;
> > 
> > ExponentComponent is signed.  ExponentShift is unsigned.  By the usual
> > arithmetic conversions, the signed value will be promoted to an unsigned
> > value, using the normal modular arithmetic.  So ExponentComponent, on
> > sufficiently small numbers (I think anything 0.5 or lower in magnitude)
> > won't be rounded by this method!  Unless I'm horribly misreading, but I'm
> > pretty sure I'm not.
> > 
> > Did I mention tests yet?
> 
> That's what I was referring to. You were asking at some points for tests,
> but didn't precise whether you wanted tests for the Floating point generic
> MFBT operations OR the js maths functions. So I was asking whether you
> wanted tests for js maths, and simply observed that there weren't any I
> could see so far.

Ah!  The code you're implementing is in the JS engine, exposed purely via JS stuff.  So this should be a test in JS.  (The mfbt stuff already largely has tests.  We just need them for algorithms that use the low-level mfbt primitive bits, without just using them exactly in a 1-to-1 relationship.)  I suggest changing the C++, writing the test and verifying it fails as I noted there it should, then undoing the C++ change, to be sure the test would have properly caught the mistake.

Comment 39

[Jeff Walden [:Waldo]]

Okay, this is weird.  math_round_impl has the exact same ExponentComponent(d) >= FloatingPoint<T>::ExponentShift thing going on, and on Windows int_fast16_t is int, and so Math.round(.125) should wrongly be .125 there, it appears.  But it doesn't seem to be if I test in a nightly.  Dunno what's up.  But definitely round and roundf should be identical in these respects here, and they're not, and somehow round() is bad, which *really* should have been caught by our existing tests, so I really dunno what's up here.  (int_fast16_t is long [!] on my new Linux box, so I can't reproduce anything there.  Strange, I thought I remembered it being int even on 64-bit there, before, but maybe not.)

Comment 40

[Benjamin Bouvier [:bbouvier] (inactive)]

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #39)
> Okay, this is weird.  math_round_impl has the exact same
> ExponentComponent(d) >= FloatingPoint<T>::ExponentShift thing going on, and
> on Windows int_fast16_t is int, and so Math.round(.125) should wrongly be
> .125 there, it appears.  But it doesn't seem to be if I test in a nightly. 
> Dunno what's up.  But definitely round and roundf should be identical in
> these respects here, and they're not, and somehow round() is bad, which
> *really* should have been caught by our existing tests, so I really dunno
> what's up here.  (int_fast16_t is long [!] on my new Linux box, so I can't
> reproduce anything there.  Strange, I thought I remembered it being int even
> on 64-bit there, before, but maybe not.)

I've added a conversion from FloatingPoint<T>::ExponentShift to int_fast16_t, which is safe as the exponent shift is <= 52 (double exponent shift) and 52 has the same binary representation as an int, long or unsigned (at least in base 2). That makes the comparison safe. In the meanwhile, once again, as the unsigned and the int / long representations are the same for these values, it doesn't seem dangerous to compare them against each other directly and I suppose that's the reason why the compiler doesn't complain about it.

For sanity, should we add an assert that int_fast16_t(FloatingPoint<T>::ExponentShift) >= 0 and <= 52?