On Wed, Feb 11, 2015 at 9:23 PM, Peter Maydell <peter.mayd...@linaro.org>
wrote:
> 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...
>
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 <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?"
>
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));
>
That's a nice trick!
>
> 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.
>
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
