Re: [PATCH] Fix PR92222
On Sat, 26 Oct 2019, Richard Sandiford wrote:
> Richard Biener 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
> >
> > PR tree-optimization/9
> > * 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/pr9.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 (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
Re: [PATCH] Fix PR92222
Richard Biener 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
>
> PR tree-optimization/9
> * 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.
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.
Thanks,
Richard
>
> * gcc.dg/torture/pr9.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 (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/pr9.c
>
[PATCH] Fix PR92222
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
PR tree-optimization/9
* 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.
* gcc.dg/torture/pr9.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 (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/pr9.c
===
--- gcc/testsuite/gcc.dg/torture/pr9.c (nonexistent)
+++ gcc/testsuite/gcc.dg/torture/pr9.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();
+}
