On Sat, 26 Oct 2019, Richard Sandiford wrote:
> Richard Biener <rguent...@suse.de> writes:
> > We have to check each operand for being in a pattern, not just the
> > first when avoiding build from scalars (we could possibly handle
> > the special case of some of them being the pattern stmt root, but
> > that would be a followup improvement).
> >
> > Bootstrap & regtest running on x86-64-unknown-linux-gnu.
> >
> > Richard.
> >
> > 2019-10-25 Richard Biener <rguent...@suse.de>
> >
> > PR tree-optimization/92222
> > * tree-vect-slp.c (_slp_oprnd_info::first_pattern): Remove.
> > (_slp_oprnd_info::second_pattern): Likewise.
> > (_slp_oprnd_info::any_pattern): New.
> > (vect_create_oprnd_info): Adjust.
> > (vect_get_and_check_slp_defs): Compute whether any stmt is
> > in a pattern.
> > (vect_build_slp_tree_2): Avoid building up a node from scalars
> > if any of the operand defs, not just the first, is in a pattern.
>
> Is recording this in vect_get_and_check_slp_defs enough? AIUI we don't
> get that far if vect_build_slp_tree_1 fails, but that doesn't prevent us
> from building the vector from scalars on return from vect_build_slp_tree.
But there we've got oprnd_info for the operands of that build. So yes,
I think so (for the correctness part). What I'm not sure is whether
we can do the scalar build if the stmts are at most the pattern root
(so we can use the vect_original_stmt def as scalar operand).
> Was wondering whether we should use a helper function that explicitly
> checks whether any stmt_info in the vec is a pattern statement, rather
> than computing it on the fly.
I don't think that's necessary here.
Richard.
> Thanks,
> Richard
>
> >
> > * gcc.dg/torture/pr92222.c: New testcase.
> >
> > Index: gcc/tree-vect-slp.c
> > ===================================================================
> > --- gcc/tree-vect-slp.c (revision 277441)
> > +++ gcc/tree-vect-slp.c (working copy)
> > @@ -177,8 +177,7 @@ typedef struct _slp_oprnd_info
> > stmt. */
> > tree first_op_type;
> > enum vect_def_type first_dt;
> > - bool first_pattern;
> > - bool second_pattern;
> > + bool any_pattern;
> > } *slp_oprnd_info;
> >
> >
> > @@ -199,8 +198,7 @@ vect_create_oprnd_info (int nops, int gr
> > oprnd_info->ops.create (group_size);
> > oprnd_info->first_dt = vect_uninitialized_def;
> > oprnd_info->first_op_type = NULL_TREE;
> > - oprnd_info->first_pattern = false;
> > - oprnd_info->second_pattern = false;
> > + oprnd_info->any_pattern = false;
> > oprnds_info.quick_push (oprnd_info);
> > }
> >
> > @@ -339,13 +337,11 @@ vect_get_and_check_slp_defs (vec_info *v
> > tree oprnd;
> > unsigned int i, number_of_oprnds;
> > enum vect_def_type dt = vect_uninitialized_def;
> > - bool pattern = false;
> > slp_oprnd_info oprnd_info;
> > int first_op_idx = 1;
> > unsigned int commutative_op = -1U;
> > bool first_op_cond = false;
> > bool first = stmt_num == 0;
> > - bool second = stmt_num == 1;
> >
> > if (gcall *stmt = dyn_cast <gcall *> (stmt_info->stmt))
> > {
> > @@ -418,13 +414,12 @@ again:
> > return -1;
> > }
> >
> > - if (second)
> > - oprnd_info->second_pattern = pattern;
> > + if (def_stmt_info && is_pattern_stmt_p (def_stmt_info))
> > + oprnd_info->any_pattern = true;
> >
> > if (first)
> > {
> > oprnd_info->first_dt = dt;
> > - oprnd_info->first_pattern = pattern;
> > oprnd_info->first_op_type = TREE_TYPE (oprnd);
> > }
> > else
> > @@ -1311,7 +1306,7 @@ vect_build_slp_tree_2 (vec_info *vinfo,
> > /* ??? Rejecting patterns this way doesn't work. We'd have to
> > do extra work to cancel the pattern so the uses see the
> > scalar version. */
> > - && !is_pattern_stmt_p (SLP_TREE_SCALAR_STMTS (child)[0]))
> > + && !oprnd_info->any_pattern)
> > {
> > slp_tree grandchild;
> >
> > @@ -1358,18 +1353,16 @@ vect_build_slp_tree_2 (vec_info *vinfo,
> > /* ??? Rejecting patterns this way doesn't work. We'd have to
> > do extra work to cancel the pattern so the uses see the
> > scalar version. */
> > - && !is_pattern_stmt_p (stmt_info))
> > + && !is_pattern_stmt_p (stmt_info)
> > + && !oprnd_info->any_pattern)
> > {
> > if (dump_enabled_p ())
> > dump_printf_loc (MSG_NOTE, vect_location,
> > "Building vector operands from scalars\n");
> > this_tree_size++;
> > - child = vect_create_new_slp_node (oprnd_info->def_stmts);
> > - SLP_TREE_DEF_TYPE (child) = vect_external_def;
> > - SLP_TREE_SCALAR_OPS (child) = oprnd_info->ops;
> > + child = vect_create_new_slp_node (oprnd_info->ops);
> > children.safe_push (child);
> > oprnd_info->ops = vNULL;
> > - oprnd_info->def_stmts = vNULL;
> > continue;
> > }
> >
> > @@ -1469,7 +1440,7 @@ vect_build_slp_tree_2 (vec_info *vinfo,
> > /* ??? Rejecting patterns this way doesn't work. We'd have
> > to do extra work to cancel the pattern so the uses see the
> > scalar version. */
> > - && !is_pattern_stmt_p (SLP_TREE_SCALAR_STMTS (child)[0]))
> > + && !oprnd_info->any_pattern)
> > {
> > unsigned int j;
> > slp_tree grandchild;
> > Index: gcc/testsuite/gcc.dg/torture/pr92222.c
> > ===================================================================
> > --- gcc/testsuite/gcc.dg/torture/pr92222.c (nonexistent)
> > +++ gcc/testsuite/gcc.dg/torture/pr92222.c (working copy)
> > @@ -0,0 +1,17 @@
> > +/* { dg-do compile } */
> > +/* { dg-additional-options "-ftree-vectorize" } */
> > +
> > +unsigned char *a;
> > +int b;
> > +void f();
> > +void c()
> > +{
> > + char *d;
> > + int e;
> > + for (; b; b++) {
> > + e = 7;
> > + for (; e >= 0; e--)
> > + *d++ = a[b] & 1 << e ? '1' : '0';
> > + }
> > + f();
> > +}
>
--
Richard Biener <rguent...@suse.de>
SUSE Software Solutions Germany GmbH, Maxfeldstrasse 5, 90409 Nuernberg,
Germany; GF: Felix Imendörffer; HRB 36809 (AG Nuernberg)
