Re: [PATCH 1/6] rs6000: Support SSE4.1 "round" intrinsics

["Paul A. Clarke" <pc@us.ibm.com>]

On Wed, Aug 18, 2021 at 05:46:58PM -0500, Segher Boessenkool wrote:
> On Mon, Aug 09, 2021 at 03:23:50PM -0500, Paul A. Clarke wrote:
> > Suppress exceptions (when specified), by saving, manipulating, and
> > restoring the FPSCR.  Similarly, save, set, and restore the floating-point
> > rounding mode when required.
> > 
> > No attempt is made to optimize writing the FPSCR (by checking if the new
> > value would be the same), other than using lighter weight instructions
> > when possible.
> 
> There are __builtin_set_fpscr_rn and friends, please use those, those
> are optimised for any platform.

I do.  (Unless I missed an opportunity somewhere?)

The "optimize" comment refers to, for example, not checking the current
rounding mode before setting and restoring it.

> > 	* config/rs6000/smmintrin.h (_mm_ceil_pd, _mm_ceil_ps, _mm_ceil_sd,
> > 	_mm_ceil_ss, _mm_floor_pd, _mm_floor_ps, _mm_floor_sd, _mm_floor_ss):
> > 	Convert from function to macro.
> 
> Please explain why you regress this (not in the changelog of course).

I'm not sure what "regress" means here?

I should've said that these are now identical implementations to those
found in config/i386/smmintrin.h.  I'll add that to the commit message
in v2.

> > +/* Rounding mode macros. */
> > +#define _MM_FROUND_TO_NEAREST_INT       0x00
> > +#define _MM_FROUND_TO_ZERO              0x01
> > +#define _MM_FROUND_TO_POS_INF           0x02
> > +#define _MM_FROUND_TO_NEG_INF           0x03
> > +#define _MM_FROUND_CUR_DIRECTION        0x04
> 
> You can just write "0" .. "4", heh.

Copied from config/i386/smmintrin.h.

> > +#define _MM_FROUND_NINT		\
> > +  (_MM_FROUND_TO_NEAREST_INT | _MM_FROUND_RAISE_EXC)
> > +#define _MM_FROUND_FLOOR	\
> > +  (_MM_FROUND_TO_NEG_INF | _MM_FROUND_RAISE_EXC)
> > +#define _MM_FROUND_CEIL		\
> > +  (_MM_FROUND_TO_POS_INF | _MM_FROUND_RAISE_EXC)
> > +#define _MM_FROUND_TRUNC	\
> > +  (_MM_FROUND_TO_ZERO | _MM_FROUND_RAISE_EXC)
> > +#define _MM_FROUND_RINT		\
> > +  (_MM_FROUND_CUR_DIRECTION | _MM_FROUND_RAISE_EXC)
> > +#define _MM_FROUND_NEARBYINT	\
> > +  (_MM_FROUND_CUR_DIRECTION | _MM_FROUND_NO_EXC)
> 
> All these macro definitions will comfortably fit on one line.

Copied from config/i386/smmintrin.h.

> > +__inline __m128d
> > +__attribute__ ((__gnu_inline__, __always_inline__, __artificial__))
> > +_mm_round_pd (__m128d __A, int __rounding)
> > +{
> 
> Non-static inline is not what you want, esp. with gnu-inline?  Or, what
> is the goal, and why can you not do it with modern inline?

This is the same basic signature as the other 600+ intrinsics.
Actually, they were all described as "extern", but in a previous
review, you said:
> "extern" on definitions is superfluous
So, I've dropped that for newer ones.
Should they all instead be "static"?

The goal is to be compatible with the i386 implementations.
Those typically use something like:

  extern __inline __m128 __attribute__((__gnu_inline__, __always_inline__, __artificial__))

(which kinda makes me want to put "extern" back, now that I think
about it).

I'm not sure what you mean by "modern inline".

> > +  __v2df __r;
> > +  union {
> > +    double __fr;
> > +    long long __fpscr;
> > +  } __save, __tmp;
> > +
> > +  if (__rounding & _MM_FROUND_NO_EXC)
> > +  {
> 
> Wrong indent.  This code is very hard to read because of that.

OK, will fix in v2.

> If you figure that gee, it would be a nice if we had a builtin for
> mffsce, then please make one?  :-)

Is one use-case sufficient grounds?  I can give it a shot if so.

> > +    case _MM_FROUND_TO_NEAREST_INT:
> > +      __tmp.__fr = __builtin_mffsl ();
> > +      __attribute__((fallthrough));
> 
> Space before (.

OK

> > +    case _MM_FROUND_TO_NEAREST_INT |_MM_FROUND_NO_EXC:
> 
> Space after |.

OK

> Please fix these things and resend.

Will do.  Thanks!

PC