Re: [Qemu-devel] [PATCH v2 08/27] fpu/softfloat: Replace float_class_dnan with parts_default_nan

[Peter Maydell <peter.maydell@linaro.org>]

On 12 May 2018 at 01:42, Richard Henderson <richard.henderson@linaro.org> wrote:
> With a canonical representation of NaNs, we can return the
> default nan directly rather than delay the expansion until
> the final format is known.
>
> Note one case where we uselessly assigned to a.sign, which was
> overwritten/ignored later when expanding float_class_dnan.
>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>  fpu/softfloat-specialize.h | 37 +++++++++++++++++++++++++++++++++++++
>  fpu/softfloat.c            | 38 +++++++++++---------------------------
>  2 files changed, 48 insertions(+), 27 deletions(-)
>
> diff --git a/fpu/softfloat-specialize.h b/fpu/softfloat-specialize.h
> index 515cb12cfa..0d3d81a52b 100644
> --- a/fpu/softfloat-specialize.h
> +++ b/fpu/softfloat-specialize.h
> @@ -101,6 +101,43 @@ static bool parts_is_snan_frac(uint64_t frac, float_status *status)
>  #endif
>  }
>
> +/*----------------------------------------------------------------------------
> +| The pattern for a default generated deconstructed floating-point NaN.
> +*----------------------------------------------------------------------------*/
> +
> +static FloatParts parts_default_nan(float_status *status)
> +{
> +    bool sign = 0;
> +    uint64_t frac;
> +
> +#if defined(TARGET_SPARC) || defined(TARGET_M68K)
> +    frac = (1ULL << DECOMPOSED_BINARY_POINT) - 1;
> +#elif defined(TARGET_PPC) || defined(TARGET_ARM) || defined(TARGET_ALPHA) || \
> +      defined(TARGET_S390X) || defined(TARGET_RISCV)
> +    frac = 1ULL << (DECOMPOSED_BINARY_POINT - 1);
> +#elif defined(TARGET_HPPA)
> +    frac = 1ULL << (DECOMPOSED_BINARY_POINT - 2);
> +#else
> +    if (status->snan_bit_is_one) {
> +        frac = (1ULL << (DECOMPOSED_BINARY_POINT - 1)) - 1;
> +    } else {
> +#if defined(TARGET_MIPS)
> +        frac = 1ULL << (DECOMPOSED_BINARY_POINT - 1);
> +#else
> +        frac = 1ULL << (DECOMPOSED_BINARY_POINT - 1);
> +        sign = 1;
> +#endif
> +    }
> +#endif

We can probably clean this logic up later (for instance the MIPS
case is the same as the PPC/ARM/Alpha/S390/RISCV case, and it
would be helpful to specifically indicate who's using the
weird set-the-sign-bit case rather than having that be the
default), but this matches the current logic in float*_default_nan()
so it's easier to review this way.

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM