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

[Richard Sandiford <richard.sandiford@arm.com>]

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.

> 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.

Thanks,
Richard