Re: [PATCH 3/4] Add ILP32 support to aarch64

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

On 08/08/17 16:01, Szabolcs Nagy wrote:
> On 05/08/17 00:15, Steve Ellcey wrote:
>> On Fri, 2017-08-04 at 00:12 +0000, Joseph Myers wrote:
>>>> On Thu, 3 Aug 2017, Wilco Dijkstra wrote:
>>>>
>>>>>> The generic implementation may well be faster... I'm not sure where the
>>>>>> requirement of not raising inexact comes from (I don't see it in the definition
>>>>>> of lrint, and we generally don't care since inexact is set by almost every FP
>>>>>> calculation), but if it is absolutely required you'd special case values larger
>>>>>> than LONG_MAX.
>>>> The requirement comes from lrint being bound to IEEE 754 conversion 
>>>> operations, so only raising inexact under the conditions specified and no 
>>>> spurious inexact.
>>
>> Here is a new version of this patch.  It (mostly) avoids fenv calls
>> when not needed and preserves any exceptions that may be set on entry
>> to the function.
>>
> ...
>> +#if IREG_SIZE == 64 && OREG_SIZE == 32
>> +  if (__builtin_fabs (x) > INT32_MAX - 2)
> 
> i don't understand the -2 here.
> 
>> +    {
>> +      /* Converting large values to a 32 bit in may cause the frintx/fcvtza
> 
> s/in/int/
> 
>> +	 sequence to set both FE_INVALID and FE_INEXACT.  To avoid this
>> +         we save and restore the FE and only set one or the other.  */
>> +
>> +      fenv_t env;
>> +      bool invalid_p, inexact_p;
>> +
>> +      libc_feholdexcept (&env);
>> +      asm ( "frintx" "\t%" IREGS "1, %" IREGS "2\n\t"
>> +	    "fcvtzs" "\t%" OREGS "0, %" IREGS "1"
>> +	    : "=r" (result), "=w" (temp) : "w" (x) );
>> +      invalid_p = libc_fetestexcept (FE_INVALID);
>> +      inexact_p = libc_fetestexcept (FE_INEXACT);
> 
> multiple flags can be tested/raised in a single call.
> 
>> +      libc_fesetenv (&env);
>> +
>> +      if (invalid_p)
>> +	feraiseexcept (FE_INVALID);
>> +      else if (inexact_p)
>> +	feraiseexcept (FE_INEXACT);
>> +
> 
> i think correct trapping is not guaranteed by glibc,
> only correct status flags when the function returns,
> so spurious inexact is not a problem if it is already
> raised, and then i expect better code gen for the
> inexact clearing approach:
> 
> if (fabs (x) > INT32_MAX && fetestexcept (FE_INEXACT) == 0)
>   {
>     asm (...);
>     if (fetestexcept (FE_INVALID|FE_INEXACT) == (FE_INVALID|FE_INEXACT))
>       feclearexcept (FE_INEXACT);

Wilco pointed out to me that this approach would be
more complicated because invalid may be already raised
so you need to check that too, clear it if it's set
and restore it at the end..

>   }
> else
>   asm (...);
> 
>