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

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.