Re: [PATCH/2nd version] Re: nextafter() about an order of magnitude slower than trivial implementation

[Adhemerval Zanella <adhemerval.zanella@linaro.org>]

On 20/08/2021 17:19, Stefan Kanthak wrote:
> "Joseph Myers" <joseph@codesourcery.com> wrote:
> 
>> On Fri, 20 Aug 2021, Stefan Kanthak wrote:
>>
>>>         if(ax > 0x7ffull << 53)         /* x is nan */
>>>             return x;
>>>         if(ay > 0x7ffull << 53)         /* y is nan */
>>>             return y;
>>
>> How has this been tested?  I'd have expected it to fail the nextafter 
>> tests in the glibc testsuite (libm-test-nextafter.inc), because they 
>> verify that sNaN arguments produce a qNaN result with the "invalid" 
>> exception raised, and this looks like it would just return an sNaN 
>> argument unchanged.
> 
> It doesn't look like it would, it REALLY does!
> As I explicitly wrote, my changes avoid FP operations if possible.
> 
> <https://pubs.opengroup.org/onlinepubs/9699919799/functions/nextafter.html>
> 
> | If x or y is NaN, a NaN shall be returned.
> 
> I choose to return the argument NaN, what both POSIX and ISO C allow.
> If my mind serves me well, one of the former editions even stated
> "If an argument is NaN, this argument shall be returned". This may
> but be influenced/spoiled by
> <https://docs.microsoft.com/en-us/cpp/c-runtime-library/reference/nextafter-functions>
> 
> | If either x or y is a NAN, then the return value is one of the input NANs.
> 
> 
> If that bothers you, change these 4 lines to either
> 
>          if(ax > 0x7ffull << 53)         /* x is nan */
>              return 0.0 + x;
>          if(ay > 0x7ffull << 53)         /* y is nan */
>              return 0.0 + y;
> 
> (the addition has eventually to be guarded from optimisation) or
> 
>          if(ax > 0x7ffull << 53          /* x is nan */
>          || ay > 0x7ffull << 53)         /* y is nan */
>              return x + y;

Sorry, but sending half-baked patches where one will need to evaluate
and decide which is the best strategy is not the best way forward to
accept this change.

Please do follow the contributor checklist [1] and the suggestion I
gave you when you asked for input in the libc-help:

  * Check for regressions by testing your patch against glibc testsuite
    (make check).  As Joseph has put, your patch triggers a lot of
    regressions:

    $ grep ^FAIL math/subdir-tests.sum
    FAIL: math/bug-nextafter
    FAIL: math/test-double-nextafter
    FAIL: math/test-float32x-nextafter
    FAIL: math/test-float64-nextafter
    FAIL: math/test-misc

  * You removed the copyright since it is a new implementation, but
    the any code still requires a Copyright (even if is not assigned
    to FSF).  Check the item 3.4 on the Contributor checklist.

  * Also, since is a new implementation follow the usual style and
    code guidelines instead of using the current one used.

  * It would be good to add a benchmark to evaluate this change if
    the motivation for your change is performance.  Follow what
    other math benchmarks does on benchtests (exp2 or exp10 for
    instance).

  * Sending a patch with a define switch and stating "Choose whatever
    you like best" does not help in anything and put the burden or
    evaluating your change to the maintainers. 
 
[1] https://sourceware.org/glibc/wiki/Contribution%20checklist