Re: [Qemu-devel] [PATCH] softfloat: roundAndPackFloat16 should return FPInfinity and FPMaxNormal based on overflow_to_inf.

[Xiangyu Hu]

On Wed, Feb 11, 2015 at 5:23 AM, Peter Maydell <address@hidden> wrote:

On 9 February 2015 at 14:56, Xiangyu Hu <address@hidden> 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...

Yes. Actually, a lot of single/double_to_half instructions would lead to inconsistent errors 

when calling roundAndPackFloat16. This fix on FPMaxNormal/FPInfinity handling

only recover some of them. Failure to justify "round_up" flag described in the manual

exposed other bugs.

I had another implementation that completely followed manual's FPRound(), which

brought no failed cases at all.

I wonder where current algrithm of roundAndPackFloat16 comes from?

 

> Signed-off-by: Xiangyu Hu <address@hidden>
> ---
>  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?"

 

Yes... my original thought it was not meaningful as the manual and

added the overflow_to_inf flag. 

> @@ -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.

That's a nice trick!

 

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.

Again, I am not quite following the whole implemented algorithm of 

this roundAndPackFloat16; it cannot align with roundAndPackFloat[32|64]

which originates softfloat(John Hauser).

 

thanks
-- PMM

Thanks!

- xiangyu