Re: [Qemu-devel] [RFC PATCH] softfloat: re-factor float to float conversions

[Richard Henderson <richard.henderson@linaro.org>]

On 04/26/2018 05:40 AM, Alex Bennée wrote:
> +    bool arm_hp = !ieee && (src_sz == 16);

Surely you should be testing the destination format.
The float16_unpack_canonical step should be handling
the (non-)special cases to get into FloatParts.

It would probably be better to invert the parameter to be arm_hp rather than
ieee; that way you don't need to test the format; it'll be implied.

> +        /* Our only option now is to "re-pack" the NaN. As the
> +         * canonilization process doesn't mess with fraction bits for
> +         * NaNs we do it all here
> +         */
> +        switch (src_sz) {
> +        case 64:
> +            break;
> +        case 32:
> +            a.frac = deposit64(0, 29, 22, extract64(a.frac, 0, 22));
> +            break;
> +        case 16:
> +            a.frac = deposit64(0, 42, 9, extract64(a.frac, 0, 9));
> +            break;
> +        default:
> +            g_assert_not_reached();
> +            break;
> +        }
> +
> +        /* Get all the frac bits we need with MSB set */

This is assuming QNaN has MSB set.  We also support SNaN with MSB set.

> +        switch (dst_sz) {
> +        case 64:
> +            a.frac = (1ULL << 51) | extract64(a.frac, 0, 51);
> +            break;
> +        case 32:
> +            a.frac = (1 << 22) | extract64(a.frac, 29, 22);
> +            break;
> +        case 16:
> +            a.frac = (1 << 9) | extract64(a.frac, 42, 9);
> +            break;
> +        default:
> +            g_assert_not_reached();
> +            break;
> +        }

This would be better with

  a.frac = a.frac << (64 - src->frac_size) >> (64 - dst->frac_size);
  a.cls = float_class_msnan;

which has no such magic numbers.

> +    } else if (a.cls == float_class_inf && arm_hp) {
> +        /* FPProcessException(FPExc_InvalidOp, fpcr); */
> +        s->float_exception_flags |= float_flag_invalid;
> +        /* Alt HP returns result = sign:Ones(M-1) - faking qnan
> +         * forces round_canonical to use frac for the final bits
> +         */
> +        a.cls = float_class_qnan;
> +        a.frac = -1;

Wow, that's... ugly.  I would really prefer that you put the correct values
into the structure.

You'll need to pass ieee into float16_round_pack_canonical anyway, since
otherwise you'll get the wrong results for e.g. 1.111p31 (overflows to inf with
ieee hp; representable with armhp).


r~