diff mbox series

i386: Fix up _mm_min_ss etc. handling of zeros and NaNs [PR116738]

Message ID ZuyOXVv01X+ifwcK@tucnak
State New
Headers show
Series i386: Fix up _mm_min_ss etc. handling of zeros and NaNs [PR116738] | expand

Commit Message

Jakub Jelinek Sept. 19, 2024, 8:49 p.m. UTC
Hi!

min/max patterns for intrinsics which on x86 result in the second
input operand if the two operands are both zeros or one or both of them
are a NaN shouldn't use SMIN/SMAX RTL, because that is similarly to
MIN_EXPR/MAX_EXPR undefined what will be the result in those cases.

The following patch adds an expander which uses either a new pattern with
UNSPEC_IEEE_M{AX,IN} or use the S{MIN,MAX} representation of the same.

Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?

P.S. I have a patch to replace UNSPEC_IEEE_M{AX,IN} with IF_THEN_ELSE
(except for the 3dNOW! PFMIN/MAX, those actually are documented to behave
differently), but it actually doesn't improve anything much, as
simplify_const_relational_operation nor simplify_ternary_operation aren't
able to fold comparisons with two CONST_VECTOR operands or IF_THEN_ELSE
with 3 CONST_VECTOR operands.
So, maybe better approach will be to generic fold the builtins with constant
arguments (maybe leaving NaNs to runtime).

2024-09-19  Uros Bizjak  <ubizjak@gmail.com>
	    Jakub Jelinek  <jakub@redhat.com>

	PR target/116738
	* config/i386/subst.md (mask_scalar_operand_arg34,
	mask_scalar_expand_op3, round_saeonly_scalar_mask_arg3): New
	subst attributes.
	* config/i386/sse.md
	(<sse>_vm<code><mode>3<mask_scalar_name><round_saeonly_scalar_name>):
	Change from define_insn to define_expand, rename the old define_insn
	to ...
	(*<sse>_vm<code><mode>3<mask_scalar_name><round_saeonly_scalar_name>):
	... this.
	(<sse>_ieee_vm<ieee_maxmin><mode>3<mask_scalar_name><round_saeonly_scalar_name>):
	New define_insn.

	* gcc.target/i386/sse-pr116738.c: New test.


	Jakub

Comments

Richard Biener Sept. 20, 2024, 6:01 a.m. UTC | #1
On Thu, Sep 19, 2024 at 10:50 PM Jakub Jelinek <jakub@redhat.com> wrote:
>
> Hi!
>
> min/max patterns for intrinsics which on x86 result in the second
> input operand if the two operands are both zeros or one or both of them
> are a NaN shouldn't use SMIN/SMAX RTL, because that is similarly to
> MIN_EXPR/MAX_EXPR undefined what will be the result in those cases.
>
> The following patch adds an expander which uses either a new pattern with
> UNSPEC_IEEE_M{AX,IN} or use the S{MIN,MAX} representation of the same.
>
> Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?
>
> P.S. I have a patch to replace UNSPEC_IEEE_M{AX,IN} with IF_THEN_ELSE
> (except for the 3dNOW! PFMIN/MAX, those actually are documented to behave
> differently), but it actually doesn't improve anything much, as
> simplify_const_relational_operation nor simplify_ternary_operation aren't
> able to fold comparisons with two CONST_VECTOR operands or IF_THEN_ELSE
> with 3 CONST_VECTOR operands.
> So, maybe better approach will be to generic fold the builtins with constant
> arguments (maybe leaving NaNs to runtime).

It would be possible to fold them in the gimple folding hook to VEC_COND_EXPRs
with the chance the min/max operation being lost when expanding to RTL.

Richard.

>
> 2024-09-19  Uros Bizjak  <ubizjak@gmail.com>
>             Jakub Jelinek  <jakub@redhat.com>
>
>         PR target/116738
>         * config/i386/subst.md (mask_scalar_operand_arg34,
>         mask_scalar_expand_op3, round_saeonly_scalar_mask_arg3): New
>         subst attributes.
>         * config/i386/sse.md
>         (<sse>_vm<code><mode>3<mask_scalar_name><round_saeonly_scalar_name>):
>         Change from define_insn to define_expand, rename the old define_insn
>         to ...
>         (*<sse>_vm<code><mode>3<mask_scalar_name><round_saeonly_scalar_name>):
>         ... this.
>         (<sse>_ieee_vm<ieee_maxmin><mode>3<mask_scalar_name><round_saeonly_scalar_name>):
>         New define_insn.
>
>         * gcc.target/i386/sse-pr116738.c: New test.
>
> --- gcc/config/i386/subst.md.jj 2024-09-18 15:49:42.200791315 +0200
> +++ gcc/config/i386/subst.md    2024-09-19 12:32:51.048626421 +0200
> @@ -366,6 +366,8 @@ (define_subst_attr "mask_scalar_operand4
>  (define_subst_attr "mask_scalarcz_operand4" "mask_scalarcz" "" "%{%5%}%N4")
>  (define_subst_attr "mask_scalar4_dest_false_dep_for_glc_cond" "mask_scalar" "1" "operands[4] == CONST0_RTX(<MODE>mode)")
>  (define_subst_attr "mask_scalarc_dest_false_dep_for_glc_cond" "mask_scalarc" "1" "operands[3] == CONST0_RTX(V8HFmode)")
> +(define_subst_attr "mask_scalar_operand_arg34" "mask_scalar" "" ", operands[3], operands[4]")
> +(define_subst_attr "mask_scalar_expand_op3" "mask_scalar" "3" "5")
>
>  (define_subst "mask_scalar"
>    [(set (match_operand:SUBST_V 0)
> @@ -473,6 +475,7 @@ (define_subst_attr "round_saeonly_scalar
>  (define_subst_attr "round_saeonly_scalar_constraint" "round_saeonly_scalar" "vm" "v")
>  (define_subst_attr "round_saeonly_scalar_prefix" "round_saeonly_scalar" "vex" "evex")
>  (define_subst_attr "round_saeonly_scalar_nimm_predicate" "round_saeonly_scalar" "nonimmediate_operand" "register_operand")
> +(define_subst_attr "round_saeonly_scalar_mask_arg3" "round_saeonly_scalar" "" ", operands[<mask_scalar_expand_op3>]")
>
>  (define_subst "round_saeonly_scalar"
>    [(set (match_operand:SUBST_V 0)
> --- gcc/config/i386/sse.md.jj   2024-09-10 16:26:02.875151133 +0200
> +++ gcc/config/i386/sse.md      2024-09-19 12:43:31.693030695 +0200
> @@ -3333,7 +3333,27 @@ (define_insn "*ieee_<ieee_maxmin><mode>3
>            (const_string "*")))
>     (set_attr "mode" "<ssescalarmode>")])
>
> -(define_insn "<sse>_vm<code><mode>3<mask_scalar_name><round_saeonly_scalar_name>"
> +(define_expand "<sse>_vm<code><mode>3<mask_scalar_name><round_saeonly_scalar_name>"
> +  [(set (match_operand:VFH_128 0 "register_operand")
> +       (vec_merge:VFH_128
> +         (smaxmin:VFH_128
> +           (match_operand:VFH_128 1 "register_operand")
> +           (match_operand:VFH_128 2 "nonimmediate_operand"))
> +        (match_dup 1)
> +        (const_int 1)))]
> +  "TARGET_SSE"
> +{
> +  if (!flag_finite_math_only || flag_signed_zeros)
> +    {
> +      emit_insn (gen_<sse>_ieee_vm<maxmin_float><mode>3<mask_scalar_name><round_saeonly_scalar_name>
> +                (operands[0], operands[1], operands[2]
> +                 <mask_scalar_operand_arg34>
> +                 <round_saeonly_scalar_mask_arg3>));
> +      DONE;
> +    }
> +})
> +
> +(define_insn "*<sse>_vm<code><mode>3<mask_scalar_name><round_saeonly_scalar_name>"
>    [(set (match_operand:VFH_128 0 "register_operand" "=x,v")
>         (vec_merge:VFH_128
>           (smaxmin:VFH_128
> @@ -3348,6 +3368,25 @@ (define_insn "<sse>_vm<code><mode>3<mask
>    [(set_attr "isa" "noavx,avx")
>     (set_attr "type" "sse")
>     (set_attr "btver2_sse_attr" "maxmin")
> +   (set_attr "prefix" "<round_saeonly_scalar_prefix>")
> +   (set_attr "mode" "<ssescalarmode>")])
> +
> +(define_insn "<sse>_ieee_vm<ieee_maxmin><mode>3<mask_scalar_name><round_saeonly_scalar_name>"
> +  [(set (match_operand:VFH_128 0 "register_operand" "=x,v")
> +       (vec_merge:VFH_128
> +         (unspec:VFH_128
> +           [(match_operand:VFH_128 1 "register_operand" "0,v")
> +            (match_operand:VFH_128 2 "nonimmediate_operand" "xm,<round_saeonly_scalar_constraint>")]
> +           IEEE_MAXMIN)
> +        (match_dup 1)
> +        (const_int 1)))]
> +  "TARGET_SSE"
> +  "@
> +   <ieee_maxmin><ssescalarmodesuffix>\t{%2, %0|%0, %<iptr>2}
> +   v<ieee_maxmin><ssescalarmodesuffix>\t{<round_saeonly_scalar_mask_op3>%2, %1, %0<mask_scalar_operand3>|%0<mask_scalar_operand3>, %1, %<iptr>2<round_saeonly_scalar_mask_op3>}"
> +  [(set_attr "isa" "noavx,avx")
> +   (set_attr "type" "sse")
> +   (set_attr "btver2_sse_attr" "maxmin")
>     (set_attr "prefix" "<round_saeonly_scalar_prefix>")
>     (set_attr "mode" "<ssescalarmode>")])
>
> --- gcc/testsuite/gcc.target/i386/sse-pr116738.c.jj     2024-09-19 12:52:33.502681950 +0200
> +++ gcc/testsuite/gcc.target/i386/sse-pr116738.c        2024-09-19 12:54:20.938219741 +0200
> @@ -0,0 +1,28 @@
> +/* PR target/116738 */
> +/* { dg-do run } */
> +/* { dg-options "-O2 -msse" } */
> +/* { dg-require-effective-target sse } */
> +
> +#include "sse-check.h"
> +
> +static inline float
> +clamp (float f)
> +{
> +  __m128 v = _mm_set_ss (f);
> +  __m128 zero = _mm_setzero_ps ();
> +  __m128 greatest = _mm_set_ss (__FLT_MAX__);
> +  v = _mm_min_ss (v, greatest);
> +  v = _mm_max_ss (v, zero);
> +  return _mm_cvtss_f32 (v);
> +}
> +
> +static void
> +sse_test (void)
> +{
> +  float f = clamp (-0.0f);
> +  if (f != 0.0f || __builtin_signbitf (f))
> +    abort ();
> +  f = clamp (__builtin_nanf (""));
> +  if (__builtin_isnanf (f) || f != __FLT_MAX__)
> +    abort ();
> +}
>
>         Jakub
>
Uros Bizjak Sept. 20, 2024, 6:19 a.m. UTC | #2
On Thu, Sep 19, 2024 at 10:49 PM Jakub Jelinek <jakub@redhat.com> wrote:
>
> Hi!
>
> min/max patterns for intrinsics which on x86 result in the second
> input operand if the two operands are both zeros or one or both of them
> are a NaN shouldn't use SMIN/SMAX RTL, because that is similarly to
> MIN_EXPR/MAX_EXPR undefined what will be the result in those cases.
>
> The following patch adds an expander which uses either a new pattern with
> UNSPEC_IEEE_M{AX,IN} or use the S{MIN,MAX} representation of the same.
>
> Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?
>
> P.S. I have a patch to replace UNSPEC_IEEE_M{AX,IN} with IF_THEN_ELSE
> (except for the 3dNOW! PFMIN/MAX, those actually are documented to behave
> differently), but it actually doesn't improve anything much, as
> simplify_const_relational_operation nor simplify_ternary_operation aren't
> able to fold comparisons with two CONST_VECTOR operands or IF_THEN_ELSE
> with 3 CONST_VECTOR operands.
> So, maybe better approach will be to generic fold the builtins with constant
> arguments (maybe leaving NaNs to runtime).

I think it is still worth it to implement insn patterns with generic
RTXes instead of unspecs. Maybe some future improvement to generic RTX
simplification will be able to handle them.

>
> 2024-09-19  Uros Bizjak  <ubizjak@gmail.com>
>             Jakub Jelinek  <jakub@redhat.com>
>
>         PR target/116738
>         * config/i386/subst.md (mask_scalar_operand_arg34,
>         mask_scalar_expand_op3, round_saeonly_scalar_mask_arg3): New
>         subst attributes.
>         * config/i386/sse.md
>         (<sse>_vm<code><mode>3<mask_scalar_name><round_saeonly_scalar_name>):
>         Change from define_insn to define_expand, rename the old define_insn
>         to ...
>         (*<sse>_vm<code><mode>3<mask_scalar_name><round_saeonly_scalar_name>):
>         ... this.
>         (<sse>_ieee_vm<ieee_maxmin><mode>3<mask_scalar_name><round_saeonly_scalar_name>):
>         New define_insn.
>
>         * gcc.target/i386/sse-pr116738.c: New test.

OK, also for backports.

Thanks,
Uros.

>
> --- gcc/config/i386/subst.md.jj 2024-09-18 15:49:42.200791315 +0200
> +++ gcc/config/i386/subst.md    2024-09-19 12:32:51.048626421 +0200
> @@ -366,6 +366,8 @@ (define_subst_attr "mask_scalar_operand4
>  (define_subst_attr "mask_scalarcz_operand4" "mask_scalarcz" "" "%{%5%}%N4")
>  (define_subst_attr "mask_scalar4_dest_false_dep_for_glc_cond" "mask_scalar" "1" "operands[4] == CONST0_RTX(<MODE>mode)")
>  (define_subst_attr "mask_scalarc_dest_false_dep_for_glc_cond" "mask_scalarc" "1" "operands[3] == CONST0_RTX(V8HFmode)")
> +(define_subst_attr "mask_scalar_operand_arg34" "mask_scalar" "" ", operands[3], operands[4]")
> +(define_subst_attr "mask_scalar_expand_op3" "mask_scalar" "3" "5")
>
>  (define_subst "mask_scalar"
>    [(set (match_operand:SUBST_V 0)
> @@ -473,6 +475,7 @@ (define_subst_attr "round_saeonly_scalar
>  (define_subst_attr "round_saeonly_scalar_constraint" "round_saeonly_scalar" "vm" "v")
>  (define_subst_attr "round_saeonly_scalar_prefix" "round_saeonly_scalar" "vex" "evex")
>  (define_subst_attr "round_saeonly_scalar_nimm_predicate" "round_saeonly_scalar" "nonimmediate_operand" "register_operand")
> +(define_subst_attr "round_saeonly_scalar_mask_arg3" "round_saeonly_scalar" "" ", operands[<mask_scalar_expand_op3>]")
>
>  (define_subst "round_saeonly_scalar"
>    [(set (match_operand:SUBST_V 0)
> --- gcc/config/i386/sse.md.jj   2024-09-10 16:26:02.875151133 +0200
> +++ gcc/config/i386/sse.md      2024-09-19 12:43:31.693030695 +0200
> @@ -3333,7 +3333,27 @@ (define_insn "*ieee_<ieee_maxmin><mode>3
>            (const_string "*")))
>     (set_attr "mode" "<ssescalarmode>")])
>
> -(define_insn "<sse>_vm<code><mode>3<mask_scalar_name><round_saeonly_scalar_name>"
> +(define_expand "<sse>_vm<code><mode>3<mask_scalar_name><round_saeonly_scalar_name>"
> +  [(set (match_operand:VFH_128 0 "register_operand")
> +       (vec_merge:VFH_128
> +         (smaxmin:VFH_128
> +           (match_operand:VFH_128 1 "register_operand")
> +           (match_operand:VFH_128 2 "nonimmediate_operand"))
> +        (match_dup 1)
> +        (const_int 1)))]
> +  "TARGET_SSE"
> +{
> +  if (!flag_finite_math_only || flag_signed_zeros)
> +    {
> +      emit_insn (gen_<sse>_ieee_vm<maxmin_float><mode>3<mask_scalar_name><round_saeonly_scalar_name>
> +                (operands[0], operands[1], operands[2]
> +                 <mask_scalar_operand_arg34>
> +                 <round_saeonly_scalar_mask_arg3>));
> +      DONE;
> +    }
> +})
> +
> +(define_insn "*<sse>_vm<code><mode>3<mask_scalar_name><round_saeonly_scalar_name>"
>    [(set (match_operand:VFH_128 0 "register_operand" "=x,v")
>         (vec_merge:VFH_128
>           (smaxmin:VFH_128
> @@ -3348,6 +3368,25 @@ (define_insn "<sse>_vm<code><mode>3<mask
>    [(set_attr "isa" "noavx,avx")
>     (set_attr "type" "sse")
>     (set_attr "btver2_sse_attr" "maxmin")
> +   (set_attr "prefix" "<round_saeonly_scalar_prefix>")
> +   (set_attr "mode" "<ssescalarmode>")])
> +
> +(define_insn "<sse>_ieee_vm<ieee_maxmin><mode>3<mask_scalar_name><round_saeonly_scalar_name>"
> +  [(set (match_operand:VFH_128 0 "register_operand" "=x,v")
> +       (vec_merge:VFH_128
> +         (unspec:VFH_128
> +           [(match_operand:VFH_128 1 "register_operand" "0,v")
> +            (match_operand:VFH_128 2 "nonimmediate_operand" "xm,<round_saeonly_scalar_constraint>")]
> +           IEEE_MAXMIN)
> +        (match_dup 1)
> +        (const_int 1)))]
> +  "TARGET_SSE"
> +  "@
> +   <ieee_maxmin><ssescalarmodesuffix>\t{%2, %0|%0, %<iptr>2}
> +   v<ieee_maxmin><ssescalarmodesuffix>\t{<round_saeonly_scalar_mask_op3>%2, %1, %0<mask_scalar_operand3>|%0<mask_scalar_operand3>, %1, %<iptr>2<round_saeonly_scalar_mask_op3>}"
> +  [(set_attr "isa" "noavx,avx")
> +   (set_attr "type" "sse")
> +   (set_attr "btver2_sse_attr" "maxmin")
>     (set_attr "prefix" "<round_saeonly_scalar_prefix>")
>     (set_attr "mode" "<ssescalarmode>")])
>
> --- gcc/testsuite/gcc.target/i386/sse-pr116738.c.jj     2024-09-19 12:52:33.502681950 +0200
> +++ gcc/testsuite/gcc.target/i386/sse-pr116738.c        2024-09-19 12:54:20.938219741 +0200
> @@ -0,0 +1,28 @@
> +/* PR target/116738 */
> +/* { dg-do run } */
> +/* { dg-options "-O2 -msse" } */
> +/* { dg-require-effective-target sse } */
> +
> +#include "sse-check.h"
> +
> +static inline float
> +clamp (float f)
> +{
> +  __m128 v = _mm_set_ss (f);
> +  __m128 zero = _mm_setzero_ps ();
> +  __m128 greatest = _mm_set_ss (__FLT_MAX__);
> +  v = _mm_min_ss (v, greatest);
> +  v = _mm_max_ss (v, zero);
> +  return _mm_cvtss_f32 (v);
> +}
> +
> +static void
> +sse_test (void)
> +{
> +  float f = clamp (-0.0f);
> +  if (f != 0.0f || __builtin_signbitf (f))
> +    abort ();
> +  f = clamp (__builtin_nanf (""));
> +  if (__builtin_isnanf (f) || f != __FLT_MAX__)
> +    abort ();
> +}
>
>         Jakub
>
Jakub Jelinek Sept. 20, 2024, 6:32 a.m. UTC | #3
On Fri, Sep 20, 2024 at 08:01:58AM +0200, Richard Biener wrote:
> > P.S. I have a patch to replace UNSPEC_IEEE_M{AX,IN} with IF_THEN_ELSE
> > (except for the 3dNOW! PFMIN/MAX, those actually are documented to behave
> > differently), but it actually doesn't improve anything much, as
> > simplify_const_relational_operation nor simplify_ternary_operation aren't
> > able to fold comparisons with two CONST_VECTOR operands or IF_THEN_ELSE
> > with 3 CONST_VECTOR operands.
> > So, maybe better approach will be to generic fold the builtins with constant
> > arguments (maybe leaving NaNs to runtime).
> 
> It would be possible to fold them in the gimple folding hook to VEC_COND_EXPRs
> with the chance the min/max operation being lost when expanding to RTL.

Sure, but we don't actually pattern recognize 
typedef float v4sf __attribute__((vector_size (sizeof (4 * sizeof (float)))));

v4sf
foo (v4sf x, v4sf y)
{
  return x < y ? y : x;
}
back to maxpd etc.  So it wouldn't be an optimization in most cases, at
least until we do that, user was looking for such insn or better with _mm_max_ps...
Maybe we should.

For scalar ('-Dvector_size(x)=') this is currently matched in ce2.

Exception-wise, seems the insn raise Invalid on NaN input (either) and if y
is SNaN, actually propagate it rather than turn it into QNaN, so I think it
is actually an exact match for x < y ? y : x (or x > y ? y : x).

	Jakub
Richard Biener Sept. 20, 2024, 7:08 a.m. UTC | #4
On Fri, Sep 20, 2024 at 8:32 AM Jakub Jelinek <jakub@redhat.com> wrote:
>
> On Fri, Sep 20, 2024 at 08:01:58AM +0200, Richard Biener wrote:
> > > P.S. I have a patch to replace UNSPEC_IEEE_M{AX,IN} with IF_THEN_ELSE
> > > (except for the 3dNOW! PFMIN/MAX, those actually are documented to behave
> > > differently), but it actually doesn't improve anything much, as
> > > simplify_const_relational_operation nor simplify_ternary_operation aren't
> > > able to fold comparisons with two CONST_VECTOR operands or IF_THEN_ELSE
> > > with 3 CONST_VECTOR operands.
> > > So, maybe better approach will be to generic fold the builtins with constant
> > > arguments (maybe leaving NaNs to runtime).
> >
> > It would be possible to fold them in the gimple folding hook to VEC_COND_EXPRs
> > with the chance the min/max operation being lost when expanding to RTL.
>
> Sure, but we don't actually pattern recognize
> typedef float v4sf __attribute__((vector_size (sizeof (4 * sizeof (float)))));
>
> v4sf
> foo (v4sf x, v4sf y)
> {
>   return x < y ? y : x;
> }
> back to maxpd etc.

Interesting - we did with GCC 13 but it seems regress with GCC 14 here, even
when using -msse4.2.  But yeah, this is done at RTL expansion and quite fragile
and later the combine patterns are not good enough it seems even with
UNSPEC_BLENDV ...

>  So it wouldn't be an optimization in most cases, at
> least until we do that, user was looking for such insn or better with _mm_max_ps...
> Maybe we should.
>
> For scalar ('-Dvector_size(x)=') this is currently matched in ce2.
>
> Exception-wise, seems the insn raise Invalid on NaN input (either) and if y
> is SNaN, actually propagate it rather than turn it into QNaN, so I think it
> is actually an exact match for x < y ? y : x (or x > y ? y : x).

Yes, it is documented in the Intel ISA manual that way.

Richard.

>         Jakub
>
Richard Biener Sept. 20, 2024, 7:12 a.m. UTC | #5
On Fri, Sep 20, 2024 at 9:08 AM Richard Biener
<richard.guenther@gmail.com> wrote:
>
> On Fri, Sep 20, 2024 at 8:32 AM Jakub Jelinek <jakub@redhat.com> wrote:
> >
> > On Fri, Sep 20, 2024 at 08:01:58AM +0200, Richard Biener wrote:
> > > > P.S. I have a patch to replace UNSPEC_IEEE_M{AX,IN} with IF_THEN_ELSE
> > > > (except for the 3dNOW! PFMIN/MAX, those actually are documented to behave
> > > > differently), but it actually doesn't improve anything much, as
> > > > simplify_const_relational_operation nor simplify_ternary_operation aren't
> > > > able to fold comparisons with two CONST_VECTOR operands or IF_THEN_ELSE
> > > > with 3 CONST_VECTOR operands.
> > > > So, maybe better approach will be to generic fold the builtins with constant
> > > > arguments (maybe leaving NaNs to runtime).
> > >
> > > It would be possible to fold them in the gimple folding hook to VEC_COND_EXPRs
> > > with the chance the min/max operation being lost when expanding to RTL.
> >
> > Sure, but we don't actually pattern recognize
> > typedef float v4sf __attribute__((vector_size (sizeof (4 * sizeof (float)))));
> >
> > v4sf
> > foo (v4sf x, v4sf y)
> > {
> >   return x < y ? y : x;
> > }
> > back to maxpd etc.
>
> Interesting - we did with GCC 13 but it seems regress with GCC 14 here, even
> when using -msse4.2.  But yeah, this is done at RTL expansion and quite fragile
> and later the combine patterns are not good enough it seems even with
> UNSPEC_BLENDV ...

I have opened PR116787 for this.

> >  So it wouldn't be an optimization in most cases, at
> > least until we do that, user was looking for such insn or better with _mm_max_ps...
> > Maybe we should.
> >
> > For scalar ('-Dvector_size(x)=') this is currently matched in ce2.
> >
> > Exception-wise, seems the insn raise Invalid on NaN input (either) and if y
> > is SNaN, actually propagate it rather than turn it into QNaN, so I think it
> > is actually an exact match for x < y ? y : x (or x > y ? y : x).
>
> Yes, it is documented in the Intel ISA manual that way.
>
> Richard.
>
> >         Jakub
> >
diff mbox series

Patch

--- gcc/config/i386/subst.md.jj	2024-09-18 15:49:42.200791315 +0200
+++ gcc/config/i386/subst.md	2024-09-19 12:32:51.048626421 +0200
@@ -366,6 +366,8 @@  (define_subst_attr "mask_scalar_operand4
 (define_subst_attr "mask_scalarcz_operand4" "mask_scalarcz" "" "%{%5%}%N4")
 (define_subst_attr "mask_scalar4_dest_false_dep_for_glc_cond" "mask_scalar" "1" "operands[4] == CONST0_RTX(<MODE>mode)")
 (define_subst_attr "mask_scalarc_dest_false_dep_for_glc_cond" "mask_scalarc" "1" "operands[3] == CONST0_RTX(V8HFmode)")
+(define_subst_attr "mask_scalar_operand_arg34" "mask_scalar" "" ", operands[3], operands[4]")
+(define_subst_attr "mask_scalar_expand_op3" "mask_scalar" "3" "5")
 
 (define_subst "mask_scalar"
   [(set (match_operand:SUBST_V 0)
@@ -473,6 +475,7 @@  (define_subst_attr "round_saeonly_scalar
 (define_subst_attr "round_saeonly_scalar_constraint" "round_saeonly_scalar" "vm" "v")
 (define_subst_attr "round_saeonly_scalar_prefix" "round_saeonly_scalar" "vex" "evex")
 (define_subst_attr "round_saeonly_scalar_nimm_predicate" "round_saeonly_scalar" "nonimmediate_operand" "register_operand")
+(define_subst_attr "round_saeonly_scalar_mask_arg3" "round_saeonly_scalar" "" ", operands[<mask_scalar_expand_op3>]")
 
 (define_subst "round_saeonly_scalar"
   [(set (match_operand:SUBST_V 0)
--- gcc/config/i386/sse.md.jj	2024-09-10 16:26:02.875151133 +0200
+++ gcc/config/i386/sse.md	2024-09-19 12:43:31.693030695 +0200
@@ -3333,7 +3333,27 @@  (define_insn "*ieee_<ieee_maxmin><mode>3
 	   (const_string "*")))
    (set_attr "mode" "<ssescalarmode>")])
 
-(define_insn "<sse>_vm<code><mode>3<mask_scalar_name><round_saeonly_scalar_name>"
+(define_expand "<sse>_vm<code><mode>3<mask_scalar_name><round_saeonly_scalar_name>"
+  [(set (match_operand:VFH_128 0 "register_operand")
+	(vec_merge:VFH_128
+	  (smaxmin:VFH_128
+	    (match_operand:VFH_128 1 "register_operand")
+	    (match_operand:VFH_128 2 "nonimmediate_operand"))
+	 (match_dup 1)
+	 (const_int 1)))]
+  "TARGET_SSE"
+{
+  if (!flag_finite_math_only || flag_signed_zeros)
+    {
+      emit_insn (gen_<sse>_ieee_vm<maxmin_float><mode>3<mask_scalar_name><round_saeonly_scalar_name>
+		 (operands[0], operands[1], operands[2]
+		  <mask_scalar_operand_arg34>
+		  <round_saeonly_scalar_mask_arg3>));
+      DONE;
+    }
+})
+
+(define_insn "*<sse>_vm<code><mode>3<mask_scalar_name><round_saeonly_scalar_name>"
   [(set (match_operand:VFH_128 0 "register_operand" "=x,v")
 	(vec_merge:VFH_128
 	  (smaxmin:VFH_128
@@ -3348,6 +3368,25 @@  (define_insn "<sse>_vm<code><mode>3<mask
   [(set_attr "isa" "noavx,avx")
    (set_attr "type" "sse")
    (set_attr "btver2_sse_attr" "maxmin")
+   (set_attr "prefix" "<round_saeonly_scalar_prefix>")
+   (set_attr "mode" "<ssescalarmode>")])
+
+(define_insn "<sse>_ieee_vm<ieee_maxmin><mode>3<mask_scalar_name><round_saeonly_scalar_name>"
+  [(set (match_operand:VFH_128 0 "register_operand" "=x,v")
+	(vec_merge:VFH_128
+	  (unspec:VFH_128
+	    [(match_operand:VFH_128 1 "register_operand" "0,v")
+	     (match_operand:VFH_128 2 "nonimmediate_operand" "xm,<round_saeonly_scalar_constraint>")]
+	    IEEE_MAXMIN)
+	 (match_dup 1)
+	 (const_int 1)))]
+  "TARGET_SSE"
+  "@
+   <ieee_maxmin><ssescalarmodesuffix>\t{%2, %0|%0, %<iptr>2}
+   v<ieee_maxmin><ssescalarmodesuffix>\t{<round_saeonly_scalar_mask_op3>%2, %1, %0<mask_scalar_operand3>|%0<mask_scalar_operand3>, %1, %<iptr>2<round_saeonly_scalar_mask_op3>}"
+  [(set_attr "isa" "noavx,avx")
+   (set_attr "type" "sse")
+   (set_attr "btver2_sse_attr" "maxmin")
    (set_attr "prefix" "<round_saeonly_scalar_prefix>")
    (set_attr "mode" "<ssescalarmode>")])
 
--- gcc/testsuite/gcc.target/i386/sse-pr116738.c.jj	2024-09-19 12:52:33.502681950 +0200
+++ gcc/testsuite/gcc.target/i386/sse-pr116738.c	2024-09-19 12:54:20.938219741 +0200
@@ -0,0 +1,28 @@ 
+/* PR target/116738 */
+/* { dg-do run } */
+/* { dg-options "-O2 -msse" } */
+/* { dg-require-effective-target sse } */
+
+#include "sse-check.h"
+
+static inline float
+clamp (float f)
+{
+  __m128 v = _mm_set_ss (f);
+  __m128 zero = _mm_setzero_ps ();
+  __m128 greatest = _mm_set_ss (__FLT_MAX__);
+  v = _mm_min_ss (v, greatest);
+  v = _mm_max_ss (v, zero);
+  return _mm_cvtss_f32 (v);
+}
+
+static void
+sse_test (void)
+{
+  float f = clamp (-0.0f);
+  if (f != 0.0f || __builtin_signbitf (f))
+    abort ();
+  f = clamp (__builtin_nanf (""));
+  if (__builtin_isnanf (f) || f != __FLT_MAX__)
+    abort ();
+}