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

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.