CURRENT: CLANG 3.3 and -stad=c++11 and -stdlib=libc++: isnan()/isninf() oddity

Post by Tijl Coosemans
I think isnan(double) and isinf(double) in math.h should only be
visible if (_BSD_VISIBLE || _XSI_VISIBLE) && __ISO_C_VISIBLE < 1999.
For C99 and higher there should only be the isnan/isinf macros.

I believe you are correct. POSIX.1-2008 (which is aligned with C99)
consistently calls isnan() a "macro", and gives a pseudo-prototype of

int isnan(real-floating x);

-GAWollman

Bruce Evans

2013-07-11 04:21:19 UTC

Post by Garrett Wollman

I believe you are correct. POSIX.1-2008 (which is aligned with C99)
consistently calls isnan() a "macro", and gives a pseudo-prototype of
int isnan(real-floating x);

Almost any macro may be implemented as a function, if no conforming
program can tell the difference. It is impossible for technical reasons
to implement isnan() as a macro (except on weird implementations where
all real-floating types are physically the same). In the FreeBSD
implementation, isnan() is a macro, but it is also a function, and
the macro expands to the function in double precision:

% #define isnan(x) \
% ((sizeof (x) == sizeof (float)) ? __isnanf(x) \
% : (sizeof (x) == sizeof (double)) ? isnan(x) \
% : __isnanl(x))

I don't see how any conforming program can access the isnan() function
directly. It is just as protected as __isnan() would be. (isnan)()
gives the function (the function prototype uses this), but conforming
programs can't do that since the function might not exist. Maybe some
non-conforming program like autoconfig reads <math.h> or libm.a and
creates a bug for C++.

The FreeBSD isnan() implementation would be broken by removing the
isnan() function from libm.a or ifdefing it in <math.h>. Changing the
function to __isnan() would cause compatibility problems. The function
is intentionally named isnan() to reduce compatibility problems.

OTOH, the all of the extern sub-functions that are currently used should
bever never be used, since using them gives a very low quality of
implementation:
- the functions are very slow
- the functions have names that confuse compilers and thus prevent
compilers from replacing them by builtins. Currently, only gcc
automatically replaces isnan() by __builtin_isnan(). This only
works in double precision. So the FreeBSD implementation only
works right in double precision too, only with gcc, __because__
it replaces the macro isnan(x) by the function isnan(x). The
result is inline expansion, the same as if the macro isnan()
is replaced by __builtin_isnan(). clang never does this automatic
replacement, so it generates calls to the slow library functions.
Other things go wrong for gcc in other precisions:
- if <math.h> is not included, then isnan(x) gives
__builtin_isnan((double)x). This sort of works on x86, but is
low quality since it is broken for signaling NaNs (see below).
One of the main reasons reason for the existence of the
classification macros is that simply converting the arg to a common
type and classifying the result doesn't always work.
- if <math.h> is not included, then spelling the API isnanf() or
isnanl() gives correct results but a warning about these APIs
not being declared. These APIs are nonstandard but are converted
to __builtin_isnan[fl] by gcc.
- if <math.h> is included, then:
- if the API is spelled isnan(), then the macro converts to
__isnanf() or __isnanl(). gcc doesn't understand these, and
the slow extern functions are used.
- if the API is spelled isnanf() or isnanl(), then the result is
correct and the warning magically goes away. <math.h> declares
isnanf(), but gcc apparently declares both iff <math.h> is included.
gcc also optimizes isnanl() on a float arg to __builtin_isnanf().
- no function version can work in some cases, because any function version
may have unwanted side effects. This is another of the main reason
for the existence of these and other macros. The main unwanted side
effect is signaling for signaling NaNs. C99 doesn't really support
signaling NaNs, even with the IEC 60559 extensions, so almost anything
is allowed for them. But IEEE 854 is fairly clear that isnan() and
classification macros shouldn't raise any exceptions. IEEE 854 is
even clearer that copying values without changing their representation
should (shall?) not cause exceptions. But on i387, just loading a float
or double value changes its representation and generates an exception
for signaling NaNs, while just loading a long double value conforms to
IEEE 854 and doesn't change its representation or generate an exception.
Passing of args to functions may or may not load the values. ABIs may
require a change of representation. On i387, passing of double args
should go through the FPU for efficiency reasons, and this changes the
representation twice to not even get back to the original (for signaling
NaNs, it generates an exception and sets the quiet bit in the result;
thus a classification function can never see a signaling NaN in double
precision). So a high quality inplementation must not use function
versions, and it must also use builtins that don't even load the values
into normal FP registers if this might cause an exception or change the
values. Both gcc's and clang's builtins are broken on i387 (i386 or
amd64 -m32) since the do load the value.

In view of all these bugs, the best available implementation of isnan(x)
is ((x) != (x)) (using an unportable statement-expression to avoid
multiple evaluation). The known bugs in this implementation are:
- it will load the value and thus give unwanted exceptions for signaling
NaNs in some cases. But compiler builtins are no better.
(IEEE 854 has a lot to say about exceptions for comparison operators,
and the builtin comparison operators exist in C99 for related reasons.
IIRC, the comparison operators are useless with IEEE 854 conformance,
since ordinary comparison operators then do the right thing. The details
are unclear, but I couldn't find any cases where gcc or clang on x86
do the wrong thing or do different things for the builtin comparison
operators. For ((x) != (x)), they generate an "unordered" comparison
and this is the right thing for isnan(). Their builtin isnan()s generate
exactly the same code as for ((x) != (x)).)
- it may be broken by flags like -ffast-math that turn off IEEE conformance.

Bruce

David Chisnall

2013-07-11 08:46:17 UTC

Hi Bruce,

You're joining in this discussion starting in the middle, so you probably missed the earlier explanation.

Post by Bruce Evans
I don't see how any conforming program can access the isnan() function
directly. It is just as protected as __isnan() would be. (isnan)()
gives the function (the function prototype uses this), but conforming
programs can't do that since the function might not exist. Maybe some
non-conforming program like autoconfig reads <math.h> or libm.a and
creates a bug for C++.

The cmath header defines a template function isnan that invokes the isnan macro, but then undefines the isnan macro. This causes a problem because when someone does something along the lines of using namespace std then they end up with two functions called isnan and the compiler gets to pick the one to use. Unfortunately, std::isnan() returns a bool, whereas isnan() returns an int.

The C++ headers are not required to be conforming C code, because they are not C, and our math.h causes namespace pollution in C++ when included from <cmath>.

Post by Bruce Evans
The FreeBSD isnan() implementation would be broken by removing the
isnan() function from libm.a or ifdefing it in <math.h>. Changing the
function to __isnan() would cause compatibility problems. The function
is intentionally named isnan() to reduce compatibility problems.

On OS X this is avoided because their isnan() macro expands to call one of the __-prefixed inline functions (which adopt your suggestion of being implemented as x != x, for all types). I am not sure that this is required for standards conformance, but it is certainly cleaner. Your statement that having the function not called isnan() causes compatibility problems is demonstrably false, as neither OS X nor glibc has a function called isnan() and, unlike us, they do not experience problems with this macro.

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:

#if __has_builtin(__builtin_isnan)
#define isnan(x) __builtin_isnan(x)
#else
static __inline int
__isnanf(float __x)
{
return (__x != __x);
}
static __inline int
__isnand(double __x)
{
return (__x != __x);
}
static __inline int
__isnanl(long double __x)
{
return (__x != __x);
}
#if __STDC_VERSION__ >= 201112L
#define isnan(x) _Generic((x), \
float: __isnanf(x), \
double: __isnand(x), \
long double: __isnanl(x))
#else
#define isnan(x) \
((sizeof (x) == sizeof (float)) ? __isnanf(x) \
: (sizeof (x) == sizeof (double)) ? __isnand(x) \
: __isnanl(x))
#endif
#endif

For a trivial example of why this is an improvement in terms of error reporting, consider this trivial piece of code:

int is(int x)
{
return isnan(x);
}

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:

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)

With the C11 version, it reports this error:

isnan.c:35:15: error: controlling expression type 'int' not compatible with any
generic association type
return isnan(x);
^

David

Tijl Coosemans

2013-07-11 09:37:20 UTC

Post by Garrett Wollman

I believe you are correct. POSIX.1-2008 (which is aligned with C99)
consistently calls isnan() a "macro", and gives a pseudo-prototype of
int isnan(real-floating x);

The C99 standard says isnan is a macro. I would say that only means
defined(isnan) is true. Whether that macro then expands to function
calls or not is not important.

I don't think the standard allows a function to be declared with the same
name as a standard macro (it does allow the reverse: define a macro with
the same name as a standard function). I believe the following code is
C99 conforming but it currently does not compile with our math.h:

------
#include <math.h>

int (isnan)(int a, int b, int c) {
return (a + b + c);
}
------

Bruce Evans

2013-07-11 12:11:00 UTC

Post by David Chisnall
You're joining in this discussion starting in the middle, so you probably missed the earlier explanation.

I was mainly addressing a C99 point. I know little about C++ or C11.

The cmath header defines a template function isnan that invokes the isnan macro, but then undefines the isnan macro. This causes a problem because when someone does something along the lines of using namespace std then they end up with two functions called isnan and the compiler gets to pick the one to use. Unfortunately, std::isnan() returns a bool, whereas isnan() returns an int.
The C++ headers are not required to be conforming C code, because they are not C, and our math.h causes namespace pollution in C++ when included from <cmath>.

<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.

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.

Post by David Chisnall
#if __has_builtin(__builtin_isnan)

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.

It also doesn't even work. clang has squillions of builtins that
aren't really builtines so they reduce to libcalls. gcc has fewer
builtins, but still many that reduce to libcalls. An example is fma().
__has_builtin(__builtin_fma) is true for clang on amd64 (freefall),
but at least freefalls's CPU doesn't support fma in hardware, so the
builtin can't really work, and in fact it doesn't -- it reduces to a
libcall. This might change if the hardware supports fma, but then
__has_builtin(__builtin_fma) would be even more useless for telling
if fma is worth using. C99 has macros FP_FAST_FMA[FL] whose
implementation makes them almost equally useless. For example, ia64
has fma in hardware and the implementation defines all of
FP_FAST_FMA[FL] for ia64. But fma is implemented as an extern function,
partly because there is no way to tell if __builtin_fma is any good
(but IIRC, __builtin_fma is no good on ia64 either, since it reduces
to the same extern function). The extern function is slow (something
like 20 cycles instead of 1 for the fma operation). But if you ignore
the existence of the C99 fma API and just write expressions of the
form (a*x + b), then gcc on ia64 will automatically use the hardware
fma, although this is technically wrong in some fenv environments.

For gcc-4.2.1, __has_builtin(__builtin_fma) is a syntax error. I
test with gcc-3.x. It is also missing __builtin_isnan().

The msun implementation knows that isnan() and other classification
macros are too slow to actually use, and rarely uses them. Sometimes
it is inherent that it can do better, because it already knows the
bits in the macros and can test the bits directly. I have had limited
tests with arranging access macros so that loads of the bits can be
shared.

Post by David Chisnall
#define isnan(x) __builtin_isnan(x)
#else
static __inline int
__isnanf(float __x)
{
return (__x != __x);
}

Here we can do better in most cases by hard-coding this without the ifdef.

Post by David Chisnall
static __inline int
__isnand(double __x)
{
return (__x != __x);
}

__isnand() is a strange name, and doesn't match compiler conventions for
builtins. Compilers use __builtin_isnan() and map this to the libcall
__isnan().

Post by David Chisnall
static __inline int
__isnanl(long double __x)
{
return (__x != __x);
}
#if __STDC_VERSION__ >= 201112L
#define isnan(x) _Generic((x), \
float: __isnanf(x), \
double: __isnand(x), \
long double: __isnanl(x))

Does _Generic() have no side effects, like sizeof()?

Post by David Chisnall
#else
#define isnan(x) \
((sizeof (x) == sizeof (float)) ? __isnanf(x) \
: (sizeof (x) == sizeof (double)) ? __isnand(x) \
: __isnanl(x))
#endif
#endif

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). It is the most useful FP builtins like
__builtin_fabs*() that aren't generic. C99 probably prevents fabs*()
being generic, but I think any builtin can be generic, and the isnan()
macro is specified to be generic, so if compilers understood it directly
their builtin for it needs to be and can be generic.

Post by David Chisnall
int is(int x)
{
return isnan(x);
}
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)
isnan.c:35:15: error: controlling expression type 'int' not compatible with any
generic association type
return isnan(x);
^

The error message for the __builtin_isnan() version is slightly better up
to where it says more.

The less-unportable macro can do more classification and detect problems
at compile time using __typeof().

isnan(integer) = 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.

Bruce

David Chisnall

2013-07-11 12:49:57 UTC

Post by Bruce Evans
<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.

Post by Bruce Evans
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.

Post by David Chisnall
#if __has_builtin(__builtin_isnan)

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.

Post by Bruce Evans
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.

Post by Bruce Evans
The msun implementation knows that isnan() and other classification
macros are too slow to actually use, and rarely uses them.

Which makes any concerns that only apply to msun internals irrelevant from the perspective of discussing what goes into this header.

Post by David Chisnall
#define isnan(x) __builtin_isnan(x)
#else
static __inline int
__isnanf(float __x)
{
return (__x != __x);
}

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.

Post by David Chisnall
static __inline int
__isnand(double __x)
{
return (__x != __x);
}

__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.

Does _Generic() have no side effects, like sizeof()?

Yes.

Post by David Chisnall
#else
#define isnan(x) \
((sizeof (x) == sizeof (float)) ? __isnanf(x) \
: (sizeof (x) == sizeof (double)) ? __isnand(x) \
: __isnanl(x))
#endif
#endif

I'm not sure what you were testing:

$ cat isnan2.c

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 = fcmp uno float %f, 0.000000e+00
%cmp1 = fcmp uno double %d, 0.000000e+00
%or4 = or i1 %cmp, %cmp1
%cmp2 = 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.

The error message for the __builtin_isnan() version is slightly better up
to where it says more.

I agree.

Post by Bruce Evans
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().

Post by Bruce Evans
isnan(integer) = 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

Bruce Evans

2013-07-11 13:02:10 UTC

Post by Tijl Coosemans

Post by Garrett Wollman

I believe you are correct. POSIX.1-2008 (which is aligned with C99)
consistently calls isnan() a "macro", and gives a pseudo-prototype of
int isnan(real-floating x);

The C99 standard says isnan is a macro. I would say that only means
defined(isnan) is true. Whether that macro then expands to function
calls or not is not important.

I think it means only that defined(isnan) is true. isnan() can still be
a function (declared or just in the compile-time namespace somewhere,
or in a library object). It is reserved in the compile-time namespace,
and the standard doesn't cover library objects, so conforming applications
can't reference either except via the isnan() macro (if that has its
strange historical implementation).

Post by Tijl Coosemans

I don't think the standard allows a function to be declared with the same
name as a standard macro (it does allow the reverse: define a macro with
the same name as a standard function). I believe the following code is
------
#include <math.h>
int (isnan)(int a, int b, int c) {
return (a + b + c);
}
------

I think isnan is just reserved, so you can't redefine it an any way. I
think the reverse is even less allowed. Almost any standard function may
be implemented as a macro, and then any macro definition of it would
conflict with the previous macro even more than with a previous prototype.
E.g.:

/* Header. */
void exit(int);
#define exit(x) __exit(x)

/* Application. */
#undef exit /* non-conforming */
#define exit(x) my_exit(x) /* conflicts without the #undef */

Now suppose the header doesn't define exit().

#define exit(x) my_exit(x)

This hides the protoype but doesn't automatically cause problems, especially
if exit() is not used after this point. But this is still non-conforming,
since exit() is reserved.

Here are some relevant parts of C99 (n869.txt):

%%%
-- Each identifier with file scope listed in any of the
following subclauses (including the future library
directions) is reserved for use as macro and as an
identifier with file scope in the same name space if
any of its associated headers is included.

[#2] No other identifiers are reserved. If the program
declares or defines an identifier in a context in which it
is reserved (other than as allowed by 7.1.4), or defines a
reserved identifier as a macro name, the behavior is
undefined.

[#3] If the program removes (with #undef) any macro
definition of an identifier in the first group listed above,
the behavior is undefined.
%%%

Without any include of a header that is specified to declare exit(),
file scope things are permitted for it, including defining it and
making it a static function, but not making it an extern function.

isnan is reserved for use as a macro and as an identifier with file
scope by the first clause above. Thus (isnan) cannot even be defined
as a static function. But (isnan) is not reserved in inner scopes.
I thought that declarations like "int (isnan);" are impossible since
they look like syntax errors, but this syntax seems to be allowed an
actually work with gcc-3.3.3 and TenDRA-5.0.0. So you can have
variables with silly names like (isnan) and (getchar) :-). However,
(NULL) for a variable name doesn't work, and (isnan) is a syntax error
for struct member names. The compilers may be correct in allowing
(isnan) but not (NULL) for variables. isnan happens to be function-like,
so the parentheses are special for (isnan), but the parentheses are not
special for (NULL). cpp confirms this -- NULL inside of parentheses
still gets expanded. The above quote of C99 can cover both since it
just says that isnan is reserved for use as a macro. In (isnan), the
parentheses prevent isnan being interpreted as a macro so the reservation
doesn't apply.

Bruce

David Chisnall

2013-07-11 14:33:34 UTC

Post by Bruce Evans
The error message for the __builtin_isnan() version is slightly better up
to where it says more.
The less-unportable macro can do more classification and detect problems
at compile time using __typeof().

The attached patch fixes the related test cases in the libc++ test suite. Please review.

This does not use __builtin_isnan(), but it does:

- Stop exposing isnan and isinf in the header. We already have __isinf in libc, so this is used instead.

- Call the static functions for isnan __inline__isnan*() so that they don't conflict with the ones in libm.

- Add an __fp_type_select() macro that uses either __Generic(), __builtin_choose_expr() / __builtin_choose_expr(), or sizeof() comparisons, depending on what the compiler supports.

- Refactor all of the type-generic macros to use __fp_type_select().

David

Bruce Evans

2013-07-11 15:28:26 UTC

Post by Bruce Evans
<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.

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.

I don't even see why old code needs the symbol. Old code should link to
old compat libraries that still have it.

Post by David Chisnall
#if __has_builtin(__builtin_isnan)

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.

The old compilers run on old systems that don't have that in cdefs.h
(though I sometimes edit it to add compatibility cruft like that). msun
sources are otherwise portable to these systems. Well, not quite. They
are not fully modular and also depend on stuff in libc/include and
libc/${ARCH}. I have to update or edit headers there.

This hack also doesn't work with gcc in -current. gcc has __builtin_isnan
but not __has_builtin(), so __has_builtin(__builtin_isnan) gives the wrong
result 0.

Post by Bruce Evans
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.

Yes it is. It means that nothing should use isnan() or FP_FAST_FMA* outside
of libm either, since isnan() is too slow and FP_FAST_FMA* can't be trusted.
Even the implementation can't reliably tell if __builtin_isnan is usuable
or better than alternatives.

Post by Bruce Evans
The msun implementation knows that isnan() and other classification
macros are too slow to actually use, and rarely uses them.

Which makes any concerns that only apply to msun internals irrelevant from the perspective of discussing what goes into this header.

No, the efficiency of isnan() is more important for externals, because the
internals already have work-arounds.

Post by David Chisnall
#define isnan(x) __builtin_isnan(x)
#else
static __inline int
__isnanf(float __x)
{
return (__x != __x);
}

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.

On some arches the same, and not affected by -ffast-math. But this
is not necessarily the fastest code, so it is a performance bug if clang
akways generates the same code for the builtin. Bit tests are faster in
some cases, and may be required to prevent exceptions for signaling NaNs.
-ffast-math could reasonably optimize x != x to "false". It already assumes
that things like overflow and NaN results can't happen, so why not optimize
further by assuming that NaN inputs can't happen?

Post by Bruce Evans
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).

Mostly isnan() without including <math.h>, and gcc. I was confused by
gcc converting floats to doubles.

Post by David Chisnall
$ cat isnan2.c
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 = fcmp uno float %f, 0.000000e+00
%cmp1 = fcmp uno double %d, 0.000000e+00
%or4 = or i1 %cmp, %cmp1
%cmp2 = 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.

Yes, most cases work right. gcc converts f to double and compares the
result, but that mostly works. It would be just a pessimization except
te conversion gives an exception for signaling NaNs. I hope I remember
correctly that the comparision doesn't give this exception. When the
above is changed to use __builtin_isnanf() on the float, gcc does the
right thing (no conversion). The almost-correct handling of the float
is apparently not completely accidental, since for the long double gcc
doesn't convert to double before comparing.

With gcc, you also don't have to use __builtin_isnan*() to get the
builtins. gcc doesn't warn about the undeclared isnan(), and generates
the same code as the above.

You can also use isnanf() on the float to avoid the conversion.
isnanf() is nonstandard, so gcc warns about it. If isnan() is declared
as a function as in <math.h>, then gcc forgets that it is standard and
generates a conversion for the long double case too. This almost works,
like the conversion for the float case, since changing the precision
doesn't affect the classification

With clang, you only get the builtins if you use them explicitly.
clang generates verbose warnings for undeclared isnan() and always
calls the named function. Conversions occur according to the prototype,
if any.

The above was tested only on amd64.

isinf() is even more interesting and broken. Now gcc doesn't convert
automatically to builtins. It does the reverse, in broken ways when
__builtin_isinf() is used on floats and long doubles:
- it converts the float to double to create the arg. Mostly works, as above.
- it doesn't convert the long double to double, but copies it to the stack
pessimally through integer registers. The pessimization can be fixed
by compiling with -march=core2.
isinf() is called twice with a double arg and once with a long double arg.
This shows that gcc doesn't really understand its own builtin, but is just
passing the values with the default promotions that occur when there is
no prototype in scope.

When __builtin_isinff() is used on the float arg and __builtin_isinfl()
is used on the long double arg, gcc passes the args correctly except
for pessimizing the long double. It converts __builtin_isinff() to
isinff(), etc. This is very broken, since isinff() is in the application
namespace. Apart from that, it causes the following problems:
- isinff(), isinf() and isinfl() can't easily be removed from libraries
- in general, there is no way to know the name of a libcall functions that
might be generated for builtins that don't really exist. gcc also
converts __builtin_fma() to fma(). That works and is not a namespace
error since fma() is a standard API that must exist as a function.

clang does different things wrong for __builtin_isinf(). It gets the
type stuff right, as for __builtin_isnan(). It never generates
libcalls, so it doesn't need a differently named set of extern
functions. But it has the major pessimization of calling extern
functions for fabs(), fabsf() and fabsl(). The latter is weird, since
clang optimizes explicit fabs*() calls almost perfectly on amd64 using
SSE bit operations for floats and doubles and i387 hardware fabs for
long doubles (bit operations also work for long doubles, but are less
convenient and much slower when done wrong).

I probably knew some of the previous paragraph when I wrote a local
isinf() for catrig*.c. catrig*.c is unlike most of msun. It uses
classification macros a lot. Efficiency investigations showed that
these work OK provided they always use efficient builtins and in
particular, never use the standard macros (except isnan() = itself ->
builtin for gcc). isinf(x) is just (fabs(x) == INFINITY), like clang
generates for __builtin_isinf(), but much better since the fabs() is
explicit so clang doesn't neglect to optimize it. This works well for
gcc too (3.x and 4.2.1).

Bruce

Bruce Evans

2013-07-11 16:38:05 UTC

The attached patch fixes the related test cases in the libc++ test suite. Please review.

OK if the ifdefs work and the style bugs are fixed.

Post by David Chisnall
- Stop exposing isnan and isinf in the header. We already have __isinf in libc, so this is used instead.
- Call the static functions for isnan __inline__isnan*() so that they don't conflict with the ones in libm.
- Add an __fp_type_select() macro that uses either __Generic(), __builtin_choose_expr() / __builtin_choose_expr(), or sizeof() comparisons, depending on what the compiler supports.
- Refactor all of the type-generic macros to use __fp_type_select().

% Index: src/math.h
% ===================================================================
% --- src/math.h (revision 253148)
% +++ src/math.h (working copy)
% @@ -80,28 +80,39 @@
% #define FP_NORMAL 0x04
% #define FP_SUBNORMAL 0x08
% #define FP_ZERO 0x10
% +
% +#if __STDC_VERSION__ >= 201112L
% +#define __fp_type_select(x, f, d, ld) _Generic((x), \
% + float: f(x), \
% + double: d(x), \
% + long double: ld(x))

The normal formatting of this is unclear. Except for the tab after #define.
math.h has only 1 other instance of a space after #define.

% +#elif __GNUC_PREREQ__(5, 1)
% +#define __fp_type_select(x, f, d, ld) __builtin_choose_expr( \
% + __builtin_types_compatible_p(__typeof (x), long double), ld(x), \
% + __builtin_choose_expr( \
% + __builtin_types_compatible_p(__typeof (x), double), d(x), \
% + __builtin_choose_expr( \
% + __builtin_types_compatible_p(__typeof (x), float), f(x), (void)0)))

Extra space after __typeof.

Normal formatting doesn't march to the right like this...

% +#else
% +#define __fp_type_select(x, f, d, ld) \
% + ((sizeof (x) == sizeof (float)) ? f(x) \
% + : (sizeof (x) == sizeof (double)) ? d(x) \
% + : ld(x))

... or like this.

Extra space after sizeof (bug copied from old code).

% +#endif
% +
% +
% +

Extra blank lines.

% #define fpclassify(x) \
% - ((sizeof (x) == sizeof (float)) ? __fpclassifyf(x) \
% - : (sizeof (x) == sizeof (double)) ? __fpclassifyd(x) \
% - : __fpclassifyl(x))

Example of normal style in old code (except for the space after sizeof(),
and the backslashes aren't line up like they are in some other places in
this file).

% ...
% @@ -119,10 +130,8 @@
% #define isunordered(x, y) (isnan(x) || isnan(y))
% #endif /* __MATH_BUILTIN_RELOPS */
%
% -#define signbit(x) \
% - ((sizeof (x) == sizeof (float)) ? __signbitf(x) \
% - : (sizeof (x) == sizeof (double)) ? __signbit(x) \
% - : __signbitl(x))
% +#define signbit(x) \
% + __fp_type_select(x, __signbitf, __signbit, __signbitl)

The tab lossage is especially obvious here.

This macro definition fits on 1 line now. Similarly for others except
__inline_isnan*, which takes 2 lines. __inline_isnan* should be named
less verbosely, without __inline. I think this doesn't cause any
significant conflicts with libm. Might need __always_inline.
__fp_type_select is also verbose.

%
% typedef __double_t double_t;
% typedef __float_t float_t;
% @@ -175,6 +184,7 @@
% int __isfinite(double) __pure2;
% int __isfinitel(long double) __pure2;
% int __isinff(float) __pure2;
% +int __isinf(double) __pure2;
% int __isinfl(long double) __pure2;
% int __isnanf(float) __pure2;
% int __isnanl(long double) __pure2;
% @@ -185,6 +195,23 @@
% int __signbitf(float) __pure2;
% int __signbitl(long double) __pure2;

The declarations of old extern functions can probably be removed too
when they are replaced by inlines (only __isnan*() for now) . I think
the declarations of __isnan*() are now only used to prevent warnings
(at higher warning levels than have ever been used) in the file that
implement the functions.

%
% +static __inline int
% +__inline_isnanf(float __x)
% +{
% + return (__x != __x);
% +}
% +static __inline int
% +__inline_isnan(double __x)
% +{
% + return (__x != __x);
% +}
% +static __inline int
% +__inline_isnanl(long double __x)
% +{
% + return (__x != __x);
% +}
% +
% +

Extra blank lines.

Some insertion sort errors. In this file, APIs are mostly sorted in the
order double, float, long double.

All the inline functions except __inline_isnan*() only evaluate their
args once, so they can be simpler shorter and less namespace-polluting
as macros. catrig*.c uses the following macros for them (just showing
the double precision ones here):

@ #define isinf(x) (fabs(x) == INFINITY)
@ #define isnan(x) ((x) != (x))
@ #define signbit(x) (__builtin_signbit(x))

Note that that these are not quite independent of the precision, so
they don't all need multiple inline functions or macros. fabs() is
not type-generic, but __builtin_fabs() might be. fabsl() for all
precisions would work but be slower. __builtin_signbit() is even more
likely to be type-generic, like __builtin_isnan(). But catrig[fl].c
spell out the type suffixes for safety.

I hoped to copy these to math.h after fixing any technical problems.
isnan() needs a statement-expression to be safe. signbit() is more
of the problem than the others, since it is hard to do without a builtin
and the builtin doesn't exist in gcc-3.x. The builtin works fine with
-current gcc and clang, at least on x86 . So the committed version
just uses the builtin, and my local version uses libm internals that
are unsuitable for the public signbit() (mainly due to namespace
problems). The builtin might not work right in -curent for non-x86.
When the extern function is used, it uses similar libm internals, but
they take much longer when not inlined.

Bruce

Scot Hetzel

2013-07-12 16:16:46 UTC

The attached patch fixes the related test cases in the libc++ test suite. Please review.

#define fpclassify(x) \
- ((sizeof (x) == sizeof (float)) ? __fpclassifyf(x) \
- : (sizeof (x) == sizeof (double)) ? __fpclassifyd(x) \
- : __fpclassifyl(x))
+ __fp_type_select(x, __fpclassifyf, __fpclassifyd, __fpclassifyd)

The last __fpclassifyd should be __fpclassifyl.

--
DISCLAIMER:

No electrons were maimed while sending this message. Only slightly bruised.

Scot Hetzel

2013-07-12 17:13:58 UTC

The attached patch fixes the related test cases in the libc++ test suite. Please review.

I see it has already been fixed.

--
DISCLAIMER:

No electrons were maimed while sending this message. Only slightly bruised.

O. Hartmann

2013-07-12 21:47:49 UTC

On Fri, 12 Jul 2013 12:13:58 -0500

Post by Scot Hetzel

On Thu, Jul 11, 2013 at 9:33 AM, David Chisnall

Post by Bruce Evans
The error message for the __builtin_isnan() version is slightly
better up to where it says more.
The less-unportable macro can do more classification and detect
problems at compile time using __typeof().

The attached patch fixes the related test cases in the libc++ test
suite. Please review.

#define fpclassify(x) \
- ((sizeof (x) == sizeof (float)) ? __fpclassifyf(x) \
- : (sizeof (x) == sizeof (double)) ? __fpclassifyd(x) \
- : __fpclassifyl(x))
+ __fp_type_select(x, __fpclassifyf, __fpclassifyd,
__fpclassifyd)
The last __fpclassifyd should be __fpclassifyl.

I see it has already been fixed.

Obviously not really fixed, but even worse:

if I use in C code (C99, using clang 3.3 on FreeBSD 10.0-CURRENT/amd64
revision 253287) isnan(x) where x is a "const double", I receive now
the following error (which doesn't appear on previous versions):

[...]
Making all in scaling
/bin/sh ../../libtool --tag=CC --mode=compile cc -DHAVE_CONFIG_H -I.
-I../.. -I. -I../ -I/usr/local/include -O0 -march=native -g -pipe
-DHAVE_INLINE -g -O2 -MT libscaling_la-scalingTransientCroft.lo -MD -MP
-MF .deps/libscaling_la-scalingTransientCroft.Tpo -c -o
libscaling_la-scalingTransientCroft.lo `test -f
'scalingTransientCroft.c' || echo './'`scalingTransientCroft.c libtool:
compile: cc -DHAVE_CONFIG_H -I. -I../.. -I. -I../ -I/usr/local/include
-O0 -march=native -g -pipe -DHAVE_INLINE -g -O2 -MT
libscaling_la-scalingTransientCroft.lo -MD -MP
-MF .deps/libscaling_la-scalingTransientCroft.Tpo -c
scalingTransientCroft.c -o libscaling_la-scalingTransientCroft.o
scalingTransientCroft.c:48:12: error: controlling expression type
'const double' not compatible with any generic association type if
(isnan(Dsg) || isnan(Dsc)) ^~~ /usr/include/math.h:109:19: note:
expanded from macro 'isnan' __fp_type_select(x, __inline_isnanf,
__inline_isnan, __inline_isnanl) ^ /usr/include/math.h:86:49: note:
expanded from macro '__fp_type_select' #define __fp_type_select(x, f,
d, ld) _Generic((x),
[...]

The variables in question (Dsg and Dsc) are of type "const double".

Pasi Parviainen

2013-07-13 12:19:24 UTC