Thanks Tamar and Richard for enlightening.
> I think you're doing the MIN_EXPR wrong - the above says MIN_EXPR
> <b_12(D), b_12(D)> which doesn't make
> sense anyway. I suspect you fail to put the MIN_EXPR to a separate statement?
Make sense, will have another try for this.
> Aye, you need to emit the additional statements through
> append_pattern_def_seq,
> This is also because the scalar statement doesn’t require them, so it makes
> costing easier.
> The vectorizer expects arguments to be simple use, so compound statements
> aren't
> Supported as they make costing and codegen harder.
Yes, you are right. It is not ssa_name during simple use check and then return
failures to vectorization_convertion.
Pan
-----Original Message-----
From: Tamar Christina <tamar.christ...@arm.com>
Sent: Friday, June 28, 2024 9:39 PM
To: Richard Biener <richard.guent...@gmail.com>; Li, Pan2 <pan2...@intel.com>
Cc: gcc-patches@gcc.gnu.org; juzhe.zh...@rivai.ai; kito.ch...@gmail.com;
jeffreya...@gmail.com; rdapp....@gmail.com
Subject: RE: [PATCH v3] Vect: Support truncate after .SAT_SUB pattern in zip
> -----Original Message-----
> From: Richard Biener <richard.guent...@gmail.com>
> Sent: Friday, June 28, 2024 6:39 AM
> To: Li, Pan2 <pan2...@intel.com>
> Cc: gcc-patches@gcc.gnu.org; juzhe.zh...@rivai.ai; kito.ch...@gmail.com;
> jeffreya...@gmail.com; rdapp....@gmail.com; Tamar Christina
> <tamar.christ...@arm.com>
> Subject: Re: [PATCH v3] Vect: Support truncate after .SAT_SUB pattern in zip
>
> On Thu, Jun 27, 2024 at 4:45 PM Li, Pan2 <pan2...@intel.com> wrote:
> >
> > Hi Richard,
> >
> > As mentioned by tamar in previous, would like to try even more optimization
> based on this patch.
> > Assume we take zip benchmark as example, we may have gimple similar as below
> >
> > unsigned int _1, _2;
> > unsigned short int _9;
> >
> > _9 = (unsigned short int).SAT_SUB (_1, _2);
> >
> > If we can locate the _1 is in the range of unsigned short, we can
> > distribute the
> convert into
> > the .SAT_SUB, aka:
> >
> > From:
> > _1 = (unsigned int short)_other;
> > _9 = (unsigned short int).SAT_SUB (_1, _2);
> >
> > To:
> > _9 = .SAT_SUB ((unsigned int short)_1, (unsigned int short)MIN_EXPR (_2,
> 65536)));
> >
> > Unfortunately, it failed to vectorize when I try to perform above changes.
> > The
> vectorizable_conversion
> > considers it is not simple use and then return fail to vect_analyze_loop_2.
> >
> > zip.test.c:15:12: note: ==> examining pattern def statement: patt_42 =
> > (short
> unsigned int) MIN_EXPR <b_12(D), b_12(D)>;
> > zip.test.c:15:12: note: ==> examining statement: patt_42 = (short
> > unsigned int)
> MIN_EXPR <b_12(D), b_12(D)>;
> > zip.test.c:15:12: note: vect_is_simple_use: operand MIN_EXPR <b_12(D),
> b_12(D)>, type of def: unknown
> > zip.test.c:15:12: missed: Unsupported pattern.
> > zip.test.c:15:12: missed: use not simple.
> > zip.test.c:15:12: note: vect_is_simple_use: operand MIN_EXPR <b_12(D),
> b_12(D)>, type of def: unknown
> > zip.test.c:15:12: missed: Unsupported pattern.
> > zip.test.c:15:12: missed: use not simple.
> > zip.test.c:15:12: note: vect_is_simple_use: operand MIN_EXPR <b_12(D),
> b_12(D)>, type of def: unknown
> > zip.test.c:15:12: missed: Unsupported pattern.
> > zip.test.c:15:12: missed: use not simple.
> > zip.test.c:7:6: missed: not vectorized: relevant stmt not supported:
> > patt_42 =
> (short unsigned int) MIN_EXPR <b_12(D), b_12(D)>;
> > zip.test.c:15:12: missed: bad operation or unsupported loop bound.
> >
> > I tried to take COND_EXPR here instead of MIN_EXPR but almost the same
> behavior. I am not sure if we can unblock this by the
> > vectorizable_conversion or we need some improvements from other pass.
>
> I think you're doing the MIN_EXPR wrong - the above says MIN_EXPR
> <b_12(D), b_12(D)> which doesn't make
> sense anyway. I suspect you fail to put the MIN_EXPR to a separate statement?
>
Aye, you need to emit the additional statements through append_pattern_def_seq,
This is also because the scalar statement doesn’t require them, so it makes
costing easier.
The vectorizer expects arguments to be simple use, so compound statements aren't
Supported as they make costing and codegen harder.
Cheers,
Tamar
> > Thanks a lot.
> >
> > Pan
> >
> > -----Original Message-----
> > From: Li, Pan2
> > Sent: Thursday, June 27, 2024 2:14 PM
> > To: Richard Biener <richard.guent...@gmail.com>
> > Cc: gcc-patches@gcc.gnu.org; juzhe.zh...@rivai.ai; kito.ch...@gmail.com;
> jeffreya...@gmail.com; rdapp....@gmail.com
> > Subject: RE: [PATCH v3] Vect: Support truncate after .SAT_SUB pattern in zip
> >
> > > OK
> >
> > Committed, thanks Richard.
> >
> > Pan
> >
> > -----Original Message-----
> > From: Richard Biener <richard.guent...@gmail.com>
> > Sent: Thursday, June 27, 2024 2:04 PM
> > To: Li, Pan2 <pan2...@intel.com>
> > Cc: gcc-patches@gcc.gnu.org; juzhe.zh...@rivai.ai; kito.ch...@gmail.com;
> jeffreya...@gmail.com; rdapp....@gmail.com
> > Subject: Re: [PATCH v3] Vect: Support truncate after .SAT_SUB pattern in zip
> >
> > On Thu, Jun 27, 2024 at 3:31 AM <pan2...@intel.com> wrote:
> > >
> > > From: Pan Li <pan2...@intel.com>
> >
> > OK
> >
> > > The zip benchmark of coremark-pro have one SAT_SUB like pattern but
> > > truncated as below:
> > >
> > > void test (uint16_t *x, unsigned b, unsigned n)
> > > {
> > > unsigned a = 0;
> > > register uint16_t *p = x;
> > >
> > > do {
> > > a = *--p;
> > > *p = (uint16_t)(a >= b ? a - b : 0); // Truncate after .SAT_SUB
> > > } while (--n);
> > > }
> > >
> > > It will have gimple before vect pass, it cannot hit any pattern of
> > > SAT_SUB and then cannot vectorize to SAT_SUB.
> > >
> > > _2 = a_11 - b_12(D);
> > > iftmp.0_13 = (short unsigned int) _2;
> > > _18 = a_11 >= b_12(D);
> > > iftmp.0_5 = _18 ? iftmp.0_13 : 0;
> > >
> > > This patch would like to improve the pattern match to recog above
> > > as truncate after .SAT_SUB pattern. Then we will have the pattern
> > > similar to below, as well as eliminate the first 3 dead stmt.
> > >
> > > _2 = a_11 - b_12(D);
> > > iftmp.0_13 = (short unsigned int) _2;
> > > _18 = a_11 >= b_12(D);
> > > iftmp.0_5 = (short unsigned int).SAT_SUB (a_11, b_12(D));
> > >
> > > The below tests are passed for this patch.
> > > 1. The rv64gcv fully regression tests.
> > > 2. The rv64gcv build with glibc.
> > > 3. The x86 bootstrap tests.
> > > 4. The x86 fully regression tests.
> > >
> > > gcc/ChangeLog:
> > >
> > > * match.pd: Add convert description for minus and capture.
> > > * tree-vect-patterns.cc (vect_recog_build_binary_gimple_call): Add
> > > new logic to handle in_type is incompatibile with out_type, as
> > > well as rename from.
> > > (vect_recog_build_binary_gimple_stmt): Rename to.
> > > (vect_recog_sat_add_pattern): Leverage above renamed func.
> > > (vect_recog_sat_sub_pattern): Ditto.
> > >
> > > Signed-off-by: Pan Li <pan2...@intel.com>
> > > ---
> > > gcc/match.pd | 4 +--
> > > gcc/tree-vect-patterns.cc | 51 ++++++++++++++++++++++++---------------
> > > 2 files changed, 33 insertions(+), 22 deletions(-)
> > >
> > > diff --git a/gcc/match.pd b/gcc/match.pd
> > > index cf8a399a744..820591a36b3 100644
> > > --- a/gcc/match.pd
> > > +++ b/gcc/match.pd
> > > @@ -3164,9 +3164,9 @@ DEFINE_INT_AND_FLOAT_ROUND_FN (RINT)
> > > /* Unsigned saturation sub, case 2 (branch with ge):
> > > SAT_U_SUB = X >= Y ? X - Y : 0. */
> > > (match (unsigned_integer_sat_sub @0 @1)
> > > - (cond^ (ge @0 @1) (minus @0 @1) integer_zerop)
> > > + (cond^ (ge @0 @1) (convert? (minus (convert1? @0) (convert1? @1)))
> integer_zerop)
> > > (if (INTEGRAL_TYPE_P (type) && TYPE_UNSIGNED (type)
> > > - && types_match (type, @0, @1))))
> > > + && TYPE_UNSIGNED (TREE_TYPE (@0)) && types_match (@0, @1))))
> > >
> > > /* Unsigned saturation sub, case 3 (branchless with gt):
> > > SAT_U_SUB = (X - Y) * (X > Y). */
> > > diff --git a/gcc/tree-vect-patterns.cc b/gcc/tree-vect-patterns.cc
> > > index cef901808eb..519d15f2a43 100644
> > > --- a/gcc/tree-vect-patterns.cc
> > > +++ b/gcc/tree-vect-patterns.cc
> > > @@ -4490,26 +4490,37 @@ vect_recog_mult_pattern (vec_info *vinfo,
> > > extern bool gimple_unsigned_integer_sat_add (tree, tree*, tree
> > > (*)(tree));
> > > extern bool gimple_unsigned_integer_sat_sub (tree, tree*, tree
> > > (*)(tree));
> > >
> > > -static gcall *
> > > -vect_recog_build_binary_gimple_call (vec_info *vinfo, gimple *stmt,
> > > +static gimple *
> > > +vect_recog_build_binary_gimple_stmt (vec_info *vinfo, stmt_vec_info
> stmt_info,
> > > internal_fn fn, tree *type_out,
> > > - tree op_0, tree op_1)
> > > + tree lhs, tree op_0, tree op_1)
> > > {
> > > tree itype = TREE_TYPE (op_0);
> > > - tree vtype = get_vectype_for_scalar_type (vinfo, itype);
> > > + tree otype = TREE_TYPE (lhs);
> > > + tree v_itype = get_vectype_for_scalar_type (vinfo, itype);
> > > + tree v_otype = get_vectype_for_scalar_type (vinfo, otype);
> > >
> > > - if (vtype != NULL_TREE
> > > - && direct_internal_fn_supported_p (fn, vtype, OPTIMIZE_FOR_BOTH))
> > > + if (v_itype != NULL_TREE && v_otype != NULL_TREE
> > > + && direct_internal_fn_supported_p (fn, v_itype, OPTIMIZE_FOR_BOTH))
> > > {
> > > gcall *call = gimple_build_call_internal (fn, 2, op_0, op_1);
> > > + tree in_ssa = vect_recog_temp_ssa_var (itype, NULL);
> > >
> > > - gimple_call_set_lhs (call, vect_recog_temp_ssa_var (itype, NULL));
> > > + gimple_call_set_lhs (call, in_ssa);
> > > gimple_call_set_nothrow (call, /* nothrow_p */ false);
> > > - gimple_set_location (call, gimple_location (stmt));
> > > + gimple_set_location (call, gimple_location (STMT_VINFO_STMT
> (stmt_info)));
> > > +
> > > + *type_out = v_otype;
> > >
> > > - *type_out = vtype;
> > > + if (types_compatible_p (itype, otype))
> > > + return call;
> > > + else
> > > + {
> > > + append_pattern_def_seq (vinfo, stmt_info, call, v_itype);
> > > + tree out_ssa = vect_recog_temp_ssa_var (otype, NULL);
> > >
> > > - return call;
> > > + return gimple_build_assign (out_ssa, NOP_EXPR, in_ssa);
> > > + }
> > > }
> > >
> > > return NULL;
> > > @@ -4541,13 +4552,13 @@ vect_recog_sat_add_pattern (vec_info *vinfo,
> stmt_vec_info stmt_vinfo,
> > >
> > > if (gimple_unsigned_integer_sat_add (lhs, ops, NULL))
> > > {
> > > - gcall *call = vect_recog_build_binary_gimple_call (vinfo,
> > > last_stmt,
> > > - IFN_SAT_ADD,
> > > type_out,
> > > - ops[0], ops[1]);
> > > - if (call)
> > > + gimple *stmt = vect_recog_build_binary_gimple_stmt (vinfo,
> > > stmt_vinfo,
> > > + IFN_SAT_ADD,
> > > type_out,
> > > + lhs, ops[0],
> > > ops[1]);
> > > + if (stmt)
> > > {
> > > vect_pattern_detected ("vect_recog_sat_add_pattern", last_stmt);
> > > - return call;
> > > + return stmt;
> > > }
> > > }
> > >
> > > @@ -4579,13 +4590,13 @@ vect_recog_sat_sub_pattern (vec_info *vinfo,
> stmt_vec_info stmt_vinfo,
> > >
> > > if (gimple_unsigned_integer_sat_sub (lhs, ops, NULL))
> > > {
> > > - gcall *call = vect_recog_build_binary_gimple_call (vinfo,
> > > last_stmt,
> > > - IFN_SAT_SUB,
> > > type_out,
> > > - ops[0], ops[1]);
> > > - if (call)
> > > + gimple *stmt = vect_recog_build_binary_gimple_stmt (vinfo,
> > > stmt_vinfo,
> > > + IFN_SAT_SUB,
> > > type_out,
> > > + lhs, ops[0],
> > > ops[1]);
> > > + if (stmt)
> > > {
> > > vect_pattern_detected ("vect_recog_sat_sub_pattern", last_stmt);
> > > - return call;
> > > + return stmt;
> > > }
> > > }
> > >
> > > --
> > > 2.34.1
> > >
