[PATCH] math: Order signed zeros in fmin and fmax.

[Wilco Dijkstra]

Hi Adhemerval/James,

>> I believe the patch results in the desired behavior on all targets,
>> even though it modifies only the i386, i686, x86_64, and the generic C
>> implementation.

That's good to hear, it would be useful to mention something like that in the
description.

> As a side note, I don't think there is much gain is having the i386 version.
> If you a felling adventurous, maybe you can send a patch to just remove the
> i386 variants and making the i686 the default one.

I think the same is true for the x86_64 version, with my proposed improvement
below, it generates:

"__fmin":
        ucomisd xmm0, xmm1
        movapd  xmm2, xmm0
        jp      .L14
        je      .L29
        minsd   xmm2, xmm1
        movapd  xmm0, xmm2
        ret

This is from writing fmin/fmax like this:

  if (!isunordered (x, y))
  {
    if (__glibc_unlikely (x == y))
      return signbit (x) ? x : y;
    return x < y ? x : y;
  }

This has fewer operations and branches, and allows targets to use conditional moves
or min/max in both return cases.

The only remaining issue with the generic C implementation is that we don't yet
inline issignaling, so the NaN handling is slightly slower. If that is inlined then the
i686 assembler implementation could likely be removed too.

> The additional targets in glibc which don't use the generic C
> implementation are:
> - riscv
> - loongarch (via setting USE_F{MIN,MAX}{,F}_BUILTIN)
> - aarch64 (via setting USE_F{MIN,MAX}{,F}_BUILTIN)
>
> On these architectures, the operation is already implemented by a
> single hardware instruction with the desired behavior; no changes are
> required.

There are more, eg. power will emit "xsmindp" since power7 at least, but they don't
seem to define USE_F{MIN,MAX}{,F}_BUILTIN.

>> There are at least
>> 3 different variations of fmin/fmax in the IEEE standard, so I'm wondering whether
>> they do agree on how to handle signed zeroes... If it works on all modern targets,
>> that's great, but that's not clear from your description because ISAs typically
>> follow IEEE specs.
>
> Yes, the other variations of floating-point min/max are fminimum,
> fminimum_mag, fminimum_num, fminimum_mag_num (and their corresponding
> maximums). Those functions are required to order signed zeros by the C
> standard, unlike fmin/fmax where it is only recommended.

fminmag/fmaxmag currently don't order signed zeroes either:

FLOAT
M_DECL_FUNC (__fminmag) (FLOAT x, FLOAT y)
{
  FLOAT ax = M_FABS (x);
  FLOAT ay = M_FABS (y);
  if (isless (ax, ay))
    return x;
  else if (isgreater (ax, ay))
    return y;
  else if (ax == ay)
    return x < y ? x : y;
  else if (issignaling (x) || issignaling (y))
    return x + y;
  else
    return isnan (y) ? x : y;
}

This one does do it, but inefficiently (there is also different NaN handling...):

FLOAT
M_DECL_FUNC (__fminimum_mag_num) (FLOAT x, FLOAT y)
{
  FLOAT ax = M_FABS (x);
  FLOAT ay = M_FABS (y);
  if (isless (ax, ay))
    return x;
  else if (isgreater (ax, ay))
    return y;
  else if (ax == ay)
    return (M_COPYSIGN (1, x) <= M_COPYSIGN (1, y) ? x : y);
  else
    return isnan (y) ? (isnan (x) ? x + y : x) : y;
}

So it would be great to make them consistent for signed zero and use the same efficient
sequence in all cases.

Cheers,
Wilco