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

On 11 Jul 2013, at 13:11, Bruce Evans <brde_at_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.  

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

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

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

>> 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()?

Yes.

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

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.

>> 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);
>>                    ^
> 
> 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) = 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