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

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

On 05/07/18 11:01, Szabolcs Nagy wrote:
> On 05/07/18 09:48, Corinna Vinschen wrote:
> > 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).

the int32_t cast changes code generation with clang (it adds
sign extension instructions) it seems compilers can figure out

 (uint64_t)(int32_t)(long)round() << 32;

but not

 (uint64_t)(int32_t)round() << 32;

because the former lowers into a double->int64 conversion
on 64bit systems the latter lowers into double->int32
conversion and then the int32 requires a sign extend.

so i'll keep the code as is for now.

> > 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.

i think i should remove this part and only use TOINT_INTRINSICS
vs !TOINT_INTRINSICS instead of various TOINT_* macros that
nobody uses.