Skip site navigation (1)Skip section navigation (2)
Date:      Fri, 29 May 2015 22:13:19 +1000 (EST)
From:      Bruce Evans <brde@optusnet.com.au>
To:        Andrew Turner <andrew@freebsd.org>
Cc:        src-committers@freebsd.org, svn-src-all@freebsd.org,  svn-src-head@freebsd.org
Subject:   Re: svn commit: r283693 - head/lib/libc/gen
Message-ID:  <20150529204844.H1965@besplex.bde.org>
In-Reply-To: <201505290923.t4T9NLrf029425@svn.freebsd.org>
References:  <201505290923.t4T9NLrf029425@svn.freebsd.org>

next in thread | previous in thread | raw e-mail | index | archive | help
On Fri, 29 May 2015, Andrew Turner wrote:

> Log:
>  Fix __isinfl on architectures where double == long double. This is the
>  case on at least ARM and PowerPC.

Urk.  I rarely look at the parts of libm misplaced outside of libc.

> Modified: head/lib/libc/gen/isinf.c
> ==============================================================================
> --- head/lib/libc/gen/isinf.c	Fri May 29 09:17:59 2015	(r283692)
> +++ head/lib/libc/gen/isinf.c	Fri May 29 09:23:20 2015	(r283693)
> @@ -26,6 +26,8 @@
>  * $FreeBSD$
>  */
>
> +#include <machine/float.h>
> +
> #include <math.h>
>
> #include "fpmath.h"
> @@ -62,9 +64,9 @@ __isinfl(long double e)
>
> 	u.e = e;
> 	mask_nbit_l(u);
> -#ifndef __alpha__
> -	return (u.bits.exp == 32767 && u.bits.manl == 0 && u.bits.manh == 0);
> -#else
> +#if LDBL_MANT_DIG == 53
> 	return (u.bits.exp == 2047 && u.bits.manl == 0 && u.bits.manh == 0);
> +#else
> +	return (u.bits.exp == 32767 && u.bits.manl == 0 && u.bits.manh == 0);
> #endif
> }

This is still quite bad (slow and unportable) code.  It should be
rarely executed, because is isinf() should be inlined (isinf() is a
type-generic macro and the specific float, double and long double
versions are only directly accessible as implementation details).
clang inlines it well.  However, isinf() is poorly implemented as a
selection of one of the slow extern versions in libc, so the compiler
can never see it to optimize it except using hacks like '#undef isinf'
-- the compiler can only see the calls to the slow extern versions
that it doesn't understand.  This is hard to implement better.
Translating isinf() to __builtin_isinf() is no good since
__builtin_isinf() doesn't exist for some compilers and is not inlined
by other compilers.

Also, most uses of mask_nbit_l() are wrong, and the one above is no
exception.  In the above, masking out the normalization bit prevents
detection of +-pseudo-Inf in ld80 format (same bits as +-Inf but
normalization bit clear).  IIRC, +-pseudo-Inf is treated as a
signaling NaN by the hardware.

The best implementation of isinf(x) seems to be (fabs(x) == INFINITY),
with certain complications.  clang's ininling gives exactly this on
both amd64 and i386 IIRC.  Complications:
- INFINITY is float precision, but that is sufficiently type-generic
- fabs() is unfortunately not type-generic, so selection of
   fabsf()/fabs()/fabsl() is needed.  fabs*() doesn't suffer from the
   same inlining problems as isinf() so it gets inlined properly
- perhaps fabs(x) can be avoided using (x) == -INFINITY ||
   (x) == INFINITY.  This requires more care to avoid multiple evaluation
   of (x) and a smarter compiler to convert it to fabs(x).  A smart
   compiler is also needed to do the reverse conversion if fabs(x) is
   slow
- problems with signaling NaNs.  Classification should work without
   generating an exception for signaling NaNs.  They sometimes do, and
   the code that looks at their bits as in the libc version has the
   best chance of avoiding exception.  clang's inlining and the direct
   comparison may be invalid since they always give the exception.
   However, just loading a value or passing it to a function may raise
   an exception.  This is very MD.  On i386, loading a signaling-NaN
   float or double raises an exception, but loading a long double one
   doesn't; passing a signaling value to a function tends to raise an
   exception give a signal for floats and doubles by loading them to
   copy them, but copying through integer registers doesn't.  On amd64,
   loading and parameter passing in the usual ways never raises an
   exception.  It is easiest to ignore this problem and let signaling
   NaNs raise an exception in undocumented circumstances.

Code that cares about efficiency, like most of libm, never uses isinf()
or other classification functions because the functions are or might be
too slow.  libm mostly avoids isinf() by looking at bits, just like this
libc code.  Looking at bits tends to be slow for long doubles and also
slow for doubles on 32-bit arches, but is the best possible method
when there is more than one thing to classify.  The extern versions of
isinf() and classification macros lose mainly by being extern, so that
if they are used then the bits would have to be extracted many times
per function.  Otherwise, libm uses:
- in cproj*(), csqrt*(), and ctanh*() just the slow standard macro.
   These functions wouldn't have passed an adequate review.
- in catrig*.c, a home made isinf(x) defined as fabs*(x) == INFINITY.

isnan() is less important than isinf(), but has similar problems.
Just about the same ones, except the one fixed in this commit.
Somehow isnan() never had a libc version for isnanl().

The current mess for isnan() in libc is:
- no isnan*() at all in libc in the !PIC case.  This works because
   the mess in libm and math.h is smaller for isnan*() (see below)
- __isnan() and __isnanf() in libc for the PIC case.  The source code
   has a ~10 year old comment saying that they are only there until
   we can bump libm's major version number.  This number has been
   bumped a few times.  Symbol versioning might make this a non-problem
   too.  Another part of the comment suggests that it is only a hack
   to not provide these functions in the !PIC case (to avoid linkage
   problems).  I use old libc.a that still has these functions and
   see the linkage problems, and hack around them in libm instead.

The current mess for isnan() in libm is:
- isnan(x) is not so poorly implemented as a selection of one of the
   static inline versions in math.h.  These use the simplistic
   (x) != (x) implementation.  I now seem to remember that this is
   due to the compatibility problems from conflicts with historical
   mistakes in libc was even larger than for isinf(), and we use
   these static inline versions because this avoids all linkage
   problems for fresh compiles and the implementation is good enough.

Home made versions of isnan() are not as easy to write as ones for
isinf(), because the portability of (x) != (x) is unclear.  This
expression the same unwanted signals for signaling NaNs as
(x) == INFINITY.  I think that is the only problem with IEEE NaNs.
Code that checks for NaNs is just more likely to care about the
signaling case than code that checks for Infinities.

Bruce



Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20150529204844.H1965>