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

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

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.

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

> +/* 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.

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

> +__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?

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

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

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

Space before (.

> +    case _MM_FROUND_TO_NEAREST_INT |_MM_FROUND_NO_EXC:

Space after |.

Please fix these things and resend.


Segher