diff mbox series

[v2,03/10] i386: Use generic fmod

Message ID 20240327194024.1409677-4-adhemerval.zanella@linaro.org
State New
Headers show
Series Fix some libm static issues | expand

Commit Message

Adhemerval Zanella Netto March 27, 2024, 7:40 p.m. UTC 
The benchtest results shows a slight improvement (Ryzen 5900, gcc
13.2.1):

* sysdeps/i386/fpu/e_fmod.S:
  "fmod": {
   "subnormals": {
    "duration": 3.68855e+09,
    "iterations": 2.12608e+08,
    "max": 62.012,
    "min": 16.798,
    "mean": 17.349
   },
   "normal": {
    "duration": 3.88459e+09,
    "iterations": 7.168e+06,
    "max": 2879.12,
    "min": 16.909,
    "mean": 541.934
   },
   "close-exponents": {
    "duration": 3.692e+09,
    "iterations": 1.96608e+08,
    "max": 66.452,
    "min": 16.835,
    "mean": 18.7785
   }
  }

* generic
  "fmod": {
   "subnormals": {
    "duration": 3.68645e+09,
    "iterations": 2.2848e+08,
    "max": 66.896,
    "min": 15.91,
    "mean": 16.1347
   },
   "normal": {
    "duration": 4.1455e+09,
    "iterations": 8.192e+06,
    "max": 3376.18,
    "min": 15.873,
    "mean": 506.043
   },
   "close-exponents": {
    "duration": 3.70197e+09,
    "iterations": 2.08896e+08,
    "max": 69.597,
    "min": 15.947,
    "mean": 17.7216
   }
  }
---
 sysdeps/i386/fpu/Versions                 |  4 ++++
 sysdeps/i386/fpu/e_fmod.S                 | 18 ------------------
 sysdeps/i386/fpu/e_fmod.c                 |  2 ++
 sysdeps/i386/fpu/math_err.c               |  1 -
 sysdeps/i386/fpu/w_fmod_compat.c          | 15 ---------------
 sysdeps/ieee754/dbl-64/e_fmod.c           |  5 ++++-
 sysdeps/mach/hurd/i386/libm.abilist       |  1 +
 sysdeps/unix/sysv/linux/i386/libm.abilist |  1 +
 8 files changed, 12 insertions(+), 35 deletions(-)
 delete mode 100644 sysdeps/i386/fpu/e_fmod.S
 create mode 100644 sysdeps/i386/fpu/e_fmod.c
 delete mode 100644 sysdeps/i386/fpu/math_err.c
 delete mode 100644 sysdeps/i386/fpu/w_fmod_compat.c

Comments

H.J. Lu March 27, 2024, 7:55 p.m. UTC | #1 
On Wed, Mar 27, 2024 at 12:40 PM Adhemerval Zanella
<adhemerval.zanella@linaro.org> wrote:
>
> The benchtest results shows a slight improvement (Ryzen 5900, gcc
> 13.2.1):
>
> * sysdeps/i386/fpu/e_fmod.S:
>   "fmod": {
>    "subnormals": {
>     "duration": 3.68855e+09,
>     "iterations": 2.12608e+08,
>     "max": 62.012,
>     "min": 16.798,
>     "mean": 17.349
>    },
>    "normal": {
>     "duration": 3.88459e+09,
>     "iterations": 7.168e+06,
>     "max": 2879.12,
>     "min": 16.909,
>     "mean": 541.934
>    },
>    "close-exponents": {
>     "duration": 3.692e+09,
>     "iterations": 1.96608e+08,
>     "max": 66.452,
>     "min": 16.835,
>     "mean": 18.7785
>    }
>   }
>
> * generic
>   "fmod": {
>    "subnormals": {
>     "duration": 3.68645e+09,
>     "iterations": 2.2848e+08,
>     "max": 66.896,
>     "min": 15.91,
>     "mean": 16.1347
>    },
>    "normal": {
>     "duration": 4.1455e+09,
>     "iterations": 8.192e+06,
>     "max": 3376.18,
>     "min": 15.873,
>     "mean": 506.043
>    },
>    "close-exponents": {
>     "duration": 3.70197e+09,
>     "iterations": 2.08896e+08,
>     "max": 69.597,
>     "min": 15.947,
>     "mean": 17.7216
>    }
>   }
> ---
>  sysdeps/i386/fpu/Versions                 |  4 ++++
>  sysdeps/i386/fpu/e_fmod.S                 | 18 ------------------
>  sysdeps/i386/fpu/e_fmod.c                 |  2 ++
>  sysdeps/i386/fpu/math_err.c               |  1 -
>  sysdeps/i386/fpu/w_fmod_compat.c          | 15 ---------------
>  sysdeps/ieee754/dbl-64/e_fmod.c           |  5 ++++-
>  sysdeps/mach/hurd/i386/libm.abilist       |  1 +
>  sysdeps/unix/sysv/linux/i386/libm.abilist |  1 +
>  8 files changed, 12 insertions(+), 35 deletions(-)
>  delete mode 100644 sysdeps/i386/fpu/e_fmod.S
>  create mode 100644 sysdeps/i386/fpu/e_fmod.c
>  delete mode 100644 sysdeps/i386/fpu/math_err.c
>  delete mode 100644 sysdeps/i386/fpu/w_fmod_compat.c
>
> diff --git a/sysdeps/i386/fpu/Versions b/sysdeps/i386/fpu/Versions
> index a2eec371f1..d37bc1eae6 100644
> --- a/sysdeps/i386/fpu/Versions
> +++ b/sysdeps/i386/fpu/Versions
> @@ -3,4 +3,8 @@ libm {
>      # functions used in inline functions or macros
>      __expl; __expm1l;
>    }
> +  GLIBC_2.40 {
> +    # No SVID compatible error handling.
> +    fmod;
> +  }

This changes the ABI.  I assume that it fixes a real bug.   Is there a bug
report open for this?

>  }
> diff --git a/sysdeps/i386/fpu/e_fmod.S b/sysdeps/i386/fpu/e_fmod.S
> deleted file mode 100644
> index 86ac1bcfaf..0000000000
> --- a/sysdeps/i386/fpu/e_fmod.S
> +++ /dev/null
> @@ -1,18 +0,0 @@
> -/*
> - * Public domain.
> - */
> -
> -#include <machine/asm.h>
> -#include <libm-alias-finite.h>
> -
> -ENTRY(__ieee754_fmod)
> -       fldl    12(%esp)
> -       fldl    4(%esp)
> -1:     fprem
> -       fstsw   %ax
> -       sahf
> -       jp      1b
> -       fstp    %st(1)
> -       ret
> -END (__ieee754_fmod)
> -libm_alias_finite (__ieee754_fmod, __fmod)
> diff --git a/sysdeps/i386/fpu/e_fmod.c b/sysdeps/i386/fpu/e_fmod.c
> new file mode 100644
> index 0000000000..3625758f97
> --- /dev/null
> +++ b/sysdeps/i386/fpu/e_fmod.c
> @@ -0,0 +1,2 @@
> +#define FMOD_VERSION GLIBC_2_40
> +#include <sysdeps/ieee754/dbl-64/e_fmod.c>
> diff --git a/sysdeps/i386/fpu/math_err.c b/sysdeps/i386/fpu/math_err.c
> deleted file mode 100644
> index 1cc8931700..0000000000
> --- a/sysdeps/i386/fpu/math_err.c
> +++ /dev/null
> @@ -1 +0,0 @@
> -/* Not needed.  */
> diff --git a/sysdeps/i386/fpu/w_fmod_compat.c b/sysdeps/i386/fpu/w_fmod_compat.c
> deleted file mode 100644
> index 528bfc2a13..0000000000
> --- a/sysdeps/i386/fpu/w_fmod_compat.c
> +++ /dev/null
> @@ -1,15 +0,0 @@
> -/* i386 provides an optimized __ieee752_fmod.  */
> -#include <math-svid-compat.h>
> -#ifdef SHARED
> -# undef SHLIB_COMPAT
> -# define SHLIB_COMPAT(a, b, c) 1
> -# undef LIBM_SVID_COMPAT
> -# define LIBM_SVID_COMPAT 1
> -# undef compat_symbol
> -# define compat_symbol(a, b, c, d)
> -# include <math/w_fmod_compat.c>
> -libm_alias_double (__fmod_compat, fmod)
> -#else
> -#include <math-type-macros-double.h>
> -#include <w_fmod_template.c>
> -#endif
> diff --git a/sysdeps/ieee754/dbl-64/e_fmod.c b/sysdeps/ieee754/dbl-64/e_fmod.c
> index b33cfb1223..7651cd212a 100644
> --- a/sysdeps/ieee754/dbl-64/e_fmod.c
> +++ b/sysdeps/ieee754/dbl-64/e_fmod.c
> @@ -175,7 +175,10 @@ __fmod (double x, double y)
>  strong_alias (__fmod, __ieee754_fmod)
>  libm_alias_finite (__ieee754_fmod, __fmod)
>  #if LIBM_SVID_COMPAT
> -versioned_symbol (libm, __fmod, fmod, GLIBC_2_38);
> +# ifndef FMOD_VERSION
> +#  define FMOD_VERSION GLIBC_2_38
> +# endif
> +versioned_symbol (libm, __fmod, fmod, FMOD_VERSION);
>  libm_alias_double_other (__fmod, fmod)
>  #else
>  libm_alias_double (__fmod, fmod)
> diff --git a/sysdeps/mach/hurd/i386/libm.abilist b/sysdeps/mach/hurd/i386/libm.abilist
> index 8f40ddb150..30665f8b1a 100644
> --- a/sysdeps/mach/hurd/i386/libm.abilist
> +++ b/sysdeps/mach/hurd/i386/libm.abilist
> @@ -1181,3 +1181,4 @@ GLIBC_2.35 fsqrt F
>  GLIBC_2.35 fsqrtl F
>  GLIBC_2.35 hypot F
>  GLIBC_2.35 hypotf F
> +GLIBC_2.40 fmod F
> diff --git a/sysdeps/unix/sysv/linux/i386/libm.abilist b/sysdeps/unix/sysv/linux/i386/libm.abilist
> index 5d89aaa08e..44932f111d 100644
> --- a/sysdeps/unix/sysv/linux/i386/libm.abilist
> +++ b/sysdeps/unix/sysv/linux/i386/libm.abilist
> @@ -1188,3 +1188,4 @@ GLIBC_2.35 fsqrt F
>  GLIBC_2.35 fsqrtl F
>  GLIBC_2.35 hypot F
>  GLIBC_2.35 hypotf F
> +GLIBC_2.40 fmod F
> --
> 2.34.1
>
Adhemerval Zanella Netto March 27, 2024, 8:37 p.m. UTC | #2 
On 27/03/24 16:55, H.J. Lu wrote:
> On Wed, Mar 27, 2024 at 12:40 PM Adhemerval Zanella
> <adhemerval.zanella@linaro.org> wrote:
>>
>> The benchtest results shows a slight improvement (Ryzen 5900, gcc
>> 13.2.1):
>>
>> * sysdeps/i386/fpu/e_fmod.S:
>>   "fmod": {
>>    "subnormals": {
>>     "duration": 3.68855e+09,
>>     "iterations": 2.12608e+08,
>>     "max": 62.012,
>>     "min": 16.798,
>>     "mean": 17.349
>>    },
>>    "normal": {
>>     "duration": 3.88459e+09,
>>     "iterations": 7.168e+06,
>>     "max": 2879.12,
>>     "min": 16.909,
>>     "mean": 541.934
>>    },
>>    "close-exponents": {
>>     "duration": 3.692e+09,
>>     "iterations": 1.96608e+08,
>>     "max": 66.452,
>>     "min": 16.835,
>>     "mean": 18.7785
>>    }
>>   }
>>
>> * generic
>>   "fmod": {
>>    "subnormals": {
>>     "duration": 3.68645e+09,
>>     "iterations": 2.2848e+08,
>>     "max": 66.896,
>>     "min": 15.91,
>>     "mean": 16.1347
>>    },
>>    "normal": {
>>     "duration": 4.1455e+09,
>>     "iterations": 8.192e+06,
>>     "max": 3376.18,
>>     "min": 15.873,
>>     "mean": 506.043
>>    },
>>    "close-exponents": {
>>     "duration": 3.70197e+09,
>>     "iterations": 2.08896e+08,
>>     "max": 69.597,
>>     "min": 15.947,
>>     "mean": 17.7216
>>    }
>>   }
>> ---
>>  sysdeps/i386/fpu/Versions                 |  4 ++++
>>  sysdeps/i386/fpu/e_fmod.S                 | 18 ------------------
>>  sysdeps/i386/fpu/e_fmod.c                 |  2 ++
>>  sysdeps/i386/fpu/math_err.c               |  1 -
>>  sysdeps/i386/fpu/w_fmod_compat.c          | 15 ---------------
>>  sysdeps/ieee754/dbl-64/e_fmod.c           |  5 ++++-
>>  sysdeps/mach/hurd/i386/libm.abilist       |  1 +
>>  sysdeps/unix/sysv/linux/i386/libm.abilist |  1 +
>>  8 files changed, 12 insertions(+), 35 deletions(-)
>>  delete mode 100644 sysdeps/i386/fpu/e_fmod.S
>>  create mode 100644 sysdeps/i386/fpu/e_fmod.c
>>  delete mode 100644 sysdeps/i386/fpu/math_err.c
>>  delete mode 100644 sysdeps/i386/fpu/w_fmod_compat.c
>>
>> diff --git a/sysdeps/i386/fpu/Versions b/sysdeps/i386/fpu/Versions
>> index a2eec371f1..d37bc1eae6 100644
>> --- a/sysdeps/i386/fpu/Versions
>> +++ b/sysdeps/i386/fpu/Versions
>> @@ -3,4 +3,8 @@ libm {
>>      # functions used in inline functions or macros
>>      __expl; __expm1l;
>>    }
>> +  GLIBC_2.40 {
>> +    # No SVID compatible error handling.
>> +    fmod;
>> +  }
> 
> This changes the ABI.  I assume that it fixes a real bug.   Is there a bug
> report open for this?
> 

The new version is the way to provide the system without the SVID compat
support, which we for all ABIs but i386 on 2.38. For instance:

find . -iname libm.abilist | xargs grep -w fmod
./sysdeps/unix/sysv/linux/sparc/sparc32/libm.abilist:GLIBC_2.0 fmod F
./sysdeps/unix/sysv/linux/sparc/sparc32/libm.abilist:GLIBC_2.38 fmod F
[...]

For i386 specifically, the old SVID symbol will be kept as fmod@GLIBC_2.0.

>>  }
>> diff --git a/sysdeps/i386/fpu/e_fmod.S b/sysdeps/i386/fpu/e_fmod.S
>> deleted file mode 100644
>> index 86ac1bcfaf..0000000000
>> --- a/sysdeps/i386/fpu/e_fmod.S
>> +++ /dev/null
>> @@ -1,18 +0,0 @@
>> -/*
>> - * Public domain.
>> - */
>> -
>> -#include <machine/asm.h>
>> -#include <libm-alias-finite.h>
>> -
>> -ENTRY(__ieee754_fmod)
>> -       fldl    12(%esp)
>> -       fldl    4(%esp)
>> -1:     fprem
>> -       fstsw   %ax
>> -       sahf
>> -       jp      1b
>> -       fstp    %st(1)
>> -       ret
>> -END (__ieee754_fmod)
>> -libm_alias_finite (__ieee754_fmod, __fmod)
>> diff --git a/sysdeps/i386/fpu/e_fmod.c b/sysdeps/i386/fpu/e_fmod.c
>> new file mode 100644
>> index 0000000000..3625758f97
>> --- /dev/null
>> +++ b/sysdeps/i386/fpu/e_fmod.c
>> @@ -0,0 +1,2 @@
>> +#define FMOD_VERSION GLIBC_2_40
>> +#include <sysdeps/ieee754/dbl-64/e_fmod.c>
>> diff --git a/sysdeps/i386/fpu/math_err.c b/sysdeps/i386/fpu/math_err.c
>> deleted file mode 100644
>> index 1cc8931700..0000000000
>> --- a/sysdeps/i386/fpu/math_err.c
>> +++ /dev/null
>> @@ -1 +0,0 @@
>> -/* Not needed.  */
>> diff --git a/sysdeps/i386/fpu/w_fmod_compat.c b/sysdeps/i386/fpu/w_fmod_compat.c
>> deleted file mode 100644
>> index 528bfc2a13..0000000000
>> --- a/sysdeps/i386/fpu/w_fmod_compat.c
>> +++ /dev/null
>> @@ -1,15 +0,0 @@
>> -/* i386 provides an optimized __ieee752_fmod.  */
>> -#include <math-svid-compat.h>
>> -#ifdef SHARED
>> -# undef SHLIB_COMPAT
>> -# define SHLIB_COMPAT(a, b, c) 1
>> -# undef LIBM_SVID_COMPAT
>> -# define LIBM_SVID_COMPAT 1
>> -# undef compat_symbol
>> -# define compat_symbol(a, b, c, d)
>> -# include <math/w_fmod_compat.c>
>> -libm_alias_double (__fmod_compat, fmod)
>> -#else
>> -#include <math-type-macros-double.h>
>> -#include <w_fmod_template.c>
>> -#endif
>> diff --git a/sysdeps/ieee754/dbl-64/e_fmod.c b/sysdeps/ieee754/dbl-64/e_fmod.c
>> index b33cfb1223..7651cd212a 100644
>> --- a/sysdeps/ieee754/dbl-64/e_fmod.c
>> +++ b/sysdeps/ieee754/dbl-64/e_fmod.c
>> @@ -175,7 +175,10 @@ __fmod (double x, double y)
>>  strong_alias (__fmod, __ieee754_fmod)
>>  libm_alias_finite (__ieee754_fmod, __fmod)
>>  #if LIBM_SVID_COMPAT
>> -versioned_symbol (libm, __fmod, fmod, GLIBC_2_38);
>> +# ifndef FMOD_VERSION
>> +#  define FMOD_VERSION GLIBC_2_38
>> +# endif
>> +versioned_symbol (libm, __fmod, fmod, FMOD_VERSION);
>>  libm_alias_double_other (__fmod, fmod)
>>  #else
>>  libm_alias_double (__fmod, fmod)
>> diff --git a/sysdeps/mach/hurd/i386/libm.abilist b/sysdeps/mach/hurd/i386/libm.abilist
>> index 8f40ddb150..30665f8b1a 100644
>> --- a/sysdeps/mach/hurd/i386/libm.abilist
>> +++ b/sysdeps/mach/hurd/i386/libm.abilist
>> @@ -1181,3 +1181,4 @@ GLIBC_2.35 fsqrt F
>>  GLIBC_2.35 fsqrtl F
>>  GLIBC_2.35 hypot F
>>  GLIBC_2.35 hypotf F
>> +GLIBC_2.40 fmod F
>> diff --git a/sysdeps/unix/sysv/linux/i386/libm.abilist b/sysdeps/unix/sysv/linux/i386/libm.abilist
>> index 5d89aaa08e..44932f111d 100644
>> --- a/sysdeps/unix/sysv/linux/i386/libm.abilist
>> +++ b/sysdeps/unix/sysv/linux/i386/libm.abilist
>> @@ -1188,3 +1188,4 @@ GLIBC_2.35 fsqrt F
>>  GLIBC_2.35 fsqrtl F
>>  GLIBC_2.35 hypot F
>>  GLIBC_2.35 hypotf F
>> +GLIBC_2.40 fmod F
>> --
>> 2.34.1
>>
> 
>
H.J. Lu March 27, 2024, 9:38 p.m. UTC | #3 
On Wed, Mar 27, 2024 at 1:37 PM Adhemerval Zanella Netto
<adhemerval.zanella@linaro.org> wrote:
>
>
>
> On 27/03/24 16:55, H.J. Lu wrote:
> > On Wed, Mar 27, 2024 at 12:40 PM Adhemerval Zanella
> > <adhemerval.zanella@linaro.org> wrote:
> >>
> >> The benchtest results shows a slight improvement (Ryzen 5900, gcc
> >> 13.2.1):
> >>
> >> * sysdeps/i386/fpu/e_fmod.S:
> >>   "fmod": {
> >>    "subnormals": {
> >>     "duration": 3.68855e+09,
> >>     "iterations": 2.12608e+08,
> >>     "max": 62.012,
> >>     "min": 16.798,
> >>     "mean": 17.349
> >>    },
> >>    "normal": {
> >>     "duration": 3.88459e+09,
> >>     "iterations": 7.168e+06,
> >>     "max": 2879.12,
> >>     "min": 16.909,
> >>     "mean": 541.934
> >>    },
> >>    "close-exponents": {
> >>     "duration": 3.692e+09,
> >>     "iterations": 1.96608e+08,
> >>     "max": 66.452,
> >>     "min": 16.835,
> >>     "mean": 18.7785
> >>    }
> >>   }
> >>
> >> * generic
> >>   "fmod": {
> >>    "subnormals": {
> >>     "duration": 3.68645e+09,
> >>     "iterations": 2.2848e+08,
> >>     "max": 66.896,
> >>     "min": 15.91,
> >>     "mean": 16.1347
> >>    },
> >>    "normal": {
> >>     "duration": 4.1455e+09,
> >>     "iterations": 8.192e+06,
> >>     "max": 3376.18,
> >>     "min": 15.873,
> >>     "mean": 506.043
> >>    },
> >>    "close-exponents": {
> >>     "duration": 3.70197e+09,
> >>     "iterations": 2.08896e+08,
> >>     "max": 69.597,
> >>     "min": 15.947,
> >>     "mean": 17.7216
> >>    }
> >>   }
> >> ---
> >>  sysdeps/i386/fpu/Versions                 |  4 ++++
> >>  sysdeps/i386/fpu/e_fmod.S                 | 18 ------------------
> >>  sysdeps/i386/fpu/e_fmod.c                 |  2 ++
> >>  sysdeps/i386/fpu/math_err.c               |  1 -
> >>  sysdeps/i386/fpu/w_fmod_compat.c          | 15 ---------------
> >>  sysdeps/ieee754/dbl-64/e_fmod.c           |  5 ++++-
> >>  sysdeps/mach/hurd/i386/libm.abilist       |  1 +
> >>  sysdeps/unix/sysv/linux/i386/libm.abilist |  1 +
> >>  8 files changed, 12 insertions(+), 35 deletions(-)
> >>  delete mode 100644 sysdeps/i386/fpu/e_fmod.S
> >>  create mode 100644 sysdeps/i386/fpu/e_fmod.c
> >>  delete mode 100644 sysdeps/i386/fpu/math_err.c
> >>  delete mode 100644 sysdeps/i386/fpu/w_fmod_compat.c
> >>
> >> diff --git a/sysdeps/i386/fpu/Versions b/sysdeps/i386/fpu/Versions
> >> index a2eec371f1..d37bc1eae6 100644
> >> --- a/sysdeps/i386/fpu/Versions
> >> +++ b/sysdeps/i386/fpu/Versions
> >> @@ -3,4 +3,8 @@ libm {
> >>      # functions used in inline functions or macros
> >>      __expl; __expm1l;
> >>    }
> >> +  GLIBC_2.40 {
> >> +    # No SVID compatible error handling.
> >> +    fmod;
> >> +  }
> >
> > This changes the ABI.  I assume that it fixes a real bug.   Is there a bug
> > report open for this?
> >
>
> The new version is the way to provide the system without the SVID compat
> support, which we for all ABIs but i386 on 2.38. For instance:
>
> find . -iname libm.abilist | xargs grep -w fmod
> ./sysdeps/unix/sysv/linux/sparc/sparc32/libm.abilist:GLIBC_2.0 fmod F
> ./sysdeps/unix/sysv/linux/sparc/sparc32/libm.abilist:GLIBC_2.38 fmod F
> [...]
>
> For i386 specifically, the old SVID symbol will be kept as fmod@GLIBC_2.0.
>

Does it fix a run-time test which fails without the fix?
Adhemerval Zanella Netto March 28, 2024, 2:11 p.m. UTC | #4 
On 27/03/24 18:38, H.J. Lu wrote:
> On Wed, Mar 27, 2024 at 1:37 PM Adhemerval Zanella Netto
> <adhemerval.zanella@linaro.org> wrote:
>>
>>
>>
>> On 27/03/24 16:55, H.J. Lu wrote:
>>> On Wed, Mar 27, 2024 at 12:40 PM Adhemerval Zanella
>>> <adhemerval.zanella@linaro.org> wrote:
>>>>
>>>> The benchtest results shows a slight improvement (Ryzen 5900, gcc
>>>> 13.2.1):
>>>>
>>>> * sysdeps/i386/fpu/e_fmod.S:
>>>>   "fmod": {
>>>>    "subnormals": {
>>>>     "duration": 3.68855e+09,
>>>>     "iterations": 2.12608e+08,
>>>>     "max": 62.012,
>>>>     "min": 16.798,
>>>>     "mean": 17.349
>>>>    },
>>>>    "normal": {
>>>>     "duration": 3.88459e+09,
>>>>     "iterations": 7.168e+06,
>>>>     "max": 2879.12,
>>>>     "min": 16.909,
>>>>     "mean": 541.934
>>>>    },
>>>>    "close-exponents": {
>>>>     "duration": 3.692e+09,
>>>>     "iterations": 1.96608e+08,
>>>>     "max": 66.452,
>>>>     "min": 16.835,
>>>>     "mean": 18.7785
>>>>    }
>>>>   }
>>>>
>>>> * generic
>>>>   "fmod": {
>>>>    "subnormals": {
>>>>     "duration": 3.68645e+09,
>>>>     "iterations": 2.2848e+08,
>>>>     "max": 66.896,
>>>>     "min": 15.91,
>>>>     "mean": 16.1347
>>>>    },
>>>>    "normal": {
>>>>     "duration": 4.1455e+09,
>>>>     "iterations": 8.192e+06,
>>>>     "max": 3376.18,
>>>>     "min": 15.873,
>>>>     "mean": 506.043
>>>>    },
>>>>    "close-exponents": {
>>>>     "duration": 3.70197e+09,
>>>>     "iterations": 2.08896e+08,
>>>>     "max": 69.597,
>>>>     "min": 15.947,
>>>>     "mean": 17.7216
>>>>    }
>>>>   }
>>>> ---
>>>>  sysdeps/i386/fpu/Versions                 |  4 ++++
>>>>  sysdeps/i386/fpu/e_fmod.S                 | 18 ------------------
>>>>  sysdeps/i386/fpu/e_fmod.c                 |  2 ++
>>>>  sysdeps/i386/fpu/math_err.c               |  1 -
>>>>  sysdeps/i386/fpu/w_fmod_compat.c          | 15 ---------------
>>>>  sysdeps/ieee754/dbl-64/e_fmod.c           |  5 ++++-
>>>>  sysdeps/mach/hurd/i386/libm.abilist       |  1 +
>>>>  sysdeps/unix/sysv/linux/i386/libm.abilist |  1 +
>>>>  8 files changed, 12 insertions(+), 35 deletions(-)
>>>>  delete mode 100644 sysdeps/i386/fpu/e_fmod.S
>>>>  create mode 100644 sysdeps/i386/fpu/e_fmod.c
>>>>  delete mode 100644 sysdeps/i386/fpu/math_err.c
>>>>  delete mode 100644 sysdeps/i386/fpu/w_fmod_compat.c
>>>>
>>>> diff --git a/sysdeps/i386/fpu/Versions b/sysdeps/i386/fpu/Versions
>>>> index a2eec371f1..d37bc1eae6 100644
>>>> --- a/sysdeps/i386/fpu/Versions
>>>> +++ b/sysdeps/i386/fpu/Versions
>>>> @@ -3,4 +3,8 @@ libm {
>>>>      # functions used in inline functions or macros
>>>>      __expl; __expm1l;
>>>>    }
>>>> +  GLIBC_2.40 {
>>>> +    # No SVID compatible error handling.
>>>> +    fmod;
>>>> +  }
>>>
>>> This changes the ABI.  I assume that it fixes a real bug.   Is there a bug
>>> report open for this?
>>>
>>
>> The new version is the way to provide the system without the SVID compat
>> support, which we for all ABIs but i386 on 2.38. For instance:
>>
>> find . -iname libm.abilist | xargs grep -w fmod
>> ./sysdeps/unix/sysv/linux/sparc/sparc32/libm.abilist:GLIBC_2.0 fmod F
>> ./sysdeps/unix/sysv/linux/sparc/sparc32/libm.abilist:GLIBC_2.38 fmod F
>> [...]
>>
>> For i386 specifically, the old SVID symbol will be kept as fmod@GLIBC_2.0.
>>
> 
> Does it fix a run-time test which fails without the fix?
> 

Not really, but it is one less assembly implementation in favor a generic one
(which also shows a slight improvement on recent chips) and it sync i386
with generic code (so less possible issues, such as the static lib in this
patchset).
H.J. Lu March 28, 2024, 2:51 p.m. UTC | #5 
On Thu, Mar 28, 2024 at 7:11 AM Adhemerval Zanella Netto
<adhemerval.zanella@linaro.org> wrote:
>
>
>
> On 27/03/24 18:38, H.J. Lu wrote:
> > On Wed, Mar 27, 2024 at 1:37 PM Adhemerval Zanella Netto
> > <adhemerval.zanella@linaro.org> wrote:
> >>
> >>
> >>
> >> On 27/03/24 16:55, H.J. Lu wrote:
> >>> On Wed, Mar 27, 2024 at 12:40 PM Adhemerval Zanella
> >>> <adhemerval.zanella@linaro.org> wrote:
> >>>>
> >>>> The benchtest results shows a slight improvement (Ryzen 5900, gcc
> >>>> 13.2.1):
> >>>>
> >>>> * sysdeps/i386/fpu/e_fmod.S:
> >>>>   "fmod": {
> >>>>    "subnormals": {
> >>>>     "duration": 3.68855e+09,
> >>>>     "iterations": 2.12608e+08,
> >>>>     "max": 62.012,
> >>>>     "min": 16.798,
> >>>>     "mean": 17.349
> >>>>    },
> >>>>    "normal": {
> >>>>     "duration": 3.88459e+09,
> >>>>     "iterations": 7.168e+06,
> >>>>     "max": 2879.12,
> >>>>     "min": 16.909,
> >>>>     "mean": 541.934
> >>>>    },
> >>>>    "close-exponents": {
> >>>>     "duration": 3.692e+09,
> >>>>     "iterations": 1.96608e+08,
> >>>>     "max": 66.452,
> >>>>     "min": 16.835,
> >>>>     "mean": 18.7785
> >>>>    }
> >>>>   }
> >>>>
> >>>> * generic
> >>>>   "fmod": {
> >>>>    "subnormals": {
> >>>>     "duration": 3.68645e+09,
> >>>>     "iterations": 2.2848e+08,
> >>>>     "max": 66.896,
> >>>>     "min": 15.91,
> >>>>     "mean": 16.1347
> >>>>    },
> >>>>    "normal": {
> >>>>     "duration": 4.1455e+09,
> >>>>     "iterations": 8.192e+06,
> >>>>     "max": 3376.18,
> >>>>     "min": 15.873,
> >>>>     "mean": 506.043
> >>>>    },
> >>>>    "close-exponents": {
> >>>>     "duration": 3.70197e+09,
> >>>>     "iterations": 2.08896e+08,
> >>>>     "max": 69.597,
> >>>>     "min": 15.947,
> >>>>     "mean": 17.7216
> >>>>    }
> >>>>   }
> >>>> ---
> >>>>  sysdeps/i386/fpu/Versions                 |  4 ++++
> >>>>  sysdeps/i386/fpu/e_fmod.S                 | 18 ------------------
> >>>>  sysdeps/i386/fpu/e_fmod.c                 |  2 ++
> >>>>  sysdeps/i386/fpu/math_err.c               |  1 -
> >>>>  sysdeps/i386/fpu/w_fmod_compat.c          | 15 ---------------
> >>>>  sysdeps/ieee754/dbl-64/e_fmod.c           |  5 ++++-
> >>>>  sysdeps/mach/hurd/i386/libm.abilist       |  1 +
> >>>>  sysdeps/unix/sysv/linux/i386/libm.abilist |  1 +
> >>>>  8 files changed, 12 insertions(+), 35 deletions(-)
> >>>>  delete mode 100644 sysdeps/i386/fpu/e_fmod.S
> >>>>  create mode 100644 sysdeps/i386/fpu/e_fmod.c
> >>>>  delete mode 100644 sysdeps/i386/fpu/math_err.c
> >>>>  delete mode 100644 sysdeps/i386/fpu/w_fmod_compat.c
> >>>>
> >>>> diff --git a/sysdeps/i386/fpu/Versions b/sysdeps/i386/fpu/Versions
> >>>> index a2eec371f1..d37bc1eae6 100644
> >>>> --- a/sysdeps/i386/fpu/Versions
> >>>> +++ b/sysdeps/i386/fpu/Versions
> >>>> @@ -3,4 +3,8 @@ libm {
> >>>>      # functions used in inline functions or macros
> >>>>      __expl; __expm1l;
> >>>>    }
> >>>> +  GLIBC_2.40 {
> >>>> +    # No SVID compatible error handling.
> >>>> +    fmod;
> >>>> +  }
> >>>
> >>> This changes the ABI.  I assume that it fixes a real bug.   Is there a bug
> >>> report open for this?
> >>>
> >>
> >> The new version is the way to provide the system without the SVID compat
> >> support, which we for all ABIs but i386 on 2.38. For instance:
> >>
> >> find . -iname libm.abilist | xargs grep -w fmod
> >> ./sysdeps/unix/sysv/linux/sparc/sparc32/libm.abilist:GLIBC_2.0 fmod F
> >> ./sysdeps/unix/sysv/linux/sparc/sparc32/libm.abilist:GLIBC_2.38 fmod F
> >> [...]
> >>
> >> For i386 specifically, the old SVID symbol will be kept as fmod@GLIBC_2.0.
> >>
> >
> > Does it fix a run-time test which fails without the fix?
> >
>
> Not really, but it is one less assembly implementation in favor a generic one
> (which also shows a slight improvement on recent chips) and it sync i386
> with generic code (so less possible issues, such as the static lib in this
> patchset).

Why do we need a new symbol?
Adhemerval Zanella Netto March 28, 2024, 3:14 p.m. UTC | #6 
On 28/03/24 11:51, H.J. Lu wrote:
> On Thu, Mar 28, 2024 at 7:11 AM Adhemerval Zanella Netto
> <adhemerval.zanella@linaro.org> wrote:
>>
>>
>>
>> On 27/03/24 18:38, H.J. Lu wrote:
>>> On Wed, Mar 27, 2024 at 1:37 PM Adhemerval Zanella Netto
>>> <adhemerval.zanella@linaro.org> wrote:
>>>>
>>>>
>>>>
>>>> On 27/03/24 16:55, H.J. Lu wrote:
>>>>> On Wed, Mar 27, 2024 at 12:40 PM Adhemerval Zanella
>>>>> <adhemerval.zanella@linaro.org> wrote:
>>>>>>
>>>>>> The benchtest results shows a slight improvement (Ryzen 5900, gcc
>>>>>> 13.2.1):
>>>>>>
>>>>>> * sysdeps/i386/fpu/e_fmod.S:
>>>>>>   "fmod": {
>>>>>>    "subnormals": {
>>>>>>     "duration": 3.68855e+09,
>>>>>>     "iterations": 2.12608e+08,
>>>>>>     "max": 62.012,
>>>>>>     "min": 16.798,
>>>>>>     "mean": 17.349
>>>>>>    },
>>>>>>    "normal": {
>>>>>>     "duration": 3.88459e+09,
>>>>>>     "iterations": 7.168e+06,
>>>>>>     "max": 2879.12,
>>>>>>     "min": 16.909,
>>>>>>     "mean": 541.934
>>>>>>    },
>>>>>>    "close-exponents": {
>>>>>>     "duration": 3.692e+09,
>>>>>>     "iterations": 1.96608e+08,
>>>>>>     "max": 66.452,
>>>>>>     "min": 16.835,
>>>>>>     "mean": 18.7785
>>>>>>    }
>>>>>>   }
>>>>>>
>>>>>> * generic
>>>>>>   "fmod": {
>>>>>>    "subnormals": {
>>>>>>     "duration": 3.68645e+09,
>>>>>>     "iterations": 2.2848e+08,
>>>>>>     "max": 66.896,
>>>>>>     "min": 15.91,
>>>>>>     "mean": 16.1347
>>>>>>    },
>>>>>>    "normal": {
>>>>>>     "duration": 4.1455e+09,
>>>>>>     "iterations": 8.192e+06,
>>>>>>     "max": 3376.18,
>>>>>>     "min": 15.873,
>>>>>>     "mean": 506.043
>>>>>>    },
>>>>>>    "close-exponents": {
>>>>>>     "duration": 3.70197e+09,
>>>>>>     "iterations": 2.08896e+08,
>>>>>>     "max": 69.597,
>>>>>>     "min": 15.947,
>>>>>>     "mean": 17.7216
>>>>>>    }
>>>>>>   }
>>>>>> ---
>>>>>>  sysdeps/i386/fpu/Versions                 |  4 ++++
>>>>>>  sysdeps/i386/fpu/e_fmod.S                 | 18 ------------------
>>>>>>  sysdeps/i386/fpu/e_fmod.c                 |  2 ++
>>>>>>  sysdeps/i386/fpu/math_err.c               |  1 -
>>>>>>  sysdeps/i386/fpu/w_fmod_compat.c          | 15 ---------------
>>>>>>  sysdeps/ieee754/dbl-64/e_fmod.c           |  5 ++++-
>>>>>>  sysdeps/mach/hurd/i386/libm.abilist       |  1 +
>>>>>>  sysdeps/unix/sysv/linux/i386/libm.abilist |  1 +
>>>>>>  8 files changed, 12 insertions(+), 35 deletions(-)
>>>>>>  delete mode 100644 sysdeps/i386/fpu/e_fmod.S
>>>>>>  create mode 100644 sysdeps/i386/fpu/e_fmod.c
>>>>>>  delete mode 100644 sysdeps/i386/fpu/math_err.c
>>>>>>  delete mode 100644 sysdeps/i386/fpu/w_fmod_compat.c
>>>>>>
>>>>>> diff --git a/sysdeps/i386/fpu/Versions b/sysdeps/i386/fpu/Versions
>>>>>> index a2eec371f1..d37bc1eae6 100644
>>>>>> --- a/sysdeps/i386/fpu/Versions
>>>>>> +++ b/sysdeps/i386/fpu/Versions
>>>>>> @@ -3,4 +3,8 @@ libm {
>>>>>>      # functions used in inline functions or macros
>>>>>>      __expl; __expm1l;
>>>>>>    }
>>>>>> +  GLIBC_2.40 {
>>>>>> +    # No SVID compatible error handling.
>>>>>> +    fmod;
>>>>>> +  }
>>>>>
>>>>> This changes the ABI.  I assume that it fixes a real bug.   Is there a bug
>>>>> report open for this?
>>>>>
>>>>
>>>> The new version is the way to provide the system without the SVID compat
>>>> support, which we for all ABIs but i386 on 2.38. For instance:
>>>>
>>>> find . -iname libm.abilist | xargs grep -w fmod
>>>> ./sysdeps/unix/sysv/linux/sparc/sparc32/libm.abilist:GLIBC_2.0 fmod F
>>>> ./sysdeps/unix/sysv/linux/sparc/sparc32/libm.abilist:GLIBC_2.38 fmod F
>>>> [...]
>>>>
>>>> For i386 specifically, the old SVID symbol will be kept as fmod@GLIBC_2.0.
>>>>
>>>
>>> Does it fix a run-time test which fails without the fix?
>>>
>>
>> Not really, but it is one less assembly implementation in favor a generic one
>> (which also shows a slight improvement on recent chips) and it sync i386
>> with generic code (so less possible issues, such as the static lib in this
>> patchset).
> 
> Why do we need a new symbol?

Because the new fmod@GLIBC_2.40 for i386 won't have the SVID handling,
similar to what has been done for other architectures with
16439f419b270184ec501c531bf20d83b6745fb0;
H.J. Lu March 28, 2024, 4 p.m. UTC | #7 
On Thu, Mar 28, 2024 at 8:57 AM Adhemerval Zanella Netto
<adhemerval.zanella@linaro.org> wrote:
>
>
>
> On 28/03/24 12:55, H.J. Lu wrote:
> > On Thu, Mar 28, 2024 at 8:48 AM Adhemerval Zanella Netto
> > <adhemerval.zanella@linaro.org> wrote:
> >>
> >>
> >>
> >> On 28/03/24 12:42, H.J. Lu wrote:
> >>> On Thu, Mar 28, 2024 at 8:14 AM Adhemerval Zanella Netto
> >>> <adhemerval.zanella@linaro.org> wrote:
> >>>>
> >>>>
> >>>>
> >>>> On 28/03/24 11:51, H.J. Lu wrote:
> >>>>> On Thu, Mar 28, 2024 at 7:11 AM Adhemerval Zanella Netto
> >>>>> <adhemerval.zanella@linaro.org> wrote:
> >>>>>>
> >>>>>>
> >>>>>>
> >>>>>> On 27/03/24 18:38, H.J. Lu wrote:
> >>>>>>> On Wed, Mar 27, 2024 at 1:37 PM Adhemerval Zanella Netto
> >>>>>>> <adhemerval.zanella@linaro.org> wrote:
> >>>>>>>>
> >>>>>>>>
> >>>>>>>>
> >>>>>>>> On 27/03/24 16:55, H.J. Lu wrote:
> >>>>>>>>> On Wed, Mar 27, 2024 at 12:40 PM Adhemerval Zanella
> >>>>>>>>> <adhemerval.zanella@linaro.org> wrote:
> >>>>>>>>>>
> >>>>>>>>>> The benchtest results shows a slight improvement (Ryzen 5900, gcc
> >>>>>>>>>> 13.2.1):
> >>>>>>>>>>
> >>>>>>>>>> * sysdeps/i386/fpu/e_fmod.S:
> >>>>>>>>>>   "fmod": {
> >>>>>>>>>>    "subnormals": {
> >>>>>>>>>>     "duration": 3.68855e+09,
> >>>>>>>>>>     "iterations": 2.12608e+08,
> >>>>>>>>>>     "max": 62.012,
> >>>>>>>>>>     "min": 16.798,
> >>>>>>>>>>     "mean": 17.349
> >>>>>>>>>>    },
> >>>>>>>>>>    "normal": {
> >>>>>>>>>>     "duration": 3.88459e+09,
> >>>>>>>>>>     "iterations": 7.168e+06,
> >>>>>>>>>>     "max": 2879.12,
> >>>>>>>>>>     "min": 16.909,
> >>>>>>>>>>     "mean": 541.934
> >>>>>>>>>>    },
> >>>>>>>>>>    "close-exponents": {
> >>>>>>>>>>     "duration": 3.692e+09,
> >>>>>>>>>>     "iterations": 1.96608e+08,
> >>>>>>>>>>     "max": 66.452,
> >>>>>>>>>>     "min": 16.835,
> >>>>>>>>>>     "mean": 18.7785
> >>>>>>>>>>    }
> >>>>>>>>>>   }
> >>>>>>>>>>
> >>>>>>>>>> * generic
> >>>>>>>>>>   "fmod": {
> >>>>>>>>>>    "subnormals": {
> >>>>>>>>>>     "duration": 3.68645e+09,
> >>>>>>>>>>     "iterations": 2.2848e+08,
> >>>>>>>>>>     "max": 66.896,
> >>>>>>>>>>     "min": 15.91,
> >>>>>>>>>>     "mean": 16.1347
> >>>>>>>>>>    },
> >>>>>>>>>>    "normal": {
> >>>>>>>>>>     "duration": 4.1455e+09,
> >>>>>>>>>>     "iterations": 8.192e+06,
> >>>>>>>>>>     "max": 3376.18,
> >>>>>>>>>>     "min": 15.873,
> >>>>>>>>>>     "mean": 506.043
> >>>>>>>>>>    },
> >>>>>>>>>>    "close-exponents": {
> >>>>>>>>>>     "duration": 3.70197e+09,
> >>>>>>>>>>     "iterations": 2.08896e+08,
> >>>>>>>>>>     "max": 69.597,
> >>>>>>>>>>     "min": 15.947,
> >>>>>>>>>>     "mean": 17.7216
> >>>>>>>>>>    }
> >>>>>>>>>>   }
> >>>>>>>>>> ---
> >>>>>>>>>>  sysdeps/i386/fpu/Versions                 |  4 ++++
> >>>>>>>>>>  sysdeps/i386/fpu/e_fmod.S                 | 18 ------------------
> >>>>>>>>>>  sysdeps/i386/fpu/e_fmod.c                 |  2 ++
> >>>>>>>>>>  sysdeps/i386/fpu/math_err.c               |  1 -
> >>>>>>>>>>  sysdeps/i386/fpu/w_fmod_compat.c          | 15 ---------------
> >>>>>>>>>>  sysdeps/ieee754/dbl-64/e_fmod.c           |  5 ++++-
> >>>>>>>>>>  sysdeps/mach/hurd/i386/libm.abilist       |  1 +
> >>>>>>>>>>  sysdeps/unix/sysv/linux/i386/libm.abilist |  1 +
> >>>>>>>>>>  8 files changed, 12 insertions(+), 35 deletions(-)
> >>>>>>>>>>  delete mode 100644 sysdeps/i386/fpu/e_fmod.S
> >>>>>>>>>>  create mode 100644 sysdeps/i386/fpu/e_fmod.c
> >>>>>>>>>>  delete mode 100644 sysdeps/i386/fpu/math_err.c
> >>>>>>>>>>  delete mode 100644 sysdeps/i386/fpu/w_fmod_compat.c
> >>>>>>>>>>
> >>>>>>>>>> diff --git a/sysdeps/i386/fpu/Versions b/sysdeps/i386/fpu/Versions
> >>>>>>>>>> index a2eec371f1..d37bc1eae6 100644
> >>>>>>>>>> --- a/sysdeps/i386/fpu/Versions
> >>>>>>>>>> +++ b/sysdeps/i386/fpu/Versions
> >>>>>>>>>> @@ -3,4 +3,8 @@ libm {
> >>>>>>>>>>      # functions used in inline functions or macros
> >>>>>>>>>>      __expl; __expm1l;
> >>>>>>>>>>    }
> >>>>>>>>>> +  GLIBC_2.40 {
> >>>>>>>>>> +    # No SVID compatible error handling.
> >>>>>>>>>> +    fmod;
> >>>>>>>>>> +  }
> >>>>>>>>>
> >>>>>>>>> This changes the ABI.  I assume that it fixes a real bug.   Is there a bug
> >>>>>>>>> report open for this?
> >>>>>>>>>
> >>>>>>>>
> >>>>>>>> The new version is the way to provide the system without the SVID compat
> >>>>>>>> support, which we for all ABIs but i386 on 2.38. For instance:
> >>>>>>>>
> >>>>>>>> find . -iname libm.abilist | xargs grep -w fmod
> >>>>>>>> ./sysdeps/unix/sysv/linux/sparc/sparc32/libm.abilist:GLIBC_2.0 fmod F
> >>>>>>>> ./sysdeps/unix/sysv/linux/sparc/sparc32/libm.abilist:GLIBC_2.38 fmod F
> >>>>>>>> [...]
> >>>>>>>>
> >>>>>>>> For i386 specifically, the old SVID symbol will be kept as fmod@GLIBC_2.0.
> >>>>>>>>
> >>>>>>>
> >>>>>>> Does it fix a run-time test which fails without the fix?
> >>>>>>>
> >>>>>>
> >>>>>> Not really, but it is one less assembly implementation in favor a generic one
> >>>>>> (which also shows a slight improvement on recent chips) and it sync i386
> >>>>>> with generic code (so less possible issues, such as the static lib in this
> >>>>>> patchset).
> >>>>>
> >>>>> Why do we need a new symbol?
> >>>>
> >>>> Because the new fmod@GLIBC_2.40 for i386 won't have the SVID handling,
> >>>> similar to what has been done for other architectures with
> >>>> 16439f419b270184ec501c531bf20d83b6745fb0;
> >>>
> >>> Does it change i386 fmod behavior? If yes, we need a testcase to verify it.
> >>> If not, why is it needed?
> >>>
> >>
> >> It is not strictly required, but it makes i386 has one less assembly optimization
> >> that do not follow the rest of the code and it optimizes it slight because. Since
> >> we do actually have check for SVID, the default math tests already check the
> >> required symbol semantic.
> >
> > fmod@GLIBC_2.40 is added because of the SVID handling.  But there is no
> > user visible behavior change.  Is this correct?
>
> The user visible is the missing SVID handling (which I think noone actually uses
> it).  That's the main reason we need the compat dance and this extra complexity.
> Maybe one day we just can drop this for good...

If we want to provide the SVID compatibility, 2 testcases are needed:

1.  A testcase to show that the new implementation is incompatible with SVID.
2.  A testcase to show that the compat symbol provides the SVID compatibility.
Adhemerval Zanella Netto March 28, 2024, 6:22 p.m. UTC | #8 
On 28/03/24 13:00, H.J. Lu wrote:
> On Thu, Mar 28, 2024 at 8:57 AM Adhemerval Zanella Netto
> <adhemerval.zanella@linaro.org> wrote:
>>
>>
>>
>> On 28/03/24 12:55, H.J. Lu wrote:
>>> On Thu, Mar 28, 2024 at 8:48 AM Adhemerval Zanella Netto
>>> <adhemerval.zanella@linaro.org> wrote:
>>>>
>>>>
>>>>
>>>> On 28/03/24 12:42, H.J. Lu wrote:
>>>>> On Thu, Mar 28, 2024 at 8:14 AM Adhemerval Zanella Netto
>>>>> <adhemerval.zanella@linaro.org> wrote:
>>>>>>
>>>>>>
>>>>>>
>>>>>> On 28/03/24 11:51, H.J. Lu wrote:
>>>>>>> On Thu, Mar 28, 2024 at 7:11 AM Adhemerval Zanella Netto
>>>>>>> <adhemerval.zanella@linaro.org> wrote:
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> On 27/03/24 18:38, H.J. Lu wrote:
>>>>>>>>> On Wed, Mar 27, 2024 at 1:37 PM Adhemerval Zanella Netto
>>>>>>>>> <adhemerval.zanella@linaro.org> wrote:
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> On 27/03/24 16:55, H.J. Lu wrote:
>>>>>>>>>>> On Wed, Mar 27, 2024 at 12:40 PM Adhemerval Zanella
>>>>>>>>>>> <adhemerval.zanella@linaro.org> wrote:
>>>>>>>>>>>>
>>>>>>>>>>>> The benchtest results shows a slight improvement (Ryzen 5900, gcc
>>>>>>>>>>>> 13.2.1):
>>>>>>>>>>>>
>>>>>>>>>>>> * sysdeps/i386/fpu/e_fmod.S:
>>>>>>>>>>>>   "fmod": {
>>>>>>>>>>>>    "subnormals": {
>>>>>>>>>>>>     "duration": 3.68855e+09,
>>>>>>>>>>>>     "iterations": 2.12608e+08,
>>>>>>>>>>>>     "max": 62.012,
>>>>>>>>>>>>     "min": 16.798,
>>>>>>>>>>>>     "mean": 17.349
>>>>>>>>>>>>    },
>>>>>>>>>>>>    "normal": {
>>>>>>>>>>>>     "duration": 3.88459e+09,
>>>>>>>>>>>>     "iterations": 7.168e+06,
>>>>>>>>>>>>     "max": 2879.12,
>>>>>>>>>>>>     "min": 16.909,
>>>>>>>>>>>>     "mean": 541.934
>>>>>>>>>>>>    },
>>>>>>>>>>>>    "close-exponents": {
>>>>>>>>>>>>     "duration": 3.692e+09,
>>>>>>>>>>>>     "iterations": 1.96608e+08,
>>>>>>>>>>>>     "max": 66.452,
>>>>>>>>>>>>     "min": 16.835,
>>>>>>>>>>>>     "mean": 18.7785
>>>>>>>>>>>>    }
>>>>>>>>>>>>   }
>>>>>>>>>>>>
>>>>>>>>>>>> * generic
>>>>>>>>>>>>   "fmod": {
>>>>>>>>>>>>    "subnormals": {
>>>>>>>>>>>>     "duration": 3.68645e+09,
>>>>>>>>>>>>     "iterations": 2.2848e+08,
>>>>>>>>>>>>     "max": 66.896,
>>>>>>>>>>>>     "min": 15.91,
>>>>>>>>>>>>     "mean": 16.1347
>>>>>>>>>>>>    },
>>>>>>>>>>>>    "normal": {
>>>>>>>>>>>>     "duration": 4.1455e+09,
>>>>>>>>>>>>     "iterations": 8.192e+06,
>>>>>>>>>>>>     "max": 3376.18,
>>>>>>>>>>>>     "min": 15.873,
>>>>>>>>>>>>     "mean": 506.043
>>>>>>>>>>>>    },
>>>>>>>>>>>>    "close-exponents": {
>>>>>>>>>>>>     "duration": 3.70197e+09,
>>>>>>>>>>>>     "iterations": 2.08896e+08,
>>>>>>>>>>>>     "max": 69.597,
>>>>>>>>>>>>     "min": 15.947,
>>>>>>>>>>>>     "mean": 17.7216
>>>>>>>>>>>>    }
>>>>>>>>>>>>   }
>>>>>>>>>>>> ---
>>>>>>>>>>>>  sysdeps/i386/fpu/Versions                 |  4 ++++
>>>>>>>>>>>>  sysdeps/i386/fpu/e_fmod.S                 | 18 ------------------
>>>>>>>>>>>>  sysdeps/i386/fpu/e_fmod.c                 |  2 ++
>>>>>>>>>>>>  sysdeps/i386/fpu/math_err.c               |  1 -
>>>>>>>>>>>>  sysdeps/i386/fpu/w_fmod_compat.c          | 15 ---------------
>>>>>>>>>>>>  sysdeps/ieee754/dbl-64/e_fmod.c           |  5 ++++-
>>>>>>>>>>>>  sysdeps/mach/hurd/i386/libm.abilist       |  1 +
>>>>>>>>>>>>  sysdeps/unix/sysv/linux/i386/libm.abilist |  1 +
>>>>>>>>>>>>  8 files changed, 12 insertions(+), 35 deletions(-)
>>>>>>>>>>>>  delete mode 100644 sysdeps/i386/fpu/e_fmod.S
>>>>>>>>>>>>  create mode 100644 sysdeps/i386/fpu/e_fmod.c
>>>>>>>>>>>>  delete mode 100644 sysdeps/i386/fpu/math_err.c
>>>>>>>>>>>>  delete mode 100644 sysdeps/i386/fpu/w_fmod_compat.c
>>>>>>>>>>>>
>>>>>>>>>>>> diff --git a/sysdeps/i386/fpu/Versions b/sysdeps/i386/fpu/Versions
>>>>>>>>>>>> index a2eec371f1..d37bc1eae6 100644
>>>>>>>>>>>> --- a/sysdeps/i386/fpu/Versions
>>>>>>>>>>>> +++ b/sysdeps/i386/fpu/Versions
>>>>>>>>>>>> @@ -3,4 +3,8 @@ libm {
>>>>>>>>>>>>      # functions used in inline functions or macros
>>>>>>>>>>>>      __expl; __expm1l;
>>>>>>>>>>>>    }
>>>>>>>>>>>> +  GLIBC_2.40 {
>>>>>>>>>>>> +    # No SVID compatible error handling.
>>>>>>>>>>>> +    fmod;
>>>>>>>>>>>> +  }
>>>>>>>>>>>
>>>>>>>>>>> This changes the ABI.  I assume that it fixes a real bug.   Is there a bug
>>>>>>>>>>> report open for this?
>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> The new version is the way to provide the system without the SVID compat
>>>>>>>>>> support, which we for all ABIs but i386 on 2.38. For instance:
>>>>>>>>>>
>>>>>>>>>> find . -iname libm.abilist | xargs grep -w fmod
>>>>>>>>>> ./sysdeps/unix/sysv/linux/sparc/sparc32/libm.abilist:GLIBC_2.0 fmod F
>>>>>>>>>> ./sysdeps/unix/sysv/linux/sparc/sparc32/libm.abilist:GLIBC_2.38 fmod F
>>>>>>>>>> [...]
>>>>>>>>>>
>>>>>>>>>> For i386 specifically, the old SVID symbol will be kept as fmod@GLIBC_2.0.
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>> Does it fix a run-time test which fails without the fix?
>>>>>>>>>
>>>>>>>>
>>>>>>>> Not really, but it is one less assembly implementation in favor a generic one
>>>>>>>> (which also shows a slight improvement on recent chips) and it sync i386
>>>>>>>> with generic code (so less possible issues, such as the static lib in this
>>>>>>>> patchset).
>>>>>>>
>>>>>>> Why do we need a new symbol?
>>>>>>
>>>>>> Because the new fmod@GLIBC_2.40 for i386 won't have the SVID handling,
>>>>>> similar to what has been done for other architectures with
>>>>>> 16439f419b270184ec501c531bf20d83b6745fb0;
>>>>>
>>>>> Does it change i386 fmod behavior? If yes, we need a testcase to verify it.
>>>>> If not, why is it needed?
>>>>>
>>>>
>>>> It is not strictly required, but it makes i386 has one less assembly optimization
>>>> that do not follow the rest of the code and it optimizes it slight because. Since
>>>> we do actually have check for SVID, the default math tests already check the
>>>> required symbol semantic.
>>>
>>> fmod@GLIBC_2.40 is added because of the SVID handling.  But there is no
>>> user visible behavior change.  Is this correct?
>>
>> The user visible is the missing SVID handling (which I think noone actually uses
>> it).  That's the main reason we need the compat dance and this extra complexity.
>> Maybe one day we just can drop this for good...
> 
> If we want to provide the SVID compatibility, 2 testcases are needed:
> 
> 1.  A testcase to show that the new implementation is incompatible with SVID.
> 2.  A testcase to show that the compat symbol provides the SVID compatibility.

We don't really have SVID compatibility tests for any other optimization/simplification,
and although I don't really oppose on adding I also thinking that this is making this
change even more complicated than it would require.

I can drop the i386 changes to use generic implementations if you think it would
simplify this patchset.
Joseph Myers March 28, 2024, 6:38 p.m. UTC | #9 
On Thu, 28 Mar 2024, Adhemerval Zanella Netto wrote:

> We don't really have SVID compatibility tests for any other 
> optimization/simplification, and although I don't really oppose on 
> adding I also thinking that this is making this change even more 
> complicated than it would require.

We have math/test-matherr* as generic tests of matherr working with 
binaries that specifically use the compat symbol matherr and not with 
newly linked binaries.  Those don't however attempt to test properties of 
specific symbols for individual libm functions.

A change like the present one is not meant to be user-visible for any 
newly linked binary, as a legitimate newly linked binary couldn't link 
against the matherr compat symbol anyway.  And we don't have any attempt 
to ensure that old symbol versions of individual libm functions can be 
used together with the compat symbol of matherr - just that the compat 
symbol of matherr works with one libm function.
Adhemerval Zanella Netto March 28, 2024, 7:37 p.m. UTC | #10 
On 28/03/24 15:38, Joseph Myers wrote:
> On Thu, 28 Mar 2024, Adhemerval Zanella Netto wrote:
> 
>> We don't really have SVID compatibility tests for any other 
>> optimization/simplification, and although I don't really oppose on 
>> adding I also thinking that this is making this change even more 
>> complicated than it would require.
> 
> We have math/test-matherr* as generic tests of matherr working with 
> binaries that specifically use the compat symbol matherr and not with 
> newly linked binaries.  Those don't however attempt to test properties of 
> specific symbols for individual libm functions.

I meant that we don't have explicit SVID tests that check its semantic
(including return code, matherr status, stdout return code), like we do
for some compat symbols.  And I took that this what H.J is asking about.

> 
> A change like the present one is not meant to be user-visible for any 
> newly linked binary, as a legitimate newly linked binary couldn't link 
> against the matherr compat symbol anyway.  And we don't have any attempt 
> to ensure that old symbol versions of individual libm functions can be 
> used together with the compat symbol of matherr - just that the compat 
> symbol of matherr works with one libm function.
>
H.J. Lu March 28, 2024, 7:57 p.m. UTC | #11 
On Thu, Mar 28, 2024 at 12:37 PM Adhemerval Zanella Netto
<adhemerval.zanella@linaro.org> wrote:
>
>
>
> On 28/03/24 15:38, Joseph Myers wrote:
> > On Thu, 28 Mar 2024, Adhemerval Zanella Netto wrote:
> >
> >> We don't really have SVID compatibility tests for any other
> >> optimization/simplification, and although I don't really oppose on
> >> adding I also thinking that this is making this change even more
> >> complicated than it would require.
> >
> > We have math/test-matherr* as generic tests of matherr working with
> > binaries that specifically use the compat symbol matherr and not with
> > newly linked binaries.  Those don't however attempt to test properties of
> > specific symbols for individual libm functions.
>
> I meant that we don't have explicit SVID tests that check its semantic
> (including return code, matherr status, stdout return code), like we do
> for some compat symbols.  And I took that this what H.J is asking about.

Yes, this is what I was asking about.  If we don't have the SVID tests, we
don't need to provide the SVID compat symbols.
diff mbox series

Patch

diff --git a/sysdeps/i386/fpu/Versions b/sysdeps/i386/fpu/Versions
index a2eec371f1..d37bc1eae6 100644
--- a/sysdeps/i386/fpu/Versions
+++ b/sysdeps/i386/fpu/Versions
@@ -3,4 +3,8 @@  libm {
     # functions used in inline functions or macros
     __expl; __expm1l;
   }
+  GLIBC_2.40 {
+    # No SVID compatible error handling.
+    fmod;
+  }
 }
diff --git a/sysdeps/i386/fpu/e_fmod.S b/sysdeps/i386/fpu/e_fmod.S
deleted file mode 100644
index 86ac1bcfaf..0000000000
--- a/sysdeps/i386/fpu/e_fmod.S
+++ /dev/null
@@ -1,18 +0,0 @@ 
-/*
- * Public domain.
- */
-
-#include <machine/asm.h>
-#include <libm-alias-finite.h>
-
-ENTRY(__ieee754_fmod)
-	fldl	12(%esp)
-	fldl	4(%esp)
-1:	fprem
-	fstsw	%ax
-	sahf
-	jp	1b
-	fstp	%st(1)
-	ret
-END (__ieee754_fmod)
-libm_alias_finite (__ieee754_fmod, __fmod)
diff --git a/sysdeps/i386/fpu/e_fmod.c b/sysdeps/i386/fpu/e_fmod.c
new file mode 100644
index 0000000000..3625758f97
--- /dev/null
+++ b/sysdeps/i386/fpu/e_fmod.c
@@ -0,0 +1,2 @@ 
+#define FMOD_VERSION GLIBC_2_40
+#include <sysdeps/ieee754/dbl-64/e_fmod.c>
diff --git a/sysdeps/i386/fpu/math_err.c b/sysdeps/i386/fpu/math_err.c
deleted file mode 100644
index 1cc8931700..0000000000
--- a/sysdeps/i386/fpu/math_err.c
+++ /dev/null
@@ -1 +0,0 @@ 
-/* Not needed.  */
diff --git a/sysdeps/i386/fpu/w_fmod_compat.c b/sysdeps/i386/fpu/w_fmod_compat.c
deleted file mode 100644
index 528bfc2a13..0000000000
--- a/sysdeps/i386/fpu/w_fmod_compat.c
+++ /dev/null
@@ -1,15 +0,0 @@ 
-/* i386 provides an optimized __ieee752_fmod.  */
-#include <math-svid-compat.h>
-#ifdef SHARED
-# undef SHLIB_COMPAT
-# define SHLIB_COMPAT(a, b, c) 1
-# undef LIBM_SVID_COMPAT
-# define LIBM_SVID_COMPAT 1
-# undef compat_symbol
-# define compat_symbol(a, b, c, d)
-# include <math/w_fmod_compat.c>
-libm_alias_double (__fmod_compat, fmod)
-#else
-#include <math-type-macros-double.h>
-#include <w_fmod_template.c>
-#endif
diff --git a/sysdeps/ieee754/dbl-64/e_fmod.c b/sysdeps/ieee754/dbl-64/e_fmod.c
index b33cfb1223..7651cd212a 100644
--- a/sysdeps/ieee754/dbl-64/e_fmod.c
+++ b/sysdeps/ieee754/dbl-64/e_fmod.c
@@ -175,7 +175,10 @@  __fmod (double x, double y)
 strong_alias (__fmod, __ieee754_fmod)
 libm_alias_finite (__ieee754_fmod, __fmod)
 #if LIBM_SVID_COMPAT
-versioned_symbol (libm, __fmod, fmod, GLIBC_2_38);
+# ifndef FMOD_VERSION
+#  define FMOD_VERSION GLIBC_2_38
+# endif
+versioned_symbol (libm, __fmod, fmod, FMOD_VERSION);
 libm_alias_double_other (__fmod, fmod)
 #else
 libm_alias_double (__fmod, fmod)
diff --git a/sysdeps/mach/hurd/i386/libm.abilist b/sysdeps/mach/hurd/i386/libm.abilist
index 8f40ddb150..30665f8b1a 100644
--- a/sysdeps/mach/hurd/i386/libm.abilist
+++ b/sysdeps/mach/hurd/i386/libm.abilist
@@ -1181,3 +1181,4 @@  GLIBC_2.35 fsqrt F
 GLIBC_2.35 fsqrtl F
 GLIBC_2.35 hypot F
 GLIBC_2.35 hypotf F
+GLIBC_2.40 fmod F
diff --git a/sysdeps/unix/sysv/linux/i386/libm.abilist b/sysdeps/unix/sysv/linux/i386/libm.abilist
index 5d89aaa08e..44932f111d 100644
--- a/sysdeps/unix/sysv/linux/i386/libm.abilist
+++ b/sysdeps/unix/sysv/linux/i386/libm.abilist
@@ -1188,3 +1188,4 @@  GLIBC_2.35 fsqrt F
 GLIBC_2.35 fsqrtl F
 GLIBC_2.35 hypot F
 GLIBC_2.35 hypotf F
+GLIBC_2.40 fmod F