Re: [Qemu-devel] [PATCH v2 6/7] softfloat: Add SoftFloat status `nan2008_mode' flag

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

On 12 December 2014 at 19:35, Maciej W. Rozycki <macro@codesourcery.com> wrote:
> Add support for switching between legacy NaN and IEEE 754-2008 NaN modes
> where required, currently for the MIPS target only.  Also handle the
> saving and restoration of the `nan2008_mode' status flag.
>
> Use qNaN bit patterns for the 2008 NaN mode as from revision 5.00 [1][2]
> of the MIPS Architecture, updated from revision 3.50 that used a
> different choice.
>
> References:
>
> [1] "MIPS Architecture For Programmers, Volume I-A: Introduction to the
>     MIPS32 Architecture", MIPS Technologies, Inc., Document Number:
>     MD00082, Revision 5.02, April 30, 2013, Table 5.3 "Value Supplied
>     When a New Quiet NaN Is Created", p. 73
>
> [2] "MIPS Architecture For Programmers, Volume I-A: Introduction to the
>     MIPS64 Architecture", MIPS Technologies, Inc., Document Number:
>     MD00083, Revision 5.01, December 15, 2012, Table 5.3 "Value Supplied
>     When a New Quiet NaN Is Created", p. 73
>
> Signed-off-by: Thomas Schwinge <thomas@codesourcery.com>
> Signed-off-by: Maciej W. Rozycki <macro@codesourcery.com>
> ---
> Changes from v1:
>
> - regenerate on top of the SoftFloat relicensing patch set.
>
> qemu-softfloat-nan2008.diff
> Index: qemu-git-trunk/fpu/softfloat-specialize.h
> ===================================================================
> --- qemu-git-trunk.orig/fpu/softfloat-specialize.h      2014-12-11 22:42:41.128934304 +0000
> +++ qemu-git-trunk/fpu/softfloat-specialize.h   2014-12-11 22:43:02.128938514 +0000
> @@ -83,10 +83,16 @@ this code that are retained.
>   * by setting the most significant bit of the mantissa for a signaling NaN?
>   * (The more common choice is to have it be zero for SNaN and one for QNaN.)
>   */
> -#if defined(TARGET_MIPS) || defined(TARGET_SH4) || defined(TARGET_UNICORE32)
> -#define SNAN_BIT_IS_ONE 1
> +#if defined(TARGET_MIPS)
> +/* Has to be decided dynamically.  */
> +#define SNAN_BIT_IS_VARIABLE   1
> +#define SNAN_BIT_IS_ONE        0
> +#elif defined(TARGET_SH4) || defined(TARGET_UNICORE32)
> +#define SNAN_BIT_IS_VARIABLE   0
> +#define SNAN_BIT_IS_ONE        1
>  #else
> -#define SNAN_BIT_IS_ONE 0
> +#define SNAN_BIT_IS_VARIABLE   0
> +#define SNAN_BIT_IS_ONE        0
>  #endif
>
>  #if defined(TARGET_XTENSA)
> @@ -103,6 +109,10 @@ inline float16 float16_default_nan(STATU
>  {
>  #if defined(TARGET_ARM)
>      return const_float16(0x7E00);
> +#elif SNAN_BIT_IS_VARIABLE
> +    return STATUS(nan2008_mode)
> +           ? const_float16(0x7E00)
> +           : const_float16(0x7DFF);
>  #elif SNAN_BIT_IS_ONE
>      return const_float16(0x7DFF);
>  #else

Ah, I see now what the previous patch was in aid of.

(I hadn't realised that the 2008 rev of IEEE754 nailed down
the SNaN/QNaN bit sense. That was always a dumb thing to have
left impdef, so good news I guess.)

Can we get the ifdefs out of these functions entirely, by
something like

#if defined(TARGET_SH4) || etc
#define SNAN_BIT_IS_ONE(status) 1
#elif defined(TARGET_MIPS)
#define SNAN_BIT_IS_ONE(status) (status->nan2008_mode)
#else
#define SNAN_BIT_IS_ONE(status) 0

and then just have all the conversion functions do
   return SNAN_BIT_IS_ONE(status) ? x : y;

(that should also eliminate the need to mark x and y
as unused I think).

NB: you probably want to do that on top of my series
for getting rid of the STATUS macros.

> Index: qemu-git-trunk/include/fpu/softfloat.h
> ===================================================================
> --- qemu-git-trunk.orig/include/fpu/softfloat.h 2014-12-11 22:42:41.128934304 +0000
> +++ qemu-git-trunk/include/fpu/softfloat.h      2014-12-11 22:43:02.128938514 +0000
> @@ -223,6 +223,7 @@ typedef struct float_status {
>      /* should denormalised inputs go to zero and set the input_denormal flag? */
>      flag flush_inputs_to_zero;
>      flag default_nan_mode;

This could use a comment about what the semantics of the new flag are...

> +    flag nan2008_mode;
>  } float_status;
>
> Index: qemu-git-trunk/target-mips/cpu.h
> ===================================================================
> --- qemu-git-trunk.orig/target-mips/cpu.h       2014-12-11 22:42:41.128934304 +0000
> +++ qemu-git-trunk/target-mips/cpu.h    2014-12-11 22:43:02.128938514 +0000
> @@ -615,7 +615,11 @@ void mips_cpu_list (FILE *f, fprintf_fun
>  extern void cpu_wrdsp(uint32_t rs, uint32_t mask_num, CPUMIPSState *env);
>  extern uint32_t cpu_rddsp(uint32_t mask_num, CPUMIPSState *env);
>
> -#define CPU_SAVE_VERSION 7
> +#define CPU_SAVE_VERSION 8
> +/* We preserve compatibility with rev. 7 images.  */
> +#define CPU_SAVE_VERSION_OLDEST_SUPPORTED 7
> +/* Rev. 8 added 2008 NaN support.  */
> +#define CPU_SAVE_VERSION_2008_NAN 8

It would be nicer to split this into two patches: one adding
the new features to softfloat, and one using them in the MIPS
target.

thanks
-- PMM