Re: [PATCH 07/28] powerpc: trunc/truncf refactor

[Adhemerval Zanella <adhemerval dot zanella at linaro dot org>]

On 09/05/2019 17:06, Gabriel F. T. Gomes wrote:
> On Fri, Mar 29 2019, Adhemerval Zanella wrote:
>>
>> 	* sysdeps/powerpc/fpu/trunc_to_integer.h (set_truncing_mode): Add
>> 	 TRUNC handling.
> 
> Similar to floor and round, remember to add a note here about the
> definition of TRUNC.
> 
>> --- a/sysdeps/powerpc/fpu/round_to_integer.h
>> +++ b/sysdeps/powerpc/fpu/round_to_integer.h
>> @@ -38,6 +38,7 @@ set_fenv_mode (enum round_mode mode)
>>    {
>>    case CEIL:  rmode = FE_UPWARD; break;
>>    case FLOOR: rmode = FE_DOWNWARD; break;
>> +  case TRUNC:
>>    case ROUND: rmode = FE_TOWARDZERO; break;
>>    default:    rmode = FE_TONEAREST; break;
>>    }
> 
> Likewise, remember to add the definition of TRUNC.
> 
> The patch looks good to me with these changes.
> 
> Reviewed-by: Gabriel F. T. Gomes <gabriel@inconstante.eti.br>
> 
>> +double
>> +__trunc (double x)
>> +{
>> +#ifdef _ARCH_PWR5X
>> +  return __builtin_trunc (x);
>> +#else
>> +  return round_to_integer_double (TRUNC, x);
>> +#endif
>> +}
> 
> OK.  Arch check looks correct.
> 
>> -/* double [fp1] trunc (double x [fp1])
>> -   IEEE 1003.1 trunc function.  IEEE specifies "trunc to the integer
>> -   value, in floating format, nearest to but no larger in magnitude
>> -   then the argument."
>> -   We set "round toward Zero" mode and trunc by adding +-2**52 then
>> -   subtracting +-2**52.  */
> 
> This comment got lost.  Would you deem it appropriate to reinstate it
> somewhere in round_to_integer_double (you would need to make it generic
> so that it fits the four rounding directions (so far), as well as
> nearbyint from the subsequent patch... perhaps something along the lines
> of the paragraph below)?
> 
>   IEEE 1003.1 nearest integer functions.  IEEE specifies several
>   functions that approximate a floating-point to a nearby integer value.
>   For each of them we set the appropriate rounding mode, then add and
>   subtract +-2**52.
> 

My view is such direct comments is useful on assembly implementation
where is not really obvious what the instruction sequence is doing.
I usually see that the C counterpart should be straightforward to infer
this. I can reinstate the comment though if you think it is really
valuable.