[PATCH v2 3/16]middle-end Add basic SLP pattern matching scaffolding.

[Tamar Christina]

Hi Richi,

This is a respin which includes the changes you requested.

Thanks,
Tamar

gcc/ChangeLog:

	* Makefile.in (tree-vect-slp-patterns.o): New.
	* doc/passes.texi: Update documentation.
	* tree-vect-slp.c (vect_print_slp_tree): Add new state.
	(vect_match_slp_patterns_2, vect_match_slp_patterns): New.
	(vect_analyze_slp): Call pattern matcher.
	* tree-vectorizer.h (enum _complex_operation):
	(class vect_pattern_match, class vect_pattern): New.
	* tree-vect-slp-patterns.c: New file.

> -----Original Message-----
> From: rguenther@c653.arch.suse.de <rguenther@c653.arch.suse.de> On
> Behalf Of Richard Biener
> Sent: Tuesday, September 29, 2020 10:42 AM
> To: Richard Sandiford <Richard.Sandiford@arm.com>
> Cc: Tamar Christina <Tamar.Christina@arm.com>; nd <nd@arm.com>; gcc-
> patches@gcc.gnu.org
> Subject: Re: [PATCH v2 3/16]middle-end Add basic SLP pattern matching
> scaffolding.
> 
> On Tue, 29 Sep 2020, Richard Sandiford wrote:
> 
> > Richard Biener <rguenther@suse.de> writes:
> > >> > > @@ -2192,6 +2378,17 @@ vect_analyze_slp_instance (vec_info
> *vinfo,
> > >> > >                               &tree_size, bst_map);
> > >> > >    if (node != NULL)
> > >> > >      {
> > >> > > +      /* Temporarily allow add_stmt calls again.  */
> > >> > > +      vinfo->stmt_vec_info_ro = false;
> > >> > > +
> > >> > > +      /* See if any patterns can be found in the constructed SLP tree
> > >> > > +        before we do any analysis on it.  */
> > >> > > +      vect_match_slp_patterns (node, vinfo, group_size,
> &max_nunits,
> > >> > > +                              matches, &npermutes, &tree_size,
> > >> > > + bst_map);
> > >> > > +
> > >> > > +      /* After this no more add_stmt calls are allowed.  */
> > >> > > +      vinfo->stmt_vec_info_ro = true;
> > >> > > +
> > >> > >
> > >> > > I think this is a bit early to match patterns - I'd defer it to
> > >> > > the point where all entries into the same SLP subgraph are
> > >> > > analyzed, thus somewhere at the end of vect_analyze_slp loop
> > >> > > over all instances and match patterns?  That way phases are more
> clearly separated.
> > >> >
> > >> > That would probably work, my only worry is that the SLP analysis
> > >> > itself may fail and bail out at
> > >> >
> > >> > 	  /* If the loads and stores can be handled with load/store-lane
> > >> > 	     instructions do not generate this SLP instance.  */
> > >> > 	  if (is_a <loop_vec_info> (vinfo)
> > >> > 	      && loads_permuted
> > >> > 	      && dr && vect_store_lanes_supported (vectype, group_size,
> > >> > false))
> > >
> > > Ah, that piece of code.  Yeah, I'm repeatedly running into it as
> > > well - it's a bad hack that stands in the way all the time :/
> >
> > At one point I was wondering about trying to drop the above, vectorise
> > with and without SLP, and then compare their costs, like for
> VECT_COMPARE_COSTS.
> > But that seemed like a dead end with the move to doing everything on
> > the SLP representation.
> 
> Yeah ... though even moving everything to the SLP representation will retain
> the issue since there it will be N group-size 1 SLP instances vs. 1 group-size N
> SLP instance.
> 
> > > I guess we should try moving this upward like to
> > > vect_analyze_loop_2 right before
> > >
> > >   /* Check the SLP opportunities in the loop, analyze and build SLP trees.
> > > */
> > >   ok = vect_analyze_slp (loop_vinfo, *n_stmts);
> > >   if (!ok)
> > >     return ok;
> > >
> > > and there check whether all grouped loads and stores can be handled
> > > with store-/load-lanes (and there are no SLP reduction chains?) in
> > > which case do not try to attempt SLP at all.  Because the testcases
> > > this check was supposed to change were all-load/store-lane or all
> > > SLP so the mixed case is probably not worth special casing.
> > >
> > > Since load-/store-lanes is an arm speciality I tried to only touch
> > > this fragile part with a ten-foot pole ;)  CCing Richard, if he acks
> > > the above I can produce a patch.
> >
> > Yeah, sounds good to me.  Probably also sorth checking whether the
> > likely_max iteration count is high enough to support group_size
> > vectors, if we have enough information to guess that.
> >
> > We could also get the gen* machinery to emit a macro that is true if
> > at least one load/store-lane pattern is defined, so that we can skip
> > the code for non-Arm targets.  I can do that as a follow-up.
> 
> I've had a second look and one complication is that we only elide the SLP
> node if any of the loads are permuted.  So if all loads/stores are unpermuted
> but load/store-lanes would work we'd keep the SLP node.
> 
> Of course without actually building the SLP node we don't know whether the
> loads will be permuted or not ...
> 
> But surely the current place for the check will cause some testcases to
> become hybrid vectorizations which is likely undesirable.
> 
> So we could move the check after all SLP discovery is completed and throw it
> all away if we can and should use load/store-lanes?
> But that might then not solve Tamars issue.
> 
> Richard.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: pr13507.patch
Type: text/x-diff
Size: 21972 bytes
Desc: pr13507.patch
URL: <https://gcc.gnu.org/pipermail/gcc-patches/attachments/20201103/3db1b56d/attachment-0001.bin>