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

[Segher Boessenkool <segher@kernel.crashing.org>]

Hi!

On Thu, Aug 19, 2021 at 01:16:16PM -0500, Paul A. Clarke wrote:
> On Wed, Aug 18, 2021 at 05:46:58PM -0500, Segher Boessenkool wrote:
> > 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?)

It looked to me like you do a lot of unnecessary asm.

> > > 	* 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?

Macros are from the 1970's, inline functions are the new hot.  Why do
you need macros here?  The patch should say (the patch message likely).

> > > +#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.

That doesn't make it less silly :-)

> > > +__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).

"extern" is not redundant for inline functions.  Since you have
always_inline here, gnu_inline extern inline has the same meaning as
static inline in portable C.

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

Not using the long deprecated gnu_inline.

> > Wrong indent.  This code is very hard to read because of that.
> 
> OK, will fix in v2.

Thanks!

> > 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.

If it is useful for others, then yes please!  Ideally you can make a
builtin that we can also reasonably implement without support for the
new insns, so we can use the builtin whenever the builtin exists.

Thanks,


Segher