On Wed, 20 Nov 2019, Richard Sandiford wrote:
> Richard Biener <rguent...@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?
OK.
Thanks,
Richard.
> > 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.sandif...@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)
>
--
Richard Biener <rguent...@suse.de>
SUSE Software Solutions Germany GmbH, Maxfeldstrasse 5, 90409 Nuernberg,
Germany; GF: Felix Imendörffer; HRB 36809 (AG Nuernberg)
