On Tue, Jul 16, 2024 at 3:22 PM Li, Pan2 <pan2...@intel.com> wrote:
>
> > I think that's a bug. Do you say __builtin_add_overflow fails to promote
> > (constant) arguments?
>
> I double checked the 022t.ssa pass for the __builtin_add_overflow operands
> tree type. It looks like that
> the 2 operands of .ADD_OVERFLOW has different tree types when one of them is
> constant.
> One is unsigned DI, and the other is int.
I think that's a bug (and a downside of internal-functions as they
have no prototype the type
verifier could work with).
That you see them in 022t.ssa means that either the frontend
mis-handles the builtin call parsing
or fold_builtin_arith_overflow which is responsible for the rewriting
to an internal function is
wrong.
I've CCed Jakub who added those.
I think we could add verification for internal functions in the set of
commutative_binary_fn_p, commutative_ternary_fn_p, associative_binary_fn_p
and possibly others where we can constrain argument and result types.
Richard.
> (gdb) call debug_gimple_stmt(stmt)
> _14 = .ADD_OVERFLOW (_4, 129);
> (gdb) call debug_tree (gimple_call_arg(stmt, 0))
> <ssa_name 0x7ffff6a0ddc8
> type <integer_type 0x7ffff6a437e0 long unsigned int sizes-gimplified
> public unsigned DI
> size <integer_cst 0x7ffff6a391b0 constant 64>
> unit-size <integer_cst 0x7ffff6a391c8 constant 8>
> align:64 warn_if_not_align:0 symtab:0 alias-set -1 canonical-type
> 0x7ffff6a437e0 precision:64 min <integer_cst 0x7ffff6a39480 0> max
> <integer_cst 0x7ffff6a03660 18446744073709551615>
> pointer_to_this <pointer_type 0x7ffff6a522a0>>
> visited
> def_stmt _4 = *_3;
> version:4>
> (gdb) call debug_tree (gimple_call_arg(stmt, 1))
> <integer_cst 0x7ffff6beac78 type <integer_type 0x7ffff6a435e8 int> constant
> 129>
> (gdb)
>
> Then we go to the vect pass, we can also see that the ops of .ADD_OVERFLOW
> has different tree types.
> As my understanding, here we should have unsigned DI for constant operands
>
> (gdb) layout src
> (gdb) list
> 506
> if (gimple_call_num_args (_c4) == 2)
> 507
> {
> 508
> tree _q40 = gimple_call_arg (_c4, 0);
> 509
> _q40 = do_valueize (valueize, _q40);
> 510
> tree _q41 = gimple_call_arg (_c4, 1);
> 511
> _q41 = do_valueize (valueize, _q41);
> 512
> if (integer_zerop (_q21))
> 513
> {
> 514
> if (integer_minus_onep (_p1))
> 515
> {
> (gdb) call debug_tree (_q40)
> <ssa_name 0x7ffff6a0ddc8
> type <integer_type 0x7ffff6a437e0 long unsigned int sizes-gimplified
> public unsigned DI
> size <integer_cst 0x7ffff6a391b0 constant 64>
> unit-size <integer_cst 0x7ffff6a391c8 constant 8>
> align:64 warn_if_not_align:0 symtab:0 alias-set -1 canonical-type
> 0x7ffff6a437e0 precision:64 min <integer_cst 0x7ffff6a39480 0> max
> <integer_cst 0x7ffff6a03660 18446744073709551615>
> pointer_to_this <pointer_type 0x7ffff6a522a0>>
> visited
> def_stmt _4 = *_3;
> version:4>
> (gdb) call debug_tree (_q41)
> <integer_cst 0x7ffff6beac78 type <integer_type 0x7ffff6a435e8 int> constant
> 129>
>
> Pan
>
> -----Original Message-----
> From: Richard Biener <richard.guent...@gmail.com>
> Sent: Wednesday, July 10, 2024 7:36 PM
> To: Li, Pan2 <pan2...@intel.com>
> Cc: gcc-patches@gcc.gnu.org; juzhe.zh...@rivai.ai; kito.ch...@gmail.com;
> tamar.christ...@arm.com; jeffreya...@gmail.com; rdapp....@gmail.com; Liu,
> Hongtao <hongtao....@intel.com>
> Subject: Re: [PATCH v1] Vect: Promote unsigned .SAT_ADD constant operand for
> vectorizable_call
>
> On Wed, Jul 10, 2024 at 11:28 AM <pan2...@intel.com> wrote:
> >
> > From: Pan Li <pan2...@intel.com>
> >
> > The .SAT_ADD has 2 operand and one of the operand may be INTEGER_CST.
> > For example _1 = .SAT_ADD (_2, 9) comes from below sample code.
> >
> > Form 3:
> > #define DEF_VEC_SAT_U_ADD_IMM_FMT_3(T, IMM) \
> > T __attribute__((noinline)) \
> > vec_sat_u_add_imm##IMM##_##T##_fmt_3 (T *out, T *in, unsigned limit) \
> > { \
> > unsigned i; \
> > T ret; \
> > for (i = 0; i < limit; i++) \
> > { \
> > out[i] = __builtin_add_overflow (in[i], IMM, &ret) ? -1 : ret; \
> > } \
> > }
> >
> > DEF_VEC_SAT_U_ADD_IMM_FMT_3(uint64_t, 9)
> >
> > It will failure to vectorize as the vectorizable_call will check the
> > operands is type_compatiable but the imm will be treated as unsigned
> > SImode from the perspective of tree.
>
> I think that's a bug. Do you say __builtin_add_overflow fails to promote
> (constant) arguments?
>
> > Aka
> >
> > uint64_t _1;
> > uint64_t _2;
> >
> > _1 = .SAT_ADD (_2, 9);
> >
> > The _1 and _2 are unsigned DImode, which is different to imm 9 unsigned
> > SImode, and then result in vectorizable_call fails. This patch would
> > like to promote the imm operand to the operand type mode of _2 if and
> > only if there is no precision/data loss. Aka convert the imm 9 to the
> > DImode for above example.
> >
> > The below test suites 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:
> >
> > * tree-vect-patterns.cc (vect_recog_promote_cst_to_unsigned): Add
> > new func impl to promote the imm tree to target type.
> > (vect_recog_sat_add_pattern): Peform the type promotion before
> > generate .SAT_ADD call.
> >
> > Signed-off-by: Pan Li <pan2...@intel.com>
> > ---
> > gcc/tree-vect-patterns.cc | 17 +++++++++++++++++
> > 1 file changed, 17 insertions(+)
> >
> > diff --git a/gcc/tree-vect-patterns.cc b/gcc/tree-vect-patterns.cc
> > index 86e893a1c43..e1013222b12 100644
> > --- a/gcc/tree-vect-patterns.cc
> > +++ b/gcc/tree-vect-patterns.cc
> > @@ -4527,6 +4527,20 @@ vect_recog_build_binary_gimple_stmt (vec_info
> > *vinfo, stmt_vec_info stmt_info,
> > return NULL;
> > }
> >
> > +static void
> > +vect_recog_promote_cst_to_unsigned (tree *op, tree type)
> > +{
> > + if (TREE_CODE (*op) != INTEGER_CST || !TYPE_UNSIGNED (type))
> > + return;
> > +
> > + unsigned precision = TYPE_PRECISION (type);
> > + wide_int type_max = wi::mask (precision, false, precision);
> > + wide_int op_cst_val = wi::to_wide (*op, precision);
> > +
> > + if (wi::leu_p (op_cst_val, type_max))
> > + *op = wide_int_to_tree (type, op_cst_val);
> > +}
> > +
> > /*
> > * Try to detect saturation add pattern (SAT_ADD), aka below gimple:
> > * _7 = _4 + _6;
> > @@ -4553,6 +4567,9 @@ vect_recog_sat_add_pattern (vec_info *vinfo,
> > stmt_vec_info stmt_vinfo,
> >
> > if (gimple_unsigned_integer_sat_add (lhs, ops, NULL))
> > {
> > + vect_recog_promote_cst_to_unsigned (&ops[0], TREE_TYPE (ops[1]));
> > + vect_recog_promote_cst_to_unsigned (&ops[1], TREE_TYPE (ops[0]));
> > +
> > gimple *stmt = vect_recog_build_binary_gimple_stmt (vinfo,
> > stmt_vinfo,
> > IFN_SAT_ADD,
> > type_out,
> > lhs, ops[0],
> > ops[1]);
> > --
> > 2.34.1
> >
