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

[Corinna Vinschen <vinschen at redhat dot com>]

On Jul  5 12:21, Szabolcs Nagy wrote:
> 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.

Sounds good.


Thanks,
Corinna

-- 
Corinna Vinschen
Cygwin Maintainer
Red Hat

Attachment: signature.asc
Description: PGP signature