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

On Thu, Aug 13, 2026 at 3:11 PM Richard Biener
<[email protected]> wrote:
>
> 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.

I will wait for your adjustments.   My minmax change triggered these
issues.   You can use my minmax change + tests to validate the
adjustments.

Thanks.

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



-- 
H.J.