test_fabs fails on arm64

[punitagrawal]

The pocl debian package is failing to build due to test_fabs failing on arm64. Following is the result of my findings when investigating the failure.

test_fabs checks for correctness of a number of math related functions. Of the ones tested ilogb and ldexp fail on arm64.

ilogb

ilogb(x) returns

FP_ILOGB0 when x = 0, and
FP_ILOGBNAN when x = NaN

According to OpenCL 1.2, the valid values for FP_ILOGB0 are either INT_MIN or -INT_MAX (note the '-' sign). Similarly the valid value for FP_ILOGBNAN are either INT_MIN or INT_MAX. ISO C has a similar specification of ilogb() so it's not that unusual to have two differing return values.

Looking at glibc, it looks like the choice of value for FP_ILOGB0 and FP_ILOGBNAN is architecture dependent, e.g., FP_ILOGB0 is INT_MIN on x86 but -INT_MAX on arm64.

The check in "test_fabs" fails because the value returned by the underlying implementation of ilogb() doesn't match on 0 and NAN. The implementation of ilogb(0) returns -INT_MAX while FP_ILOGB0 is defined as INT_MIN in pocl. Similarly, ilogb(NaN) returns INT_MAX while FP_ILOGBNAN is defined as INT_MIN in pocl.

This is my first poke at pocl and could be missing something obvious. AIUI, on arm64 sleef provides the implementation of ilogb() (via Sleef_ilogb()) but I couldn't find an obvious way to fix the failure.

Hints/suggestion welcome!

[pjaaskel]

Sorry, but we lack an Arm maintainer. Interested?

[franz]

arm64 sleef provides the implementation of ilogb() (via Sleef_ilogb()) but I couldn't find an obvious way to fix the failure

Well, from your description, it's not the ilogb() that needs fixing, but rather the test_fabs to accept both values as valid ?

[punitagrawal]

Well, from your description, it's not the ilogb() that needs fixing, but rather the test_fabs to accept both values as valid ?

Conceptually yes, but I don't think changing the test is the right fix. test_fabs doesn't explicitly use INT_MIN or -INT_MAX, it uses FP_ILOGB[0|NAN] which is correct as per the spec. See quoted code below.

     /* ilogb */
     Ivec ires;
     ires.v = ilogb(val.v);
     Ivec igoodres;
     equal = true;
     for (int n=0; n<vecsize; ++n) {
       if (ISNAN(val.s[n])) {
         igoodres.s[n] = FP_ILOGBNAN;
       } else if (val.s[n] == (stype)0) {
         igoodres.s[n] = FP_ILOGB0;
       } else if (isinf(val.s[n])) {
         igoodres.s[n] = INT_MAX;
       } else {
         // We round down to "correct" for inaccuracies in log2
         // We divide by 2 since log2 is wrong for large inputs
         igoodres.s[n] = 1+rint(floor(0.999999f*log2(0.5*fabs(val.s[n]))));
       }
       equal = equal && ires.s[n] == igoodres.s[n];
     }

The difference is in the value of FP_ILOGB[0|NAN] between pocl and the ilogb() implementation. Once we align the values the test should pass without any changes.

[punitagrawal]

Sorry, but we lack an Arm maintainer. Interested?

@pjaaskel - sounds interesting but can you elaborate on what you're expecting - both in terms of time and responsibilities? I'll be doing this in my free time and will take sometime to become familiar with pocl (but also OpenCL in general).

[franz]

OK, i think i see the problem. SLEEF is using math.h which defines FP_ILOGB[0|NAN] to different values on different platforms. I think something like this should make it consistent, unfortunately my ARM64 box is acting up so i cannot test it.

[punitagrawal]

The ilogb() test is a lot happier with your change.

Changes to misc.h aligns the values of FP_ILOGB[0|NAN]. The ilogb() tests pass with this change.

For the changes to _kernel.h, I got lost trying to understand which of the various #ifdefs apply. So can't confirm if it's really needed.

Now to understand why the other bits of test_fabs fail.

Aside - I think it might be worth splitting up the multiple math function checks into separate tests of their own. It'll help identify quicker when something breaks

[punitagrawal]

Looks like I spoke too soon in the above comment. On further testing I've found that the fix works with all lengths of vector of floats but doesn't work with a vector of doubles with more than one element.