diff mbox series

[WIP] enfoce SLP_TREE_VECTYPE a bit

Message ID nycvar.YFH.7.76.2005191147070.4397@zhemvz.fhfr.qr
State New
Headers show
Series [WIP] enfoce SLP_TREE_VECTYPE a bit | expand

Commit Message

Richard Biener May 19, 2020, 9:50 a.m. UTC 
So I came up with this, yet another overload of vect_is_simple_use (ick).
But at least for the two functions I tackled it seems to be straight-forward.

I'll see to enforce a type on all invariants in vect_get_constant_vectors
and thus try to adjust all other vectorizable_* (but I'm sure I'll miss
some...).

Comments still welcome.  The patch below passes vect.exp testing on
x86_64-linux.

Thanks,
Richard.


This tries to enforce a set SLP_TREE_VECTYPE in vect_get_constant_vectors
and provides some infrastructure for setting it in the vectorizable_*
functions.

2020-05-19  Richard Biener  <rguenther@suse.de>

	* tree-vectorizer.h (vect_is_simple_use): New overload.
	(vect_maybe_update_slp_op_vectype): New.
	* tree-vect-stmts.c (vect_is_simple_use): New overload
	accessing operands of SLP vs. non-SLP operation transparently.
	(vect_maybe_update_slp_op_vectype): New function updating
	the possibly shared SLP operands vector type.
	(vectorizable_operation): Be a bit more SLP vs non-SLP agnostic
	using the new vect_is_simple_use overload;  update SLP invariant
	operand nodes vector type.
	(vectorizable_comparison): Likewise.

	* tree-vect-slp.c (vect_get_constant_vectors): Use
	SLP_TREE_VECTYPE when set, check it matches previous
	behavior.  Enforce SLP_TREE_VECTYPE for boolean vectors.
---
 gcc/tree-vect-slp.c   | 17 +++++++-
 gcc/tree-vect-stmts.c | 99 ++++++++++++++++++++++++++++++++++++++-----
 gcc/tree-vectorizer.h |  5 +++
 3 files changed, 108 insertions(+), 13 deletions(-)

Comments

Richard Sandiford May 19, 2020, 10:05 a.m. UTC | #1 
Richard Biener <rguenther@suse.de> writes:
> So I came up with this, yet another overload of vect_is_simple_use (ick).
> But at least for the two functions I tackled it seems to be straight-forward.
>
> I'll see to enforce a type on all invariants in vect_get_constant_vectors
> and thus try to adjust all other vectorizable_* (but I'm sure I'll miss
> some...).
>
> Comments still welcome.  The patch below passes vect.exp testing on
> x86_64-linux.

LGTM FWIW.  Just wondering...

> @@ -11710,6 +11735,58 @@ vect_is_simple_use (tree operand, vec_info *vinfo, enum vect_def_type *dt,
>    return true;
>  }
>  
> +/* Function vect_is_simple_use.
> +
> +   Same as vect_is_simple_use but determines the operand by operand
> +   position OPERAND from either STMT or SLP_NODE, filling in *OP
> +   and *SLP_DEF (when SLP_NODE is not NULL).  */
> +
> +bool
> +vect_is_simple_use (vec_info *vinfo, stmt_vec_info stmt, slp_tree slp_node,
> +		    unsigned operand, tree *op, slp_tree *slp_def,
> +		    enum vect_def_type *dt,
> +		    tree *vectype, stmt_vec_info *def_stmt_info_out)
> +{
> +  if (slp_node)
> +    {
> +      if (operand >= SLP_TREE_CHILDREN (slp_node).length ())
> +	return false;

...which case needs the return false?  Asserting for out-of-range
indices (like for !slp_node) feels like it would hide fewer bugs.

Thanks,
Richard

> +      slp_tree child = SLP_TREE_CHILDREN (slp_node)[operand];
> +      *slp_def = child;
> +      if (SLP_TREE_DEF_TYPE (child) == vect_internal_def)
> +	*op = gimple_get_lhs (SLP_TREE_SCALAR_STMTS (child)[0]->stmt);
> +      else
> +	*op = SLP_TREE_SCALAR_OPS (child)[0];
> +    }
> +  else
> +    {
> +      if (gassign *ass = dyn_cast <gassign *> (stmt->stmt))
> +	*op = gimple_op (ass, operand + 1);
> +      else if (gcall *call = dyn_cast <gcall *> (stmt->stmt))
> +	*op = gimple_call_arg (call, operand);
> +      else
> +	gcc_unreachable ();
> +    }
> +
> +  /* ???  We might want to update *vectype from *slp_def here though
> +     when sharing nodes this would prevent unsharing in the caller.  */
> +  return vect_is_simple_use (*op, vinfo, dt, vectype, def_stmt_info_out);
> +}
> +
> +/* If OP is not NULL and is external or constant update its vector
> +   type with VECTYPE.  Returns true if successful or false if not,
> +   for example when conflicting vector types are present.  */
> +
> +bool
> +vect_maybe_update_slp_op_vectype (slp_tree op, tree vectype)
> +{
> +  if (!op || SLP_TREE_DEF_TYPE (op) == vect_internal_def)
> +    return true;
> +  if (SLP_TREE_VECTYPE (op))
> +    return types_compatible_p (SLP_TREE_VECTYPE (op), vectype);
> +  SLP_TREE_VECTYPE (op) = vectype;
> +  return true;
> +}
>  
>  /* Function supportable_widening_operation
>  
> diff --git a/gcc/tree-vectorizer.h b/gcc/tree-vectorizer.h
> index 38a0a1d278b..5b4188d08a3 100644
> --- a/gcc/tree-vectorizer.h
> +++ b/gcc/tree-vectorizer.h
> @@ -1695,6 +1695,11 @@ extern bool vect_is_simple_use (tree, vec_info *, enum vect_def_type *,
>  extern bool vect_is_simple_use (tree, vec_info *, enum vect_def_type *,
>  				tree *, stmt_vec_info * = NULL,
>  				gimple ** = NULL);
> +extern bool vect_is_simple_use (vec_info *, stmt_vec_info, slp_tree,
> +				unsigned, tree *, slp_tree *,
> +				enum vect_def_type *,
> +				tree *, stmt_vec_info * = NULL);
> +extern bool vect_maybe_update_slp_op_vectype (slp_tree, tree);
>  extern bool supportable_widening_operation (vec_info *,
>  					    enum tree_code, stmt_vec_info,
>  					    tree, tree, enum tree_code *,
Richard Biener May 19, 2020, 10:29 a.m. UTC | #2 
On Tue, 19 May 2020, Richard Sandiford wrote:

> Richard Biener <rguenther@suse.de> writes:
> > So I came up with this, yet another overload of vect_is_simple_use (ick).
> > But at least for the two functions I tackled it seems to be straight-forward.
> >
> > I'll see to enforce a type on all invariants in vect_get_constant_vectors
> > and thus try to adjust all other vectorizable_* (but I'm sure I'll miss
> > some...).
> >
> > Comments still welcome.  The patch below passes vect.exp testing on
> > x86_64-linux.
> 
> LGTM FWIW.  Just wondering...
> 
> > @@ -11710,6 +11735,58 @@ vect_is_simple_use (tree operand, vec_info *vinfo, enum vect_def_type *dt,
> >    return true;
> >  }
> >  
> > +/* Function vect_is_simple_use.
> > +
> > +   Same as vect_is_simple_use but determines the operand by operand
> > +   position OPERAND from either STMT or SLP_NODE, filling in *OP
> > +   and *SLP_DEF (when SLP_NODE is not NULL).  */
> > +
> > +bool
> > +vect_is_simple_use (vec_info *vinfo, stmt_vec_info stmt, slp_tree slp_node,
> > +		    unsigned operand, tree *op, slp_tree *slp_def,
> > +		    enum vect_def_type *dt,
> > +		    tree *vectype, stmt_vec_info *def_stmt_info_out)
> > +{
> > +  if (slp_node)
> > +    {
> > +      if (operand >= SLP_TREE_CHILDREN (slp_node).length ())
> > +	return false;
> 
> ...which case needs the return false?  Asserting for out-of-range
> indices (like for !slp_node) feels like it would hide fewer bugs.

vectorizable_operation happily puts rhs1 from a load stmt to
vect_is_simple_use ... it has a "negative" list of things it
doesn't want to handle (ick) and goes through for all the rest.

I agree though, I'll see to adjust this (the particular case being
easy).

Richard.

> Thanks,
> Richard
> 
> > +      slp_tree child = SLP_TREE_CHILDREN (slp_node)[operand];
> > +      *slp_def = child;
> > +      if (SLP_TREE_DEF_TYPE (child) == vect_internal_def)
> > +	*op = gimple_get_lhs (SLP_TREE_SCALAR_STMTS (child)[0]->stmt);
> > +      else
> > +	*op = SLP_TREE_SCALAR_OPS (child)[0];
> > +    }
> > +  else
> > +    {
> > +      if (gassign *ass = dyn_cast <gassign *> (stmt->stmt))
> > +	*op = gimple_op (ass, operand + 1);
> > +      else if (gcall *call = dyn_cast <gcall *> (stmt->stmt))
> > +	*op = gimple_call_arg (call, operand);
> > +      else
> > +	gcc_unreachable ();
> > +    }
> > +
> > +  /* ???  We might want to update *vectype from *slp_def here though
> > +     when sharing nodes this would prevent unsharing in the caller.  */
> > +  return vect_is_simple_use (*op, vinfo, dt, vectype, def_stmt_info_out);
> > +}
> > +
> > +/* If OP is not NULL and is external or constant update its vector
> > +   type with VECTYPE.  Returns true if successful or false if not,
> > +   for example when conflicting vector types are present.  */
> > +
> > +bool
> > +vect_maybe_update_slp_op_vectype (slp_tree op, tree vectype)
> > +{
> > +  if (!op || SLP_TREE_DEF_TYPE (op) == vect_internal_def)
> > +    return true;
> > +  if (SLP_TREE_VECTYPE (op))
> > +    return types_compatible_p (SLP_TREE_VECTYPE (op), vectype);
> > +  SLP_TREE_VECTYPE (op) = vectype;
> > +  return true;
> > +}
> >  
> >  /* Function supportable_widening_operation
> >  
> > diff --git a/gcc/tree-vectorizer.h b/gcc/tree-vectorizer.h
> > index 38a0a1d278b..5b4188d08a3 100644
> > --- a/gcc/tree-vectorizer.h
> > +++ b/gcc/tree-vectorizer.h
> > @@ -1695,6 +1695,11 @@ extern bool vect_is_simple_use (tree, vec_info *, enum vect_def_type *,
> >  extern bool vect_is_simple_use (tree, vec_info *, enum vect_def_type *,
> >  				tree *, stmt_vec_info * = NULL,
> >  				gimple ** = NULL);
> > +extern bool vect_is_simple_use (vec_info *, stmt_vec_info, slp_tree,
> > +				unsigned, tree *, slp_tree *,
> > +				enum vect_def_type *,
> > +				tree *, stmt_vec_info * = NULL);
> > +extern bool vect_maybe_update_slp_op_vectype (slp_tree, tree);
> >  extern bool supportable_widening_operation (vec_info *,
> >  					    enum tree_code, stmt_vec_info,
> >  					    tree, tree, enum tree_code *,
>
diff mbox series

Patch

diff --git a/gcc/tree-vect-slp.c b/gcc/tree-vect-slp.c
index 69a2002717f..32e0b6677b3 100644
--- a/gcc/tree-vect-slp.c
+++ b/gcc/tree-vect-slp.c
@@ -3640,9 +3640,22 @@  vect_get_constant_vectors (vec_info *vinfo,
   tree stmt_vectype = STMT_VINFO_VECTYPE (stmt_vinfo);
   if (VECT_SCALAR_BOOLEAN_TYPE_P (TREE_TYPE (op))
       && vect_mask_constant_operand_p (vinfo, stmt_vinfo, op_num))
-    vector_type = truth_type_for (stmt_vectype);
+    {
+      /* We always want SLP_TREE_VECTYPE (op_node) here correctly set
+	 as first step.  */
+      vector_type = SLP_TREE_VECTYPE (op_node);
+      gcc_assert (vector_type
+		  && types_compatible_p (vector_type,
+					 truth_type_for (stmt_vectype)));
+    }
   else
-    vector_type = get_vectype_for_scalar_type (vinfo, TREE_TYPE (op), op_node);
+    {
+      vector_type = get_vectype_for_scalar_type (vinfo,
+						 TREE_TYPE (op), op_node);
+      gcc_assert (!SLP_TREE_VECTYPE (op_node)
+		  || types_compatible_p (SLP_TREE_VECTYPE (op_node),
+					 vector_type));
+    }
 
   poly_uint64 vf = 1;
   if (loop_vec_info loop_vinfo = dyn_cast <loop_vec_info> (vinfo))
diff --git a/gcc/tree-vect-stmts.c b/gcc/tree-vect-stmts.c
index 94f233e2e27..0daf70a4a68 100644
--- a/gcc/tree-vect-stmts.c
+++ b/gcc/tree-vect-stmts.c
@@ -5993,8 +5993,9 @@  vectorizable_operation (vec_info *vinfo,
       return false;
     }
 
-  op0 = gimple_assign_rhs1 (stmt);
-  if (!vect_is_simple_use (op0, vinfo, &dt[0], &vectype))
+  slp_tree slp_op0;
+  if (!vect_is_simple_use (vinfo, stmt_info, slp_node,
+			   0, &op0, &slp_op0, &dt[0], &vectype))
     {
       if (dump_enabled_p ())
         dump_printf_loc (MSG_MISSED_OPTIMIZATION, vect_location,
@@ -6043,10 +6044,11 @@  vectorizable_operation (vec_info *vinfo,
     return false;
 
   tree vectype2 = NULL_TREE, vectype3 = NULL_TREE;
+  slp_tree slp_op1 = NULL, slp_op2 = NULL;
   if (op_type == binary_op || op_type == ternary_op)
     {
-      op1 = gimple_assign_rhs2 (stmt);
-      if (!vect_is_simple_use (op1, vinfo, &dt[1], &vectype2))
+      if (!vect_is_simple_use (vinfo, stmt_info, slp_node,
+			       1, &op1, &slp_op1, &dt[1], &vectype2))
 	{
 	  if (dump_enabled_p ())
 	    dump_printf_loc (MSG_MISSED_OPTIMIZATION, vect_location,
@@ -6056,8 +6058,8 @@  vectorizable_operation (vec_info *vinfo,
     }
   if (op_type == ternary_op)
     {
-      op2 = gimple_assign_rhs3 (stmt);
-      if (!vect_is_simple_use (op2, vinfo, &dt[2], &vectype3))
+      if (!vect_is_simple_use (vinfo, stmt_info, slp_node,
+			       2, &op2, &slp_op2, &dt[2], &vectype3))
 	{
 	  if (dump_enabled_p ())
 	    dump_printf_loc (MSG_MISSED_OPTIMIZATION, vect_location,
@@ -6169,6 +6171,18 @@  vectorizable_operation (vec_info *vinfo,
 				   vectype, NULL);
 	}
 
+      /* Put types on constant and invariant SLP children.  */
+      if (slp_node
+	  && (!vect_maybe_update_slp_op_vectype (slp_op0, vectype)
+	      || !vect_maybe_update_slp_op_vectype (slp_op1, vectype)
+	      || !vect_maybe_update_slp_op_vectype (slp_op2, vectype)))
+	{
+	  if (dump_enabled_p ())
+	    dump_printf_loc (MSG_MISSED_OPTIMIZATION, vect_location,
+			     "incompatible vector types for invariants\n");
+	  return false;
+	}
+
       STMT_VINFO_TYPE (stmt_info) = op_vec_info_type;
       DUMP_VECT_SCOPE ("vectorizable_operation");
       vect_model_simple_cost (vinfo, stmt_info,
@@ -10555,13 +10569,13 @@  vectorizable_comparison (vec_info *vinfo,
   if (TREE_CODE_CLASS (code) != tcc_comparison)
     return false;
 
-  rhs1 = gimple_assign_rhs1 (stmt);
-  rhs2 = gimple_assign_rhs2 (stmt);
-
-  if (!vect_is_simple_use (rhs1, vinfo, &dts[0], &vectype1))
+  slp_tree slp_rhs1, slp_rhs2;
+  if (!vect_is_simple_use (vinfo, stmt_info, slp_node,
+			   0, &rhs1, &slp_rhs1, &dts[0], &vectype1))
     return false;
 
-  if (!vect_is_simple_use (rhs2, vinfo, &dts[1], &vectype2))
+  if (!vect_is_simple_use (vinfo, stmt_info, slp_node,
+			   1, &rhs2, &slp_rhs2, &dts[1], &vectype2))
     return false;
 
   if (vectype1 && vectype2
@@ -10654,6 +10668,17 @@  vectorizable_comparison (vec_info *vinfo,
 	    }
 	}
 
+      /* Put types on constant and invariant SLP children.  */
+      if (slp_node
+	  && (!vect_maybe_update_slp_op_vectype (slp_rhs1, vectype)
+	      || !vect_maybe_update_slp_op_vectype (slp_rhs2, vectype)))
+	{
+	  if (dump_enabled_p ())
+	    dump_printf_loc (MSG_MISSED_OPTIMIZATION, vect_location,
+			     "incompatible vector types for invariants\n");
+	  return false;
+	}
+
       STMT_VINFO_TYPE (stmt_info) = comparison_vec_info_type;
       vect_model_simple_cost (vinfo, stmt_info,
 			      ncopies * (1 + (bitop2 != NOP_EXPR)),
@@ -11710,6 +11735,58 @@  vect_is_simple_use (tree operand, vec_info *vinfo, enum vect_def_type *dt,
   return true;
 }
 
+/* Function vect_is_simple_use.
+
+   Same as vect_is_simple_use but determines the operand by operand
+   position OPERAND from either STMT or SLP_NODE, filling in *OP
+   and *SLP_DEF (when SLP_NODE is not NULL).  */
+
+bool
+vect_is_simple_use (vec_info *vinfo, stmt_vec_info stmt, slp_tree slp_node,
+		    unsigned operand, tree *op, slp_tree *slp_def,
+		    enum vect_def_type *dt,
+		    tree *vectype, stmt_vec_info *def_stmt_info_out)
+{
+  if (slp_node)
+    {
+      if (operand >= SLP_TREE_CHILDREN (slp_node).length ())
+	return false;
+      slp_tree child = SLP_TREE_CHILDREN (slp_node)[operand];
+      *slp_def = child;
+      if (SLP_TREE_DEF_TYPE (child) == vect_internal_def)
+	*op = gimple_get_lhs (SLP_TREE_SCALAR_STMTS (child)[0]->stmt);
+      else
+	*op = SLP_TREE_SCALAR_OPS (child)[0];
+    }
+  else
+    {
+      if (gassign *ass = dyn_cast <gassign *> (stmt->stmt))
+	*op = gimple_op (ass, operand + 1);
+      else if (gcall *call = dyn_cast <gcall *> (stmt->stmt))
+	*op = gimple_call_arg (call, operand);
+      else
+	gcc_unreachable ();
+    }
+
+  /* ???  We might want to update *vectype from *slp_def here though
+     when sharing nodes this would prevent unsharing in the caller.  */
+  return vect_is_simple_use (*op, vinfo, dt, vectype, def_stmt_info_out);
+}
+
+/* If OP is not NULL and is external or constant update its vector
+   type with VECTYPE.  Returns true if successful or false if not,
+   for example when conflicting vector types are present.  */
+
+bool
+vect_maybe_update_slp_op_vectype (slp_tree op, tree vectype)
+{
+  if (!op || SLP_TREE_DEF_TYPE (op) == vect_internal_def)
+    return true;
+  if (SLP_TREE_VECTYPE (op))
+    return types_compatible_p (SLP_TREE_VECTYPE (op), vectype);
+  SLP_TREE_VECTYPE (op) = vectype;
+  return true;
+}
 
 /* Function supportable_widening_operation
 
diff --git a/gcc/tree-vectorizer.h b/gcc/tree-vectorizer.h
index 38a0a1d278b..5b4188d08a3 100644
--- a/gcc/tree-vectorizer.h
+++ b/gcc/tree-vectorizer.h
@@ -1695,6 +1695,11 @@  extern bool vect_is_simple_use (tree, vec_info *, enum vect_def_type *,
 extern bool vect_is_simple_use (tree, vec_info *, enum vect_def_type *,
 				tree *, stmt_vec_info * = NULL,
 				gimple ** = NULL);
+extern bool vect_is_simple_use (vec_info *, stmt_vec_info, slp_tree,
+				unsigned, tree *, slp_tree *,
+				enum vect_def_type *,
+				tree *, stmt_vec_info * = NULL);
+extern bool vect_maybe_update_slp_op_vectype (slp_tree, tree);
 extern bool supportable_widening_operation (vec_info *,
 					    enum tree_code, stmt_vec_info,
 					    tree, tree, enum tree_code *,