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

[Corinna Vinschen <vinschen at redhat dot com>]

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.

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?

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?


Thanks,
Corinna

-- 
Corinna Vinschen
Cygwin Maintainer
Red Hat

Attachment: signature.asc
Description: PGP signature