From owner-freebsd-toolchain@FreeBSD.ORG Thu Jul 11 12:50:10 2013 Return-Path: Delivered-To: freebsd-toolchain@FreeBSD.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:1900:2254:206a::19:1]) by hub.freebsd.org (Postfix) with ESMTP id 93F7050F; Thu, 11 Jul 2013 12:50:08 +0000 (UTC) (envelope-from theraven@FreeBSD.org) Received: from theravensnest.org (theraven.freebsd.your.org [216.14.102.27]) by mx1.freebsd.org (Postfix) with ESMTP id E41761B1C; Thu, 11 Jul 2013 12:50:07 +0000 (UTC) Received: from c120.sec.cl.cam.ac.uk (c120.sec.cl.cam.ac.uk [128.232.18.120]) (authenticated bits=0) by theravensnest.org (8.14.5/8.14.5) with ESMTP id r6BCo1km015361 (version=TLSv1/SSLv3 cipher=AES128-SHA bits=128 verify=NO); Thu, 11 Jul 2013 12:50:02 GMT (envelope-from theraven@FreeBSD.org) Content-Type: multipart/signed; boundary="Apple-Mail=_C84CB7FF-D855-40EC-9DD9-FD206EB11660"; protocol="application/pgp-signature"; micalg=pgp-sha1 Mime-Version: 1.0 (Mac OS X Mail 6.3 \(1503\)) Subject: Re: CURRENT: CLANG 3.3 and -stad=c++11 and -stdlib=libc++: isnan()/isninf() oddity From: David Chisnall In-Reply-To: <20130711202908.L84170@besplex.bde.org> Date: Thu, 11 Jul 2013 13:49:57 +0100 Message-Id: References: <20130710155809.0f589c22@thor.walstatt.dyndns.org> <20130710183315.725dfde0@thor.walstatt.dyndns.org> <20130710203200.5359fd18@thor.walstatt.dyndns.org> <51DDC04B.6040209@FreeBSD.org> <20957.49978.73666.392417@khavrinen.csail.mit.edu> <20130711130043.R920@besplex.bde.org> <20130711202908.L84170@besplex.bde.org> To: Bruce Evans X-Mailer: Apple Mail (2.1503) Cc: freebsd-toolchain@FreeBSD.org, Garrett Wollman , FreeBSD CURRENT , Tijl Coosemans , freebsd-standards@FreeBSD.org X-BeenThere: freebsd-toolchain@freebsd.org X-Mailman-Version: 2.1.14 Precedence: list List-Id: Maintenance of FreeBSD's integrated toolchain List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 11 Jul 2013 12:50:10 -0000 --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 wrote: > 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--