Re: [PATCH 04/28] powerpc: ceil/ceilf refactor

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

On 24/04/2019 22:56, Gabriel F. T. Gomes wrote:
> On Fri, Mar 29 2019, Adhemerval Zanella wrote:
>>
>> The IFUNC organization for powerpc64 is also change to be enabled only
>> for powerpc64 and not for powerpc64le.
> 
> OK.
> 
>> 	* sysdeps/powerpc/fpu/fenv_libc.h (__fesetround_inline_nocheck): New
>> 	macro.
> 
> I really wish we can get rid of the burden of writing changelogs, but
> for now shouldn't this be function, instead of macro?
> 
>> +/* Same as __fesetround_inline, however without runtime check to use DFP
>> +   mtfsfi syntax or if round is valid.  */
> 
> What would be a runtime check to use DFP syntax? Are you referring to
> the optional third parameter (W) to mtfsfi?  I don't see such mechanism
> in __fesetround_inline, so I'm not sure I understand this
> comment/comparison.

The comment is not clear indeed, to use mtfsfi correctly one should
check hwcap for PPC_FEATURE_HAS_DFP as fesetenv_register do.  I changed
comment to:

/* Same as __fesetround_inline, however without runtime check to use DFP
   mtfsfi syntax (as relax_fenv_state) or if round value is valid.  */

> 
>> +static inline void
>> +__fesetround_inline_nocheck (const int round)
>> +{
>> +  asm volatile ("mtfsfi 7,%0" : : "i" (round));
>> +}
>> +
> 
> The implementation looks OK.
> 
>> +enum round_mode
>> +{
>> +  CEIL,
>> +  FLOOR,
>> +  ROUND,
>> +  TRUNC,
>> +  NEARBYINT,
>> +};
> 
> OK.  To be used in following patches.

In fact I think it would be better to add each round_mode on each
patch it implements. I have changed it locally.

> 
>> +static inline void
>> +set_fenv_mode (enum round_mode mode)
>> +{
>> +  int rmode;
>> +  switch (mode)
>> +  {
>> +  case CEIL:  rmode = FE_UPWARD; break;
>> +  default:    rmode = FE_TONEAREST; break;
>> +  }
>> +  __fesetround_inline_nocheck (rmode);
>> +}
> 
> OK.
> 
>> +static inline float
>> +round_to_integer_float (enum round_mode mode, float x)
>> +{
>> +  /* Ensure sNaN input is converted to qNaN.  */
>> +  if (__glibc_unlikely (isnan (x)))
>> +    return x + x;
>> +
>> +  if (fabs (x) > 0x1p+23)
>> +    return x;
>> +
>> +  float r = x;
>> +
>> +  /* Save current FPU rounding mode and inexact state.  */
>> +  fenv_t fe = fegetenv_register ();
>> +  set_fenv_mode (mode);
>> +  if (x > 0.0)
>> +    {
>> +      r += 0x1p+23;
>> +      r -= 0x1p+23;
>> +      r = fabs (r);
>> +    }
>> +  else if (x < 0.0)
>> +    {
>> +      r -= 0x1p+23;
>> +      r += 0x1p+23;
>> +      r = -fabs (r);
>> +    }
>> +  __builtin_mtfsf (0xff, fe);
>> +
>> +  return r;
>> +}
> 
> Ok.
> 
>> +static inline double
>> +round_to_integer_double (enum round_mode mode, double x)
>> +{
>> +  /* Ensure sNaN input is converted to qNaN.  */
>> +  if (__glibc_unlikely (isnan (x)))
>> +    return x + x;
>> +
>> +  if (fabs (x) > 0x1p+52)
>> +    return x;
>> +
>> +  double r = x;
>> +
>> +  /* Save current FPU rounding mode and inexact state.  */
>> +  fenv_t fe = fegetenv_register ();
>> +  set_fenv_mode (mode);
>> +  if (x > 0.0)
>> +    {
>> +      r += 0x1p+52;
>> +      r -= 0x1p+52;
>> +      r = fabs (r);
>> +    }
>> +  else if (x < 0.0)
>> +    {
>> +      r -= 0x1p+52;
>> +      r += 0x1p+52;
>> +      r = -fabs (r);
>> +    }
>> +  __builtin_mtfsf (0xff, fe);
>> +
>> +  return r;
>> +}
> 
> OK.
> 
>> +double
>> +__ceil (double x)
>> +{
>> +#ifdef _ARCH_PWR5X
>> +  return __builtin_ceil (x);
>> +#else
>> +  return round_to_integer_double (CEIL, x);
>> +#endif
> 
> The arch check looks correct.
>