Re: [PATCH v2] match.pd: Change the MIN/MAX narrowing to MIN/MAX + convert

On Wed, Aug 12, 2026 at 4:16 PM H.J. Lu <[email protected]> wrote:
>
> On Tue, Aug 11, 2026 at 8:09 PM Richard Biener
> <[email protected]> wrote:
> >
> > On Tue, Aug 11, 2026 at 2:06 PM Richard Biener
> > <[email protected]> wrote:
> > >
> > > On Tue, Aug 11, 2026 at 1:53 PM H.J. Lu <[email protected]> wrote:
> > > >
> > > > On Tue, Aug 11, 2026 at 7:20 PM Richard Biener
> > > > <[email protected]> wrote:
> > > > >
> > > > > On Tue, Aug 11, 2026 at 1:13 PM H.J. Lu <[email protected]> wrote:
> > > > > >
> > > > > > "(type) minmax ((wide_type) a, (wide_type) b) to minmax (a, b)" is limited
> > > > > > to the single use of the result. It doesn't support:
> > > > > >
> > > > > > typedef int v2si __attribute__((vector_size (8)));
> > > > > > typedef long long v2di __attribute__((vector_size (16)));
> > > > > >
> > > > > > v2si
> > > > > > func (v2si a, v2si b, v2di *p)
> > > > > > {
> > > > > >   v2di x = __builtin_convertvector (a, v2di);
> > > > > >   v2di y = __builtin_convertvector (b, v2di);
> > > > > >   v2di z = x < y ? x : y;
> > > > > >   *p = z;
> > > > > >   return __builtin_convertvector (z, v2si);
> > > > > > }
> > > > > >
> > > > > > Change it to
> > > > > >
> > > > > > minmax ((wide_type) a, (wide_type) b) -> (wide_type) minmax (a, b)
> > > > > >
> > > > > > instead and add "(type) ((wide_type) a) -> a" for integer types.  Now
> > > > > > we generate
> > > > > >
> > > > > > pminsd %xmm1, %xmm0
> > > > > > pmovsxdq %xmm0, %xmm1
> > > > > > movaps %xmm1, (%rdi)
> > > > > >
> > > > > > instead of
> > > > > >
> > > > > > pmovsxdq %xmm0, %xmm2
> > > > > > pmovsxdq %xmm1, %xmm1
> > > > > > movdqa %xmm2, %xmm0
> > > > > > movdqa %xmm2, %xmm3
> > > > > > pcmpgtq %xmm1, %xmm0
> > > > > > pblendvb %xmm0, %xmm1, %xmm3
> > > > > > movdqa %xmm3, %xmm0
> > > > > > movaps %xmm3, (%rdi)
> > > > > > shufps $232, %xmm3, %xmm0
> > > > > >
> > > > > > gcc/
> > > > >
> > > > >   (simplify
> > > > > -  (convert (minmax:c@4 (convert@2 @0) (convert@3 @1)))
> > > > > +  (minmax:c (convert@2 @0) (convert@3 @1))
> > > > >
> > > > > no need for :c on minmax
> > > >
> > > > Removed.
> > > >
> > > > >    (if (ANY_INTEGRAL_TYPE_P (type)
> > > > > -       && ANY_INTEGRAL_TYPE_P (TREE_TYPE (@2))
> > > > > -       && types_match (type, TREE_TYPE (@0))
> > > > > -       && types_match (type, TREE_TYPE (@1))
> > > > > -       && types_match (TREE_TYPE (@2), TREE_TYPE (@3))
> > > > > -       && element_precision (TREE_TYPE (@2)) > element_precision (type)
> > > > > -       && TYPE_UNSIGNED (TREE_TYPE (@2)) == TYPE_UNSIGNED (type)
> > > > > +       && ANY_INTEGRAL_TYPE_P (TREE_TYPE (@0))
> > > > > +       && types_match (type, TREE_TYPE (@2))
> > > > > +       && types_match (type, TREE_TYPE (@3))
> > > > >
> > > > > the last two are redundant
> > > >
> > > > Removed.
> > > >
> > > > > +       && types_match (TREE_TYPE (@0), TREE_TYPE (@1))
> > > > > +       && element_precision (TREE_TYPE (@0)) < element_precision (type)
> > > > > +       && TYPE_UNSIGNED (TREE_TYPE (@0)) == TYPE_UNSIGNED (type)
> > > > >
> > > > > +/* (type) ((wide_type) a) -> a.  */
> > > > > +(simplify
> > > > > + (convert (convert@1 @0))
> > > > > +  (if (ANY_INTEGRAL_TYPE_P (type)
> > > > > +       && ANY_INTEGRAL_TYPE_P (TREE_TYPE (@1))
> > > > > +       && types_match (type, TREE_TYPE (@0))
> > > > > +       && element_precision (type) < element_precision (TREE_TYPE (@1)))
> > > > > +   @0))
> > > > >
> > > > > two-level conversions are already handled elsewhere, no need to add a
> > > > > new pattern.
> > > >
> > > > Where is it handled?  Without it, I got
> > >
> > > It should be handled by
> > >
> > > /* Handle cases of two conversions in a row.  */
> > > (for ocvt (convert float fix_trunc)
> > >  (for icvt (convert float)
> > >   (simplify
> > >    (ocvt (icvt@1 @0))
> > >    (with
> > >     {
> > > ...
> > >
> > > Possibly
> > >
> > >     /* In addition to the cases of two conversions in a row
> > >        handled below, if we are converting something to its own
> > >        type via an object of identical or wider precision, neither
> > >        conversion is needed.  */
> > >     (if (((GIMPLE && useless_type_conversion_p (type, inside_type))
> > >           || (GENERIC
> > >               && TYPE_MAIN_VARIANT (type) == TYPE_MAIN_VARIANT (inside_type)))
> > >          && (((inter_int || inter_ptr) && final_int)
> > >              || (inter_float && final_float))
> > >          && inter_prec >= final_prec)
> > >      (ocvt @0))
> > >
> > > is too strict in that inter_int checks INTEGRAL_TYPE_P, not ANY_INTEGRAL_TYPE_P.
> > > To avoid adjusting everything I'd add inside_any_int, etc. variables, otherwise
> > > a conservative transform would be to use ANY_INTEGRAL_TYPE_P for
> > > inside_int, etc.
> > > and replace uses with inside_int && !inside_vec, omitting !inside_vec
> > > for cases we have
> > > convinced ourselves are fine.
> >
> > Just to say, inside_float and friends _do_ include vector float types
> > (and complex float types).
> > So consistency would ask for the use of ANY_INTEGRAL_TYPE and opting
> > out of vectors
> > (and complex?) explicitly where needed.
> >
>
> Here is the v2 patch.   There are no regressions on Linux/x86-64.

@@ -5856,7 +5853,10 @@ DEFINE_INT_AND_FLOAT_ROUND_FN (RINT)
     (if (((inter_int && inside_int) || (inter_float && inside_float))
         && (final_int || final_float)
         && inter_prec >= inside_prec
-        && (inter_float || inter_unsignedp == inside_unsignedp))
+        && (inter_float
+            || ((!inter_vec
+                 || target_supports_op_p (TREE_TYPE (@0), ocvt, optab_vector))
+                && inter_unsignedp == inside_unsignedp)))

as you are touching this - since the inter_float also includes vector types this
shows the patterns lack vector validation completely.  Instead (note
inside_vec == inter_vec == final_vec in all cases) this should be

       && (!inter_vec || <... vector support check ...>)

I'll note that target_suppoorts_op_p is not the correct check here as conversion
support of course depends on both the source and the destination type.  The
appropriate function to use should be

   supportable_convert_operation (ocvt, type, TREE_TYPE (@0), &vcode)

the last argument is redundant, I'll remove it.  Let me handle the conversion
pattern adjustments, the minmax part of the patch is OK.

Thanks,
Richard.

> --
> H.J.
> "(type) minmax ((wide_type) a, (wide_type) b) to minmax (a, b)" is limited
> to the single use of the result. It doesn't support:
>
> typedef int v2si __attribute__((vector_size (8)));
> typedef long long v2di __attribute__((vector_size (16)));
>
> v2si
> func (v2si a, v2si b, v2di *p)
> {
>   v2di x = __builtin_convertvector (a, v2di);
>   v2di y = __builtin_convertvector (b, v2di);
>   v2di z = x < y ? x : y;
>   *p = z;
>   return __builtin_convertvector (z, v2si);
> }
>
> Change it to
>
> minmax ((wide_type) a, (wide_type) b) -> (wide_type) minmax (a, b)
>
> instead and update "for ocvt (convert float fix_trunc)" to replace
> INTEGRAL_TYPE_P with ANY_INTEGRAL_TYPE_P to include vector int types
> when supported by target, matching float conditionals which include
> vector float types.  Now we generate
>
> pminsd %xmm1, %xmm0
> pmovsxdq %xmm0, %xmm1
> movaps %xmm1, (%rdi)
>
> instead of
>
> pmovsxdq %xmm0, %xmm2
> pmovsxdq %xmm1, %xmm1
> movdqa %xmm2, %xmm0
> movdqa %xmm2, %xmm3
> pcmpgtq %xmm1, %xmm0
> pblendvb %xmm0, %xmm1, %xmm3
> movdqa %xmm3, %xmm0
> movaps %xmm3, (%rdi)
> shufps $232, %xmm3, %xmm0
>
> gcc/
>
> PR middle-end/126784
> PR middle-end/126788
> * match.pd ((type) minmax ((wide_type) a, (wide_type) b)): Changed
> to ...
> (minmax ((wide_type) a, (wide_type) b)): This.
> (for ocvt (convert float fix_trunc)): Replace INTEGRAL_TYPE_P
> with ANY_INTEGRAL_TYPE_P and check if vector conversion is
> supported.
>
> gcc/testsuite/
>
> PR middle-end/126784
> PR middle-end/126788
> * g++.dg/tree-ssa/vec-narrow-1.C: Use -msse4 and require int128
> for x86.
> * g++.dg/tree-ssa/vec-narrow-minmax-2.C: Likewise.
> * g++.target/i386/pr126784-1.C: New test.
> * g++.target/i386/pr126784-2.C: Likewise.
> * gcc.target/i386/pr126784-1.c: Likewise.
> * gcc.target/i386/pr126784-2.c: Likewise.
> * gcc.target/i386/pr126784-3.c: Likewise.
> * gcc.target/i386/pr126784-4.c: Likewise.
> * gcc.target/i386/pr126784-5.c: Likewise.
> * gcc.target/i386/pr126784-6.c: Likewise.
> * gcc.target/i386/pr126788-1.c: Likewise.