mips SNaN/QNaN is swapped

Hi!

Re-visiting this patch/commit after a mere ten years, ha, ha.  :-) Surely
everyone must still be having context on this issue...  (MIPS and soft-fp
maintainers CCed.)

On 01 Apr 2003 18:44:53 -0300, Alexandre Oliva <aoliva at redhat dot com> wrote:
> Index: gcc/ChangeLog
> from  Alexandre Oliva  <aoliva@redhat.com>
> 
> 	* real.h (EXP_BITS): Make room for...
> 	(struct real_value): ... added canonical bit.
> 	(struct real_format): Added pnan.
> 	(mips_single_format, mips_double_format, mips_extended_format,
> 	mips_quad_format): New.
> 	* real.c: Copy p to pnan in all formats.
> 	 (get_canonical_qnan, get_canonical_snan): Set canonical bit.
> 	(real_nan): Use pnan to compute significand's shift.
> 	(real_identical): Disregard significand in canonical
> 	NaNs.
> 	(real_hash): Likewise.  Take signalling into account.
> 	(encode_ieee_single, encode_ieee_double, encode_ieee_quad):
> 	Disregard significand bits in canonical NaNs.  Set all bits of
> 	canonical NaN if !qnan_msb_set.
> 	(encode_ibm_extended, decode_ibm_extended): Likewise.  Use
> 	qnan_msb_set to tell the base double format.
> 	(ibm_extended_format): Use 53 as pnan.
> 	(mips_single_format, mips_double_format, mips_extended_format,
> 	mips_quad_format): Copied from the corresponding ieee/ibm
> 	formats, with qnan_msb_set false.
> 	* config/mips/iris6.h (MIPS_TFMODE_FORMAT): Use mips_extended_format.
> 	* config/mips/linux64.h (MIPS_TFMODE_FORMAT): Use mips_quad_format.
> 	* config/mips/mips.c (override_options): Use mips_single_format
> 	and mips_double_format.  Default TFmode to mips_quad_format.
> 	* config/mips/t-linux64 (tp-bit.c): Define QUIET_NAN_NEGATED.
> 	* config/mips/t-irix6: Likewise.
> 	* config/mips/t-mips (fp-bit.c, dp-bit.c): Likewise.
> 	* config/fp-bit.c (pack_d, unpack_d): Obey it.

These days still, MIPS remains the only architecture to define
QUIET_NAN_NEGATED (though, HPPA would seem to be another one to do, as
Carlos recently confirmed), meaning that the topmost bit of the fraction,
indicating qNaN or sNaN, has to be interpreted the other way round than
for most other architectures (and as now defined in IEEE 754-2008).  For
a -m64 MIPS configuration (to enable the 128-bit floating-point datatype,
long double), we get, with the following test program:

    #include <stdio.h>
    
    extern int __issignalingf(float x);
    extern int __issignaling(double x);
    extern int __issignalingl(long double x);
    
    typedef union
    {
      long double f;
      int i[4];
    } u_ld;
    
    typedef union
    {
      double f;
      int i[2];
    } u_d;
    
    typedef union
    {
      float f;
      int i[1];
    } u_f;
    
    int main(void)
    {
      volatile u_f f;
      f.f = __builtin_nanf("");
      printf("%x\n", f.i[0]);
      if (__issignalingf(f.f))
        printf("  sNaN\n");
    
      volatile u_d d;
      d.f = __builtin_nan("");
      printf("%x %x\n", d.i[0], d.i[1]);
      if (__issignaling(d.f))
        printf("  sNaN\n");
      d.f = f.f;
      printf("%x %x\n", d.i[0], d.i[1]);
      if (__issignaling(d.f))
        printf("  sNaN\n");
      d.f = __builtin_nanf("");
      printf("%x %x\n", d.i[0], d.i[1]);
      if (__issignaling(d.f))
        printf("  sNaN\n");
    
      volatile u_ld ld;
      ld.f = __builtin_nanl("");
      printf("%x %x %x %x\n", ld.i[0], ld.i[1], ld.i[2], ld.i[3]);
      if (__issignalingl(ld.f))
        printf("  sNaN\n");
      ld.f = d.f;
      printf("%x %x %x %x\n", ld.i[0], ld.i[1], ld.i[2], ld.i[3]);
      if (__issignalingl(ld.f))
        printf("  sNaN\n");
      ld.f = __builtin_nan("");
      printf("%x %x %x %x\n", ld.i[0], ld.i[1], ld.i[2], ld.i[3]);
      if (__issignalingl(ld.f))
        printf("  sNaN\n");
      ld.f = f.f;
      printf("%x %x %x %x\n", ld.i[0], ld.i[1], ld.i[2], ld.i[3]);
      if (__issignalingl(ld.f))
        printf("  sNaN\n");
      ld.f = __builtin_nanf("");
      printf("%x %x %x %x\n", ld.i[0], ld.i[1], ld.i[2], ld.i[3]);
      if (__issignalingl(ld.f))
        printf("  sNaN\n");
    
      return 0;
    }

(The __issignaling* functions are those I recently implemented in glibc
commit 572676160d5639edc0ecb663147bd291841458d1; you can leave these out
and instead interpret the bit patterns.)

    7fbfffff
    7ff7ffff ffffffff
    7ff7ffff e0000000
    7ff7ffff ffffffff
    7fff7fff ffffffff ffffffff ffffffff
    7fffffff ffffffff ffffffff ffffffff
      sNaN
    7fff7fff ffffffff ffffffff ffffffff
    7fffffff ffffffff ffffffff ffffffff
      sNaN
    7fff7fff ffffffff ffffffff ffffffff

That is, for the cases where a float or double variable that contains a
qNaN value is assigned to a long double variable (that is, »ld.f = d.f;«
and »ld.f = f.f;«), the qNaN is unexpectedly turned into a sNaN during
the type conversion.

As I understand it (and I may add that this is the first time ever I'm
looking at soft-fp internals), that appears to be a bug in soft-fp, in
this very code added ten years ago ;-), which is invoked by means of
_df_to_tf.o:__extenddftf2 for this conversion operation.  (Forgive my
ignorance of MIPS ISA floating-point details, and not looking it up
myself at this late time of day -- why use soft-fp for that; isn't there
anything in hardware available to do it?)

> Index: gcc/config/fp-bit.c
> ===================================================================
> RCS file: /cvs/uberbaum/gcc/config/fp-bit.c,v
> retrieving revision 1.39
> diff -u -p -r1.39 fp-bit.c
> --- gcc/config/fp-bit.c 26 Jan 2003 10:06:57 -0000 1.39
> +++ gcc/config/fp-bit.c 1 Apr 2003 21:35:00 -0000
> @@ -210,7 +210,11 @@ pack_d ( fp_number_type *  src)
>        exp = EXPMAX;
>        if (src->class == CLASS_QNAN || 1)
>  	{
> +#ifdef QUIET_NAN_NEGATED
> +	  fraction |= QUIET_NAN - 1;
> +#else
>  	  fraction |= QUIET_NAN;
> +#endif
>  	}
>      }
>    else if (isinf (src))
> @@ -521,7 +525,11 @@ unpack_d (FLO_union_type * src, fp_numbe
>        else
>  	{
>  	  /* Nonzero fraction, means nan */
> +#ifdef QUIET_NAN_NEGATED
> +	  if ((fraction & QUIET_NAN) == 0)
> +#else
>  	  if (fraction & QUIET_NAN)
> +#endif
>  	    {
>  	      dst->class = CLASS_QNAN;
>  	    }

With the fix applied, we get the expected result:

    7fbfffff
    7ff7ffff ffffffff
    7ff7ffff e0000000
    7ff7ffff ffffffff
    7fff7fff ffffffff ffffffff ffffffff
    7fff7fff ffffffff ffffffff ffffffff
    7fff7fff ffffffff ffffffff ffffffff
    7fff7fff ffffffff ffffffff ffffffff
    7fff7fff ffffffff ffffffff ffffffff

Automated testing is still running; in case nothing turns up, does this
look OK to check in?



Grüße,
 Thomas

On Fri, 5 Apr 2013, Thomas Schwinge wrote:

> As I understand it (and I may add that this is the first time ever I'm
> looking at soft-fp internals), that appears to be a bug in soft-fp, in
> this very code added ten years ago ;-), which is invoked by means of
> _df_to_tf.o:__extenddftf2 for this conversion operation.  (Forgive my
> ignorance of MIPS ISA floating-point details, and not looking it up
> myself at this late time of day -- why use soft-fp for that; isn't there
> anything in hardware available to do it?)

 There's no direct MIPS hardware support for any FP data type wider than 
double.

> > Index: gcc/config/fp-bit.c
> > ===================================================================
> > RCS file: /cvs/uberbaum/gcc/config/fp-bit.c,v
> > retrieving revision 1.39
> > diff -u -p -r1.39 fp-bit.c
> > --- gcc/config/fp-bit.c 26 Jan 2003 10:06:57 -0000 1.39
> > +++ gcc/config/fp-bit.c 1 Apr 2003 21:35:00 -0000
> > @@ -210,7 +210,11 @@ pack_d ( fp_number_type *  src)
> >        exp = EXPMAX;
> >        if (src->class == CLASS_QNAN || 1)
> >  	{
> > +#ifdef QUIET_NAN_NEGATED
> > +	  fraction |= QUIET_NAN - 1;
> > +#else
> >  	  fraction |= QUIET_NAN;
> > +#endif
> >  	}
> >      }
> >    else if (isinf (src))
> > @@ -521,7 +525,11 @@ unpack_d (FLO_union_type * src, fp_numbe
> >        else
> >  	{
> >  	  /* Nonzero fraction, means nan */
> > +#ifdef QUIET_NAN_NEGATED
> > +	  if ((fraction & QUIET_NAN) == 0)
> > +#else
> >  	  if (fraction & QUIET_NAN)
> > +#endif
> >  	    {
> >  	      dst->class = CLASS_QNAN;
> >  	    }
> 
> With the fix applied, we get the expected result:
> 
>     7fbfffff
>     7ff7ffff ffffffff
>     7ff7ffff e0000000
>     7ff7ffff ffffffff
>     7fff7fff ffffffff ffffffff ffffffff
>     7fff7fff ffffffff ffffffff ffffffff
>     7fff7fff ffffffff ffffffff ffffffff
>     7fff7fff ffffffff ffffffff ffffffff
>     7fff7fff ffffffff ffffffff ffffffff
> 
> Automated testing is still running; in case nothing turns up, does this
> look OK to check in?
> 
> Index: libgcc/fp-bit.c
> ===================================================================
> --- libgcc/fp-bit.c	(revision 402061)
> +++ libgcc/fp-bit.c	(working copy)
> @@ -217,6 +217,9 @@ pack_d (const fp_number_type *src)
>        if (src->class == CLASS_QNAN || 1)
>  	{
>  #ifdef QUIET_NAN_NEGATED
> +	  /* Mask out the quiet/signaling bit.  */
> +	  fraction &= ~QUIET_NAN;
> +	  /* Set the remainder of the fraction to a non-zero value.  */
>  	  fraction |= QUIET_NAN - 1;
>  #else
>  	  fraction |= QUIET_NAN;

 I think the intent of this code is to preserve a NaN's payload (it 
certainly does for non-QUIET_NAN_NEGATED targets), so corrected code 
should IMHO look like:

#ifdef QUIET_NAN_NEGATED
	  fraction &= QUIET_NAN - 1;
	  if (fraction == 0)
	    fraction |= QUIET_NAN - 1;
#else
	  fraction |= QUIET_NAN;
#endif

or suchalike -- making sure a NaN is not accidentally converted to 
infinity where qNaNs are denoted by a zero bit (in which case the 
canonical qNaN is returned instead -- the code above is correct for MIPS 
legacy targets where the canonical qNaN has an all-ones payload; can't 
speak of HP-PA).  Complementing the change above I think it will also make 
sense to clear the qNaN bit when extracting a payload from fraction in 
unpack_d as the class of a NaN being handled is stored separately.

 Also I find the "|| 1" clause in the condition immediately above the 
pack_d piece concerned suspicious -- why is a qNaN returned for sNaN 
input?  Likewise why are __thenan_sf, etc. encoded as sNaNs rather than 
qNaNs?  Does anybody know?

  Maciej