Date: Thu, 11 Jul 2013 13:49:57 +0100 From: David Chisnall <theraven@FreeBSD.org> To: Bruce Evans <brde@optusnet.com.au> Cc: freebsd-toolchain@FreeBSD.org, Garrett Wollman <wollman@csail.mit.edu>, FreeBSD CURRENT <freebsd-current@FreeBSD.org>, Tijl Coosemans <tijl@FreeBSD.org>, freebsd-standards@FreeBSD.org Subject: Re: CURRENT: CLANG 3.3 and -stad=c++11 and -stdlib=libc++: isnan()/isninf() oddity Message-ID: <DE0B68CE-949E-48F1-8052-AF30C7F931F4@FreeBSD.org> In-Reply-To: <20130711202908.L84170@besplex.bde.org> References: <20130710155809.0f589c22@thor.walstatt.dyndns.org> <CD51F125-AE9E-4461-916D-CF583002B47D@FreeBSD.org> <20130710183315.725dfde0@thor.walstatt.dyndns.org> <C8C94CF2-7D5A-471B-AD63-8E961AED6274@FreeBSD.org> <20130710203200.5359fd18@thor.walstatt.dyndns.org> <51DDC04B.6040209@FreeBSD.org> <20957.49978.73666.392417@khavrinen.csail.mit.edu> <20130711130043.R920@besplex.bde.org> <FD768A6B-8B72-44A1-BC1C-14FF44CB4643@FreeBSD.org> <20130711202908.L84170@besplex.bde.org>
next in thread | previous in thread | raw e-mail | index | archive | help
--Apple-Mail=_C84CB7FF-D855-40EC-9DD9-FD206EB11660
Content-Transfer-Encoding: quoted-printable
Content-Type: text/plain;
charset=us-ascii
On 11 Jul 2013, at 13:11, Bruce Evans <brde@optusnet.com.au> wrote:
> <math.h> is also not required to be conforming C code, let alone C++ =
code,
> so there is only a practical requirement that it works when included
> in the C++ implementation.
Working with the C++ implementation is the problem that we are trying to =
solve.
> The compatibility that I'm talking about is with old versions of =
FreeBSD.
> isnan() is still in libc as a function since that was part of the =
FreeBSD
> ABI and too many things depended on getting it from there. It was =
recently
> removed from libc.so, but is still in libm.a. This causes some
> implementation problems in libm that are still not completely solved. =
I
> keep having to edit msun/src/s_isnan.c the msun sources are more =
portable.
> Mostly I need to kill the isnan() there so that it doesn't get in the
> way of the one in libc. This mostly works even if there is none in =
libc,
> since the builtins result in neither being used. isnanf() is more of =
a
> problem, since it is mapped to __isnanf() and there is no builtin for
> __isnanf(). The old functions have actually been removed from libc.a
> too. They only in libc_pic.a. libc.a still has isnan.o, but that is =
bogus
> since isnan.o is now empty.
I don't see a problem with changing the name of the function in the =
header and leaving the old symbol in libm for legacy code. =20
>> It would also be nice to implement these macros using _Generic when =
compiling in C11 mode, as it will allow the compiler to produce more =
helpful warning messages. I would propose this implementation:
>=20
>> #if __has_builtin(__builtin_isnan)
>=20
> This won't work for me, since I develop and test msun with old =
compilers
> that don't support __has_builtin(). Much the same set of compilers =
also
> don't have enough FP builtins.
Please look in cdefs.h, which defines __has_builtin(x) to 0 if we the =
compiler does not support it. It is therefore safe to use =
__has_builtin() in any FreeBSD header.
> It also doesn't even work. clang has squillions of builtins that
> aren't really builtines so they reduce to libcalls.
Which, again, is not a problem for code outside of libm. If libm needs =
different definitions of these macros then that's fine, but they should =
be private to libm, not installed as public headers.
> The msun implementation knows that isnan() and other classification
> macros are too slow to actually use, and rarely uses them. =20
Which makes any concerns that only apply to msun internals irrelevant =
from the perspective of discussing what goes into this header.
>> #define isnan(x) __builtin_isnan(x)
>> #else
>> static __inline int
>> __isnanf(float __x)
>> {
>> return (__x !=3D __x);
>> }
>=20
> Here we can do better in most cases by hard-coding this without the =
ifdef.
They will generate the same code. Clang expands the builtin in the LLVM =
IR to a fcmp uno, so will generate the correct code even when doing fast =
math optimisations.
>> static __inline int
>> __isnand(double __x)
>> {
>> return (__x !=3D __x);
>> }
>=20
> __isnand() is a strange name, and doesn't match compiler conventions =
for
> builtins. Compilers use __builtin_isnan() and map this to the libcall
> __isnan().
That's fine.
>> static __inline int
>> __isnanl(long double __x)
>> {
>> return (__x !=3D __x);
>> }
>> #if __STDC_VERSION__ >=3D 201112L
>> #define isnan(x) _Generic((x), \
>> float: __isnanf(x), \
>> double: __isnand(x), \
>> long double: __isnanl(x))
>=20
> Does _Generic() have no side effects, like sizeof()?
Yes.
>> #else
>> #define isnan(x) \
>> ((sizeof (x) =3D=3D sizeof (float)) ? __isnanf(x) \
>> : (sizeof (x) =3D=3D sizeof (double)) ? __isnand(x) \
>> : __isnanl(x))
>> #endif
>> #endif
>=20
> Both cases need to use __builtin_isnan[fl]() and depend on compiler
> magic to have any chance of avoiding side effects from loads and
> parameter passing.
> Generic stuff doesn't seem to work right for either isnan() or
> __builtin_isnan(), though it could for at least the latter. According
> to a quick grep of strings $(which clang), __builtin_classify() is
> generic but __builtin_isnan*() isn't (the former has no type suffixes
> but the latter does, and testing shows that the latter doesn't work
> without the suffices). =20
I'm not sure what you were testing:
$ cat isnan2.c=20
int test(float f, double d, long double l)
{
return __builtin_isnan(f) |
__builtin_isnan(d) |
__builtin_isnan(l);
}
$ clang isnan2.c -S -emit-llvm -o - -O1
...
%cmp =3D fcmp uno float %f, 0.000000e+00
%cmp1 =3D fcmp uno double %d, 0.000000e+00
%or4 =3D or i1 %cmp, %cmp1
%cmp2 =3D fcmp uno x86_fp80 %l, 0xK00000000000000000000
...
As you can see, it parses them as generics and generates different IR =
for each. I don't believe that there's a way that these would be =
translated back into libcalls in the back end.
>> For a trivial example of why this is an improvement in terms of error =
reporting, consider this trivial piece of code:
>>=20
>> int is(int x)
>> {
>> return isnan(x);
>> }
>>=20
>> With our current implementation, this compiles and links without any =
warnings, although it will have somewhat interesting results at run =
time. With the __builtin_isnan() version, clang reports this error:
>>=20
>> isnan.c:35:15: error: floating point classification requires argument =
of
>> floating point type (passed in 'int')
>> return isnan(x);
>> ^
>> (and then some more about the macro expansion)
>>=20
>> With the C11 version, it reports this error:
>>=20
>> isnan.c:35:15: error: controlling expression type 'int' not =
compatible with any
>> generic association type
>> return isnan(x);
>> ^
>=20
> The error message for the __builtin_isnan() version is slightly better =
up
> to where it says more.
I agree.
> The less-unportable macro can do more classification and detect =
problems
> at compile time using __typeof().
Yes, that would be an improvement, however all of our current =
type-generic macros in math.h use sizeof().
> isnan(integer) =3D 0 with no warnings is quite good though. Integers =
are
> never NaNs, and converting an integer to a floating point type should
> never give a NaN.
I disagree. isnan(integer) is almost certainly a mistake and so should =
be a compile-time error. If it is intentional, it relies on =
non-standard behaviour and so should be discouraged.
David
--Apple-Mail=_C84CB7FF-D855-40EC-9DD9-FD206EB11660
Content-Transfer-Encoding: 7bit
Content-Disposition: attachment;
filename=signature.asc
Content-Type: application/pgp-signature;
name=signature.asc
Content-Description: Message signed with OpenPGP using GPGMail
-----BEGIN PGP SIGNATURE-----
Version: GnuPG/MacGPG2 v2.0.18 (Darwin)
Comment: GPGTools - http://gpgtools.org
iQIcBAEBAgAGBQJR3qn2AAoJEKx65DEEsqIdl20P/1LQgcJ0nXFNpCOq37dEkMlY
hPc/5+f0g+2U27WGbaiEjwT1lD+Up598h3TmgZqlUsALGLVZ650tDlbHG6eF77US
zCmMuCbnWJcTitLaSBaIpvT8HBF5NrfHssPr5AXMaaO66AssKedgnL72GbCHz0CK
izxO+p4Wq+kQcG1WfXwhUJZrpCmCYZO1sdBztKN4e6EbI8RRUhM1ySce69yN3/nm
BlQbSJQqHttn35+Va9E3t02uXfZivtYjdxPphROymTMgfe4d9MJu85BZqZsrPkxm
AglIzrnIdFFkCz9bICrFX8/NNXhEHMHNlzGEhCOf9QUZWLrdiIyhI7FibOs/mtUi
RUxeiNupnXso32o0B2gcokm/swUkwiszjDfIyT8K/I3taBHyyrVEmZu2wZ40AZRx
TjI5a6pe/Phcq+5PmGFp90NXkONyQ7aCOPPRcWYVictw60BYy7AJDp/Iwa+X6rFI
ij3LSPONv+FUOYuJyNct9VIfT2zBHE71Q6LRPzto6PO50gsfVVUsBBl6eXL0ULz/
nWOCckUYHkVciPvdZkP/dV9z5t043KAJMdhkclpsY/h1eVZ9eNPj3T403RzLiWSW
hF7+t+dhOP5CkZ2wRe2ONDq6T/mLULaWhVWu5Rgp9VNFk62tWoqSdt2ZsybAjxjT
u5vg2zAg+cYrujbX8AN9
=A/T6
-----END PGP SIGNATURE-----
--Apple-Mail=_C84CB7FF-D855-40EC-9DD9-FD206EB11660--
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?DE0B68CE-949E-48F1-8052-AF30C7F931F4>