Re: [SLP] SLP vectorization: vectorize vector constructors

[Richard Biener <rguenther@suse.de>]

[-- Attachment #1: Type: text/plain, Size: 8937 bytes --]

On Wed, 30 Oct 2019, Joel Hutton wrote:

> On 15/10/2019 13:11, Richard Biener wrote:
>  >>  > You miss to check that CONSTRUCTOR_NELTS == TYPE_VECTOR_SUBPARTS
>  >>  > (we can have omitted trailing zeros).
>  >
>  > ^^^
>  >
>  > I don't see this being handled?  You give up on non-SSA names
>  > but not on the omitted trailing zeros.
> 
> I had thought checking the number of vectors produced would work.

But that's too late to fail.

> I've added that check.
> I'm slightly confused about what should be done for non-SSA names. 
> There's no scalar stmt to gather for a constant in a vector constructor.

For now simply give up.  What could be done is "compact" the
operands to vectorize to only contain SSA names, try to SLP
match and vectorize them and then combine the vectorized result
with the constant elements.

Not worth doing I guess unless it's sth regular like

 { a, b, c, d, 0, 0, 0, 0 }

or so.  But this can be handled as followup if necessary.

>  >
>  > You build a CONSTRUCTOR of vectors, thus
>  >
>  > _orig_ssa_1 = { vect_part1_2, vect_part2_3, ... };
> I've added code to do this, and a testcase which triggers it.

Great.

>  >
>  > +
>  > +         if (constructor)
>  > +           {
>  > +             SLP_INSTANCE_ROOT_STMT (new_instance) = stmt_info->stmt;
>  > +           }
>  > +         else
>  > +           SLP_INSTANCE_ROOT_STMT (new_instance) = NULL;
>  > +
>  >
>  > too much vertical space, no {} around single-stmt if clauses
> Fixed.
> 
>  >
>  >
>  > @@ -2725,6 +2760,10 @@ vect_bb_slp_scalar_cost (basic_block bb,
>  >                 stmt_vec_info use_stmt_info = vinfo->lookup_stmt
>  > (use_stmt);
>  >                 if (!use_stmt_info || !PURE_SLP_STMT (use_stmt_info))
>  >                   {
>  > +                   /* Check this is not a constructor that will be
>  > vectorized
>  > +                      away.  */
>  > +                   if (BB_VINFO_GROUPED_STORES (vinfo).contains
>  > (use_stmt_info))
>  > +                       continue;
>  >
>  > hmm, so why not set the slp type on SLP_INSTANCE_ROOT_STMT instead?
>  > In theory the stmt should be part of the SLP tree itself but that's
>  > probably too awkward to be made work at the moment ;)
> I did try this, but it was indeed very awkward to be made to work.
>
>  >
>  > vect_ssa_use_outside_bb and vect_slp_check_for_constructors are new
>  > functions which need comments.
> Fixed. I've also taken the 'vectorize the root' out into a separate 
> function.
> 
>  >
>  > +  /* For vector constructors, the same SSA name must be used to 
> maintain
>  > data
>  > +     flow into other basic blocks.  */
>  > +  if (instance->root == node && SLP_INSTANCE_ROOT_STMT (instance)
>  > +      && SLP_TREE_NUMBER_OF_VEC_STMTS (node) == 1
>  > +      && SLP_TREE_VEC_STMTS (node).exists ())
>  > +    {
>  >
>  > it should read
>  >
>  >   /* Vectorize the instance root.  */
>  >
>  > and be in vect_schedule_slp after the vect_schedule_slp_instance.
>  > As said above you need to handle SLP_TREE_NUMBER_OF_VEC_STMTS > 1,
>  > you also cannot simply do "nothing" here, "failing" vectorization
>  > (well, you probably can - DCE will remove the vectorized code - but
>  > at least if there were calls in the SLP tree they will be mangled
>  > by vectorization so the scalar code is wrecked).  SO it should be
>  >
>  >  if (SLP_INSTANCE_ROOT_STMT (instance))
>  >    .. you may not fail to replace the scalar root stmt here ..
>  >
> So what should be done in the case that CONSTRUCTOR_NELTS < 
> TYPE_VECTOR_SUBPARTS?

You have to fail SLP discovery, not code generation.  The check
you did above should have done this.
 
>  > +         if (CONSTRUCTOR_NELTS (rhs) == 0)
>  > +           vectorizable = false;
>  > +
>  >
>  > if you use continue; you can elide the 'vectorizable' variable.
> Done.
> 
>  >
>  > +         if (!vect_ssa_use_outside_bb (gimple_assign_lhs (stmt)))
>  > +           vectorizable = false;
>  > +
>  >
>  > why's that?  no comments that clarify ;)  The vector may be
>  > used as argument to a call or as source of a store.  So I'd simply
>  > remove this check (and the function).
> 
> Done. The thinking was that if the vector was used as a source of a 
> store the SLP tree would be built from the grouped store instead. Will 
> it not cause problems if both the grouped store and the vector 
> constructor are used to build SLP trees?
> 
> 
> 
> 2019-10-10  Joel Hutton  <Joel.Hutton@arm.com>
> 
>          * expr.c (store_constructor): Add case for constructor of vectors.

Why do you need this?  The vectorizer already creates such CTORs.  Any
testcase that you can show?

>          * tree-vect-slp.c (vect_analyze_slp_instance): Add case for vector constructors.
>          (vect_bb_slp_scalar_cost): Likewise.
>          (vect_slp_check_for_constructors): New function.
>          (vect_slp_analyze_bb_1): Add check for vector constructors.
>          (vect_schedule_slp_instance): Add case to fixup vector constructor stmt.
>          (vectorize_slp_instance_root_stmt): New function
>          * tree-vectorizer.h (SLP_INSTANCE_ROOT_STMT): New field.

+         SLP_INSTANCE_ROOT_STMT (new_instance) = constructor ? 
stmt_info->stmt\
+                                                 : NULL;

SLP_INSTANCE_ROOT_STMT should be a stmt_vec_info I guess...

@@ -2801,6 +2830,10 @@ vect_bb_slp_scalar_cost (basic_block bb,
                stmt_vec_info use_stmt_info = vinfo->lookup_stmt 
(use_stmt);
                if (!use_stmt_info || !PURE_SLP_STMT (use_stmt_info))
                  {
+                   /* Check this is not a constructor that will be 
vectorized
+                      away.  */
+                   if (BB_VINFO_GROUPED_STORES (vinfo).contains 
(use_stmt_info))
+                       continue;
                    (*life)[i] = true;

... which you then simply mark as PURE_SLP_STMT where we call
vect_mark_slp_stmts in vect_slp_analyze_bb_1.

I see you have the TYPE_VECTOR_SUBPARTS check still at transform
stage in vectorize_slp_instance_root_stmt, please simply move
it to vect_slp_check_for_constructors or to vect_analyze_slp_instance
where you have the other rejects (non-SSA names in the ctor).

That is, vectorize_slp_instance_root_stmt may not fail.

+bool
+vectorize_slp_instance_root_stmt (slp_tree node, slp_instance instance)
+{
+

extra newline

+  if (SLP_TREE_NUMBER_OF_VEC_STMTS (node) == 1)

the stmt replacing can be commonized between == 1 and > 1 cases.

   FOR_EACH_VEC_ELT (slp_instances, i, instance)
     {
+      slp_tree node = SLP_INSTANCE_TREE (instance);
       /* Schedule the tree of INSTANCE.  */
-      vect_schedule_slp_instance (SLP_INSTANCE_TREE (instance),
+      vect_schedule_slp_instance (node,
                                  instance, bst_map);
+
+      /* Vectorize the instance root.  */
+      if (instance->root == node && SLP_INSTANCE_ROOT_STMT (instance)
+         && SLP_TREE_VEC_STMTS (node).exists ())
+       if (!vectorize_slp_instance_root_stmt (node, instance))
+         {

instance->root == node is always true.  Likewise
SLP_TREE_VEC_STMTS (node).exists ().

@@ -4061,15 +4201,42 @@ vect_schedule_slp (vec_info *vinfo)
       if (is_a <loop_vec_info> (vinfo))

the ChangeLog doesn't mention this.  I guess this isn't necessary
unless you fail vectorizing late which you shouldn't.

diff --git a/gcc/tree-vectorizer.h b/gcc/tree-vectorizer.h
index 
56be28b0cc5a77412f996e70636b08d5b615813e..9f8419e4208b7d438ace41892022f93ebcadd019 
100644
--- a/gcc/tree-vectorizer.h
+++ b/gcc/tree-vectorizer.h
@@ -151,6 +151,10 @@ public:
   /* The root of SLP tree.  */
   slp_tree root;

+  /* For vector constructors, the constructor stmt that the SLP tree is 
built
+     from, NULL otherwise.  */
+  gimple *root_stmt;

as said, make this a stmt_vec_info

Thanks,
Richard.


> gcc/testsuite/ChangeLog:
> 
> 2019-10-10  Joel Hutton  <Joel.Hutton@arm.com>
> 
>          * gcc.dg/vect/bb-slp-40.c: New test.
>          * gcc.dg/vect/bb-slp-41.c: New test.
> 
> 

-- 
Richard Biener <rguenther@suse.de>
SUSE Software Solutions Germany GmbH, Maxfeldstrasse 5, 90409 Nuernberg,
Germany; GF: Felix Imendörffer; HRB 36809 (AG Nuernberg)