Record leader nodes in the SLP graph (PR92516)

[Richard Sandiford]

Richard Biener <rguenther@suse.de> writes:
> On Sat, 16 Nov 2019, Richard Sandiford wrote:
>> If stmt analysis failed for an SLP node, r278246 tried building the
>> node from scalars instead.  By that point we've already processed child
>> nodes (usually successfully) and might have made some of them the lead
>> nodes for their stmt list.  This is why we couldn't free the child nodes
>> when deciding to build from scalars:
>> 
>>   /* Don't remove and free the child nodes here, since they could be
>>      referenced by other structures.  The analysis and scheduling phases
>>      (need to) ignore child nodes of anything that isn't vect_internal_def.  */
>> 
>> The problem in this PR is that we (correctly) don't process the unused
>> child nodes again during the scheduling phase, which means that those
>> nodes won't become the leader again.  So some other node with same stmts
>> becomes the leader instead.  However, any internal-def child nodes of
>> this new leader won't have been processed during the analysis phase,
>> because we also (correctly) skip child nodes of non-leader nodes.
>> 
>> We talked on IRC about fixing this by sharing nodes between SLP
>> instances, so that it becomes a "proper" graph.  But that seems
>> like it could throw up some problems too, so I wanted to fix the
>> PR in a less invasive way first.
>> 
>> This patch therefore records the leader nodes during the analysis
>> phase and reuses that choice during scheduling.  When scheduling
>> a non-leader node, we first try to schedule the leader node and
>> then reuse its vector stmts.  At the moment, doing this means we
>> need to know the leader node's SLP instance, so the patch records
>> that in the SLP node too.
>> 
>> While there, I noticed that the two-operation handling returned
>> early without restoring the original def types.  That's really
>> an independent bug fix.
>
> Can you split that out please?

Sure, done as below.  Tested on aarch64-linux-gnu and x86_64-linux-gnu.
OK to install?

> For the rest I prefer the following which I am now fully testing
> on x86_64-unknown-linux-gnu (vect.exp testing went fine).  As
> discussed on IRC this makes the SLP graph nodes shared between
> SLP instances (which it has in fact been, just "virtual" via
> all the leader computations).  So SLP instances are now just
> the collection of entries into the directed SLP graph.

Looks great.  Obviously I chickened out too early :-)

Thanks,
Richard


2019-11-19  Richard Sandiford  <richard.sandiford@arm.com>

gcc/
	* tree-vect-slp.c (vect_schedule_slp_instance): Restore stmt
	def types for two-operation SLP.

Index: gcc/tree-vect-slp.c
===================================================================
--- gcc/tree-vect-slp.c	2019-11-20 09:48:57.145101295 +0000
+++ gcc/tree-vect-slp.c	2019-11-20 12:11:56.818216093 +0000
@@ -4165,6 +4165,7 @@ vect_schedule_slp_instance (slp_tree nod
 
   /* Handle two-operation SLP nodes by vectorizing the group with
      both operations and then performing a merge.  */
+  bool done_p = false;
   if (SLP_TREE_TWO_OPERATORS (node))
     {
       gassign *stmt = as_a <gassign *> (stmt_info->stmt);
@@ -4235,10 +4236,11 @@ vect_schedule_slp_instance (slp_tree nod
 	    }
 	  v0.release ();
 	  v1.release ();
-	  return;
+	  done_p = true;
 	}
     }
-  vect_transform_stmt (stmt_info, &si, node, instance);
+  if (!done_p)
+    vect_transform_stmt (stmt_info, &si, node, instance);
 
   /* Restore stmt def-types.  */
   FOR_EACH_VEC_ELT (SLP_TREE_CHILDREN (node), i, child)