On 9 February 2015 at 14:56, Xiangyu Hu <libhu...@gmail.com> wrote:
> Flag overflow_to_inf is specified by ARM manual to decide if
> FPInfinity(sign) or FPMaxNormal(sign) is returned. Current
> codepath fails to check it.
> Fixed by adding overflow_to_inf flag to return infinity when it's
> true and maxnormal otherwise.
I agree that this is wrong, but I think we want to fix it by
making roundAndPackFloat16 work more similarly to the
roundAndPackFloat32 and 64 functions...
> Signed-off-by: Xiangyu Hu <libhu...@gmail.com>
> ---
> fpu/softfloat.c | 12 +++++++++++-
> 1 file changed, 11 insertions(+), 1 deletion(-)
>
> diff --git a/fpu/softfloat.c b/fpu/softfloat.c
> index f1170fe..409a574 100644
> --- a/fpu/softfloat.c
> +++ b/fpu/softfloat.c
> @@ -3377,6 +3377,7 @@ static float32 roundAndPackFloat16(flag zSign,
> int_fast16_t zExp,
> uint32_t increment;
> bool rounding_bumps_exp;
> bool is_tiny = false;
> + bool overflow_to_inf = false;
>
> /* Calculate the mask of bits of the mantissa which are not
> * representable in half-precision and will be lost.
> @@ -3398,18 +3399,23 @@ static float32 roundAndPackFloat16(flag zSign,
> int_fast16_t zExp,
> if ((zSig & mask) == increment) {
> increment = zSig & (increment << 1);
> }
1) we move this fiddling with increment down in the function
to the point just before where we add increment to zSig
> + overflow_to_inf = true;
> break;
> case float_round_ties_away:
> increment = (mask + 1) >> 1;
> + overflow_to_inf = true;
> break;
> case float_round_up:
> increment = zSign ? 0 : mask;
> + overflow_to_inf = (zSign == 0);
> break;
> case float_round_down:
> increment = zSign ? mask : 0;
> + overflow_to_inf = (zSign == 1);
> break;
> default: /* round_to_zero */
> increment = 0;
> + overflow_to_inf = false;
> break;
> }
2) which means we don't need an extra flag, because now
"do we overflow to inf?" is always "is increment non-zero?"
> @@ -3418,7 +3424,11 @@ static float32 roundAndPackFloat16(flag zSign,
> int_fast16_t zExp,
> if (zExp > maxexp || (zExp == maxexp && rounding_bumps_exp)) {
> if (ieee) {
> float_raise(float_flag_overflow | float_flag_inexact, status);
> - return packFloat16(zSign, 0x1f, 0);
> + if (overflow_to_inf) {
> + return packFloat16(zSign, 0x1f, 0);
> + } else {
> + return packFloat16(zSign, 0x1e, 0x3ff);
> + }
3) and so you can just say
return packFloat16(zSign, 0x1f, -(increment == 0));
This is slightly tricky (we rely on packFloat16() adding
the significand argument into the value it builds up,
such that passing exponent X and significand -1 results
in a packed value with exponent X-1 and significand all-ones.)
But it's consistent with the way we handle this case in the
other roundAndPackFloat* functions, so I prefer it.
Deferring the adjustment of increment in the round_nearest_even
case looks at first glance like it might affect other cases,
because we calculate rounding_bumps_exp from increment. But
in fact rounding_bumps_exp will always have the same value
either way, if you work through the different possible
zSig values.
thanks
-- PMM
