Re: [PATCH 0/3] Updates to the new math code

[Szabolcs Nagy <szabolcs dot nagy at arm dot com>]

On 05/07/18 09:48, Corinna Vinschen wrote:
> Hi Szabolcs,
>
> On Jul  4 16:49, Szabolcs Nagy wrote:
> > There are some modifications and bug fixes in the Arm Optimized
> > Routines repo and i'd like to sync newlib with it.
> >
> > Szabolcs Nagy (3):
> >    Fix code style and comments of new math code
> >    Change the return type of converttoint and document the semantics
> >    Fix large ulp error in pow without fma very near 1.0
> >
> >   newlib/libm/common/exp.c         | 22 +++++++++++++------
> >   newlib/libm/common/exp2.c        | 18 +++++++++++-----
> >   newlib/libm/common/log.c         | 46 ++++++++++++++++++++++------------------
> >   newlib/libm/common/log2.c        | 31 ++++++++++++++-------------
> >   newlib/libm/common/math_config.h | 44 ++++++++++++++++++++++++++++++++------
> >   newlib/libm/common/pow.c         | 35 ++++++++++++++++++++++--------
> >   newlib/libm/common/sincosf.c     | 16 +++++++-------
> >   newlib/libm/common/sinf.c        | 12 +++++------
> >   8 files changed, 147 insertions(+), 77 deletions(-)
>
> while you're working on this, I have three questions:
>
> 1. There's __HAVE_FAST_FMA defined in libc/include/machine/ieeefp.h and
>     HAVE_FAST_ROUND/HAVE_FAST_LROUND defined in libm/common/math_config.h.
>     Wouldn't it make more sense to define all of them in one file?
>     Not sure which one, but libm/common/math_config.h looks right to me.

ok, i can move HAVE_FAST_FMA (without __ to be consistent
with the others) to math_config.h

> 2. You're changing converttoint to return int32_t, but the #else
>     branch is casting to long:
>
>      # if HAVE_FAST_LROUND
>        return lround (x);
>      # else
>        return (long) round (x);
>      # endif
>     
>     This looks wrong to me independent of the return type of
>     converttoint.  Shouldn't that cast to an int of fixed size, i.e.,
>     int32_t?

the (long) cast was used so it's the same operation as
lround (assuming -fno-math-errno semantics) since the
compiler has a builtin for that which targets may inline,
but i guess (int32_t) would work too (since out of bound
conversion is either undefined or unspecified).

> 3. Along the same lines, in newlib/libm/common/sf_exp.c there's the
>     following expression:
>
>      #if TOINT_INTRINSICS
>        kd = roundtoint (z);
>        ki = converttoint (z);
>      #elif TOINT_RINT
>        kd = rint (z);
>        ki = (long) kd;
>        ^^^^^^^^^^^^^^^
>
>     Shouldn't this cast to int32_t as well?

likewise.

i'll experiment and if int32_t works i'll change it.