| Message ID | 87obd6riy0.fsf@schwinge.name |
|---|---|
| State | Accepted, archived |
| Headers | show |
Hi! Ping. On Mon, 22 Apr 2013 11:52:23 +0200, I wrote: > On Fri, 5 Apr 2013 23:55:37 +0100, "Maciej W. Rozycki" <macro@codesourcery.com> wrote: > > On Fri, 5 Apr 2013, Thomas Schwinge wrote: > > > > 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 > > > I think the intent of this code is to preserve a NaN's payload (it > > certainly does for non-QUIET_NAN_NEGATED targets) > > I agree. For preserving the payload, both the unpack/pack code also has > to shift by NGARDS. > > > 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. > > I agree. > > > 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? > > I also stumbled over that, but for all these, I suppose the idea is that > when a sNaN is "arithmetically processed" (which includes datatype > conversion), an INVALID exception is to be raised (though, »[fp-bit] > implements IEEE 754 format arithmetic, but does not provide a mechanism > [...] for generating or handling exceptions«), and then converted into a > qNaN. > > Also, I found that the bit to look at for distinguishing qNaN/sNaN is > defined wrongly for float. Giving me some "interesting" test results... > ;-) > > Manual testing looks good. Automated testing is still running; in case > nothing turns up, is this OK to check in? > > libgcc/ > * fp-bit.c (unpack_d, pack_d): Properly preserve and restore a > NaN's payload. > * fp-bit.h [FLOAT] (QUIET_NAN): Correct value. > > Index: libgcc/fp-bit.c > =================================================================== > --- libgcc/fp-bit.c (revision 402061) > +++ libgcc/fp-bit.c (working copy) > @@ -214,11 +214,18 @@ pack_d (const fp_number_type *src) > else if (isnan (src)) > { > exp = EXPMAX; > + /* Restore the NaN's payload. */ > + fraction >>= NGARDS; > + fraction &= QUIET_NAN - 1; > if (src->class == CLASS_QNAN || 1) > { > #ifdef QUIET_NAN_NEGATED > - fraction |= QUIET_NAN - 1; > + /* The quiet/signaling bit remains unset. */ > + /* Make sure the fraction has a non-zero value. */ > + if (fraction == 0) > + fraction |= QUIET_NAN - 1; > #else > + /* Set the quiet/signaling bit. */ > fraction |= QUIET_NAN; > #endif > } > @@ -574,8 +581,10 @@ unpack_d (FLO_union_type * src, fp_number_type * d > { > dst->class = CLASS_SNAN; > } > - /* Keep the fraction part as the nan number */ > - dst->fraction.ll = fraction; > + /* Now that we know which kind of NaN we got, discard the > + quiet/signaling bit, but do preserve the NaN payload. */ > + fraction &= ~QUIET_NAN; > + dst->fraction.ll = fraction << NGARDS; > } > } > else > Index: libgcc/fp-bit.h > =================================================================== > --- libgcc/fp-bit.h (revision 402061) > +++ libgcc/fp-bit.h (working copy) > @@ -190,7 +190,7 @@ typedef unsigned int UTItype __attribute__ ((mode > # define EXPBIAS 127 > # define FRACBITS 23 > # define EXPMAX (0xff) > -# define QUIET_NAN 0x100000L > +# define QUIET_NAN 0x400000L > # define FRAC_NBITS 32 > # define FRACHIGH 0x80000000L > # define FRACHIGH2 0xc0000000L > @@ -298,7 +298,7 @@ typedef unsigned int UTItype __attribute__ ((mode > /* numeric parameters */ > /* F_D_BITOFF is the number of bits offset between the MSB of the mantissa > of a float and of a double. Assumes there are only two float types. > - (double::FRAC_BITS+double::NGARDS-(float::FRAC_BITS-float::NGARDS)) > + (double::FRAC_BITS+double::NGARDS-(float::FRAC_BITS+float::NGARDS)) > */ > #define F_D_BITOFF (52+8-(23+7)) > Grüße, Thomas
Hi! Ping. On Mon, 29 Apr 2013 10:16:52 +0200, I wrote: > Ping. > > On Mon, 22 Apr 2013 11:52:23 +0200, I wrote: > > On Fri, 5 Apr 2013 23:55:37 +0100, "Maciej W. Rozycki" <macro@codesourcery.com> wrote: > > > On Fri, 5 Apr 2013, Thomas Schwinge wrote: > > > > > 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 > > > > > I think the intent of this code is to preserve a NaN's payload (it > > > certainly does for non-QUIET_NAN_NEGATED targets) > > > > I agree. For preserving the payload, both the unpack/pack code also has > > to shift by NGARDS. > > > > > 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. > > > > I agree. > > > > > 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? > > > > I also stumbled over that, but for all these, I suppose the idea is that > > when a sNaN is "arithmetically processed" (which includes datatype > > conversion), an INVALID exception is to be raised (though, »[fp-bit] > > implements IEEE 754 format arithmetic, but does not provide a mechanism > > [...] for generating or handling exceptions«), and then converted into a > > qNaN. > > > > Also, I found that the bit to look at for distinguishing qNaN/sNaN is > > defined wrongly for float. Giving me some "interesting" test results... > > ;-) > > > > Manual testing looks good. Automated testing is still running; in case > > nothing turns up, is this OK to check in? > > > > libgcc/ > > * fp-bit.c (unpack_d, pack_d): Properly preserve and restore a > > NaN's payload. > > * fp-bit.h [FLOAT] (QUIET_NAN): Correct value. > > > > Index: libgcc/fp-bit.c > > =================================================================== > > --- libgcc/fp-bit.c (revision 402061) > > +++ libgcc/fp-bit.c (working copy) > > @@ -214,11 +214,18 @@ pack_d (const fp_number_type *src) > > else if (isnan (src)) > > { > > exp = EXPMAX; > > + /* Restore the NaN's payload. */ > > + fraction >>= NGARDS; > > + fraction &= QUIET_NAN - 1; > > if (src->class == CLASS_QNAN || 1) > > { > > #ifdef QUIET_NAN_NEGATED > > - fraction |= QUIET_NAN - 1; > > + /* The quiet/signaling bit remains unset. */ > > + /* Make sure the fraction has a non-zero value. */ > > + if (fraction == 0) > > + fraction |= QUIET_NAN - 1; > > #else > > + /* Set the quiet/signaling bit. */ > > fraction |= QUIET_NAN; > > #endif > > } > > @@ -574,8 +581,10 @@ unpack_d (FLO_union_type * src, fp_number_type * d > > { > > dst->class = CLASS_SNAN; > > } > > - /* Keep the fraction part as the nan number */ > > - dst->fraction.ll = fraction; > > + /* Now that we know which kind of NaN we got, discard the > > + quiet/signaling bit, but do preserve the NaN payload. */ > > + fraction &= ~QUIET_NAN; > > + dst->fraction.ll = fraction << NGARDS; > > } > > } > > else > > Index: libgcc/fp-bit.h > > =================================================================== > > --- libgcc/fp-bit.h (revision 402061) > > +++ libgcc/fp-bit.h (working copy) > > @@ -190,7 +190,7 @@ typedef unsigned int UTItype __attribute__ ((mode > > # define EXPBIAS 127 > > # define FRACBITS 23 > > # define EXPMAX (0xff) > > -# define QUIET_NAN 0x100000L > > +# define QUIET_NAN 0x400000L > > # define FRAC_NBITS 32 > > # define FRACHIGH 0x80000000L > > # define FRACHIGH2 0xc0000000L > > @@ -298,7 +298,7 @@ typedef unsigned int UTItype __attribute__ ((mode > > /* numeric parameters */ > > /* F_D_BITOFF is the number of bits offset between the MSB of the mantissa > > of a float and of a double. Assumes there are only two float types. > > - (double::FRAC_BITS+double::NGARDS-(float::FRAC_BITS-float::NGARDS)) > > + (double::FRAC_BITS+double::NGARDS-(float::FRAC_BITS+float::NGARDS)) > > */ > > #define F_D_BITOFF (52+8-(23+7)) > > Grüße, Thomas
Hi Iain! Joseph pointed out that I didn't include you, the libgcc and fp-bit maintainer, in my mails' recipient lists. I'm waiting for approval for the following changes: On Mon, 22 Apr 2013 11:52:23 +0200, I wrote: > On Fri, 5 Apr 2013 23:55:37 +0100, "Maciej W. Rozycki" <macro@codesourcery.com> wrote: > > On Fri, 5 Apr 2013, Thomas Schwinge wrote: > > > > 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 > > > I think the intent of this code is to preserve a NaN's payload (it > > certainly does for non-QUIET_NAN_NEGATED targets) > > I agree. For preserving the payload, both the unpack/pack code also has > to shift by NGARDS. > > > 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. > > I agree. > > > 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? > > I also stumbled over that, but for all these, I suppose the idea is that > when a sNaN is "arithmetically processed" (which includes datatype > conversion), an INVALID exception is to be raised (though, »[fp-bit] > implements IEEE 754 format arithmetic, but does not provide a mechanism > [...] for generating or handling exceptions«), and then converted into a > qNaN. > > Also, I found that the bit to look at for distinguishing qNaN/sNaN is > defined wrongly for float. Giving me some "interesting" test results... > ;-) > > Manual testing looks good. Automated testing is still running; in case > nothing turns up, is this OK to check in? > > libgcc/ > * fp-bit.c (unpack_d, pack_d): Properly preserve and restore a > NaN's payload. > * fp-bit.h [FLOAT] (QUIET_NAN): Correct value. > > Index: libgcc/fp-bit.c > =================================================================== > --- libgcc/fp-bit.c (revision 402061) > +++ libgcc/fp-bit.c (working copy) > @@ -214,11 +214,18 @@ pack_d (const fp_number_type *src) > else if (isnan (src)) > { > exp = EXPMAX; > + /* Restore the NaN's payload. */ > + fraction >>= NGARDS; > + fraction &= QUIET_NAN - 1; > if (src->class == CLASS_QNAN || 1) > { > #ifdef QUIET_NAN_NEGATED > - fraction |= QUIET_NAN - 1; > + /* The quiet/signaling bit remains unset. */ > + /* Make sure the fraction has a non-zero value. */ > + if (fraction == 0) > + fraction |= QUIET_NAN - 1; > #else > + /* Set the quiet/signaling bit. */ > fraction |= QUIET_NAN; > #endif > } > @@ -574,8 +581,10 @@ unpack_d (FLO_union_type * src, fp_number_type * d > { > dst->class = CLASS_SNAN; > } > - /* Keep the fraction part as the nan number */ > - dst->fraction.ll = fraction; > + /* Now that we know which kind of NaN we got, discard the > + quiet/signaling bit, but do preserve the NaN payload. */ > + fraction &= ~QUIET_NAN; > + dst->fraction.ll = fraction << NGARDS; > } > } > else > Index: libgcc/fp-bit.h > =================================================================== > --- libgcc/fp-bit.h (revision 402061) > +++ libgcc/fp-bit.h (working copy) > @@ -190,7 +190,7 @@ typedef unsigned int UTItype __attribute__ ((mode > # define EXPBIAS 127 > # define FRACBITS 23 > # define EXPMAX (0xff) > -# define QUIET_NAN 0x100000L > +# define QUIET_NAN 0x400000L > # define FRAC_NBITS 32 > # define FRACHIGH 0x80000000L > # define FRACHIGH2 0xc0000000L > @@ -298,7 +298,7 @@ typedef unsigned int UTItype __attribute__ ((mode > /* numeric parameters */ > /* F_D_BITOFF is the number of bits offset between the MSB of the mantissa > of a float and of a double. Assumes there are only two float types. > - (double::FRAC_BITS+double::NGARDS-(float::FRAC_BITS-float::NGARDS)) > + (double::FRAC_BITS+double::NGARDS-(float::FRAC_BITS+float::NGARDS)) > */ > #define F_D_BITOFF (52+8-(23+7)) > Grüße, Thomas
Thomas Schwinge <thomas@codesourcery.com> writes: >> libgcc/ >> * fp-bit.c (unpack_d, pack_d): Properly preserve and restore a >> NaN's payload. >> * fp-bit.h [FLOAT] (QUIET_NAN): Correct value. This is OK. Thanks. Ian
Hi! On Sun, 05 May 2013 18:55:09 -0700, Ian Lance Taylor <ian@airs.com> wrote: > Thomas Schwinge <thomas@codesourcery.com> writes: > > >> libgcc/ > >> * fp-bit.c (unpack_d, pack_d): Properly preserve and restore a > >> NaN's payload. > >> * fp-bit.h [FLOAT] (QUIET_NAN): Correct value. > > This is OK. Joseph suggested these two bug-fix commits (trunk r198621 and r198622) should be applied to earlier release branches, too. OK? Grüße, Thomas
Thomas Schwinge <thomas@codesourcery.com> writes: > Hi! > > On Sun, 05 May 2013 18:55:09 -0700, Ian Lance Taylor <ian@airs.com> wrote: >> Thomas Schwinge <thomas@codesourcery.com> writes: >> >> >> libgcc/ >> >> * fp-bit.c (unpack_d, pack_d): Properly preserve and restore a >> >> NaN's payload. >> >> * fp-bit.h [FLOAT] (QUIET_NAN): Correct value. >> >> This is OK. > > Joseph suggested these two bug-fix commits (trunk r198621 and r198622) > should be applied to earlier release branches, too. OK? The RM's, including Joseph, are in charge of the release branches. You don't need my separate OK. Ian
Hi! On Thu, 20 Jun 2013 15:08:16 -0700, Ian Lance Taylor <ian@airs.com> wrote: > Thomas Schwinge <thomas@codesourcery.com> writes: > > On Sun, 05 May 2013 18:55:09 -0700, Ian Lance Taylor <ian@airs.com> wrote: > >> Thomas Schwinge <thomas@codesourcery.com> writes: > >> > >> >> libgcc/ > >> >> * fp-bit.c (unpack_d, pack_d): Properly preserve and restore a > >> >> NaN's payload. > >> >> * fp-bit.h [FLOAT] (QUIET_NAN): Correct value. > >> > >> This is OK. > > > > Joseph suggested these two bug-fix commits (trunk r198621 and r198622) > > should be applied to earlier release branches, too. OK? Joseph, which release branches would you like me to commit these two patches to? > The RM's, including Joseph, are in charge of the release branches. You > don't need my separate OK. OK, thanks. Grüße, Thomas
On Fri, 21 Jun 2013, Thomas Schwinge wrote: > > > Joseph suggested these two bug-fix commits (trunk r198621 and r198622) > > > should be applied to earlier release branches, too. OK? > > Joseph, which release branches would you like me to commit these two > patches to? 4.7 and 4.8 (subject of course to testing there that they do fix the observed problems without causing regressions), since those are the active release branches. (A goal in particular being to avoid new glibc test failures on MIPS in glibc 2.18 from the tests you added verifying the kinds of NaNs resulting from functions, when building with current release branch versions of GCC. Unfortunately we may not have glibc and libgcc working quite so well together on Power Architecture, since there are several IBM long double bugs in libgcc causing glibc test failures for which patches haven't yet been produced for GCC trunk so can't be considered for release branch fixing yet.)
Index: libgcc/fp-bit.c =================================================================== --- libgcc/fp-bit.c (revision 402061) +++ libgcc/fp-bit.c (working copy) @@ -214,11 +214,18 @@ pack_d (const fp_number_type *src) else if (isnan (src)) { exp = EXPMAX; + /* Restore the NaN's payload. */ + fraction >>= NGARDS; + fraction &= QUIET_NAN - 1; if (src->class == CLASS_QNAN || 1) { #ifdef QUIET_NAN_NEGATED - fraction |= QUIET_NAN - 1; + /* The quiet/signaling bit remains unset. */ + /* Make sure the fraction has a non-zero value. */ + if (fraction == 0) + fraction |= QUIET_NAN - 1; #else + /* Set the quiet/signaling bit. */ fraction |= QUIET_NAN; #endif } @@ -574,8 +581,10 @@ unpack_d (FLO_union_type * src, fp_number_type * d { dst->class = CLASS_SNAN; } - /* Keep the fraction part as the nan number */ - dst->fraction.ll = fraction; + /* Now that we know which kind of NaN we got, discard the + quiet/signaling bit, but do preserve the NaN payload. */ + fraction &= ~QUIET_NAN; + dst->fraction.ll = fraction << NGARDS; } } else Index: libgcc/fp-bit.h =================================================================== --- libgcc/fp-bit.h (revision 402061) +++ libgcc/fp-bit.h (working copy) @@ -190,7 +190,7 @@ typedef unsigned int UTItype __attribute__ ((mode # define EXPBIAS 127 # define FRACBITS 23 # define EXPMAX (0xff) -# define QUIET_NAN 0x100000L +# define QUIET_NAN 0x400000L # define FRAC_NBITS 32 # define FRACHIGH 0x80000000L # define FRACHIGH2 0xc0000000L @@ -298,7 +298,7 @@ typedef unsigned int UTItype __attribute__ ((mode /* numeric parameters */ /* F_D_BITOFF is the number of bits offset between the MSB of the mantissa of a float and of a double. Assumes there are only two float types. - (double::FRAC_BITS+double::NGARDS-(float::FRAC_BITS-float::NGARDS)) + (double::FRAC_BITS+double::NGARDS-(float::FRAC_BITS+float::NGARDS)) */ #define F_D_BITOFF (52+8-(23+7))