[PATCH] Fix definition of INFINITY and add NAN/HUGE_VAL[F]

v2: use __builtin_inff() and also add nan/huge_val definitions

Signed-off-by: Aaron Watry <awatry@gmail.com>

Shouldn’t FLT_NAN/DBL_NAN be removed?

Jeroen

I thought that originally, but the comment in clc_nextafter.cl gave me pause:

// This file provides OpenCL C implementations of nextafter for targets that
// don't support the clang builtin.

I wasn't sure if that was just implying a lack of support for the
nextafter built-in, or which were also missing additional built-in
functions. Tom wrote this code, so if he's ok with using the nanf/nan
built-in functions, then I'm fine with that. I believe that the
commit message implies that this is related to whether libm is present
for the target, so that would seem to imply that we can fix this file
while we're here.

--Aaron

I thought that originally, but the comment in clc_nextafter.cl gave me pause:

// This file provides OpenCL C implementations of nextafter for targets that
// don't support the clang builtin.

I wasn't sure if that was just implying a lack of support for the
nextafter built-in, or which were also missing additional built-in
functions. Tom wrote this code, so if he's ok with using the nanf/nan
built-in functions, then I'm fine with that. I believe that the
commit message implies that this is related to whether libm is present
for the target, so that would seem to imply that we can fix this file
while we're here.

It should be safe to remove FLT_NAN and DBL_NAN. You are correct that
usually only the libm equivalent builtins are an issue, since the are
lowered to libcalls and R600 doesn't have libm.

-Tom

I’m getting an error when I try to use this. Apparently __builtin_nanf expects a string argument

4 errors generated.
95: divide FAILED -- clBuildProgram() failed: (-11)
Log: input.cl:18:40: error: too few arguments to function call, expected 1, have 0
               f0 = (float3)( in[3*i], NAN, NAN );
                                       ^~~
/usr/local/include/clc/float/definitions.h:2:28: note: expanded from macro 'NAN'
#define NAN __builtin_nanf()
            ~~~~~~~~~~~~~~ ^
input.cl:19:41: error: too few arguments to function call, expected 1, have 0
               f1 = (float3)( in2[3*i], NAN, NAN );
                                        ^~~
/usr/local/include/clc/float/definitions.h:2:28: note: expanded from macro 'NAN'
#define NAN __builtin_nanf()
            ~~~~~~~~~~~~~~ ^
input.cl:22:51: error: too few arguments to function call, expected 1, have 0
               f0 = (float3)( in[3*i], in[3*i+1], NAN );
                                                  ^~~
/usr/local/include/clc/float/definitions.h:2:28: note: expanded from macro 'NAN'
#define NAN __builtin_nanf()
            ~~~~~~~~~~~~~~ ^
input.cl:23:53: error: too few arguments to function call, expected 1, have 0
               f1 = (float3)( in2[3*i], in2[3*i+1], NAN );
                                                    ^~~
/usr/local/include/clc/float/definitions.h:2:28: note: expanded from macro 'NAN'
#define NAN __builtin_nanf()
            ~~~~~~~~~~~~~~ ^

This needs to be replaced with:

:#define NAN __builtin_nanf((const attribute((address_space(2))) char *)(""))

Hi Matt,

This needs to be replaced with:

:#define NAN __builtin_nanf((const attribute((address_space(2))) char *)("”))

This won’t work with the NVPTX target. It uses 4 as its constant address space and not 2 like r600.

Jeroen

It turns out this doesn’t really work either. This is what the OS X headers use, and the public version of clang this seems to be broken

Hi,

Changing the 2 to a 0 works for me with the NVPTX target:

#define NAN __builtin_nanf((const attribute((address_space(0))) char *)("”))

However, clang turns the builtin into a call into the C math library, which is not what we want…

Jeroen

Does anyone mind if I commit the infinify/huge_val[f] change now while
we continue discussing the NAN change? The infinity/huge_valf changes
are simple in comparison to the requirements of nan, and I've tested
that the __builtin_inff() and __builtin_huge_valf() implementations
work with the attached CL-based test (runnable via piglit).

--Aaron

float_specials.cl (849 Bytes)

Sure, I would even commit the broken __builtin_nanf("") for NAN. It’s better than nothing and it is a problem to fix in clang anyway

Fine with me too.

Regarding NAN. I saw that one of the header files made available by Khronos defines it as (INFINITY - INFINITY), would that work for us, or will that result in the same compiler warning that Matt pointed out before?

Jeroen

That gives me the same warning

Yea, I hit send on my last email before I copied in my 3 proposals:

//Option A
#define NAN __builtin_nanf("")
//Option B
#define NAN 0x7fc00000 //non-signalling positive NaN with no payload
//Option C:
#define NAN (INFINITY - INFINITY)

I'd prefer one of B or C, but I'll bow to any compelling arguments otherwise.

if you uncomment the nan test in the test case that was in my last
email, the 2nd two options produce a passing result. The 1st option [
__builtin_nanf("") ] results in a kernel compilation error with the
following:

Could not build program: CL_BUILD_PROGRAM_FAILURE
Build log for device AMD CEDAR: