Re: [16/n] Apply maximum nunits for BB SLP

[Richard Biener <richard.guenther@gmail.com>]

On Tue, Nov 5, 2019 at 3:09 PM Richard Sandiford
<Richard.Sandiford@arm.com> wrote:
>
> Richard Biener <richard.guenther@gmail.com> writes:
> > On Tue, Oct 29, 2019 at 6:05 PM Richard Sandiford
> > <richard.sandiford@arm.com> wrote:
> >>
> >> The BB vectoriser picked vector types in the same way as the loop
> >> vectoriser: it picked a vector mode/size for the region and then
> >> based all the vector types off that choice.  This meant we could
> >> end up trying to use vector types that had too many elements for
> >> the group size.
> >>
> >> The main part of this patch is therefore about passing the SLP
> >> group size down to routines like get_vectype_for_scalar_type and
> >> ensuring that each vector type in the SLP tree is chosen wrt the
> >> group size.  That part in itself is pretty easy and mechanical.
> >>
> >> The main warts are:
> >>
> >> (1) We normally pick a STMT_VINFO_VECTYPE for data references at an
> >>     early stage (vect_analyze_data_refs).  However, nothing in the
> >>     BB vectoriser relied on this, or on the min_vf calculated from it.
> >>     I couldn't see anything other than vect_recog_bool_pattern that
> >>     tried to access the vector type before the SLP tree is built.
> >
> > So can you not set STMT_VINFO_VECTYPE for data refs with BB vectorization
> > then?
>
> Yeah, the patch stops us from setting it during vect_analyze_data_refs.
> We still need to set it later when building the SLP tree, just like
> we do for other statements.
>
> >> (2) It's possible for the same statement to be used in the groups of
> >>     different sizes.  Taking the group size into account meant that
> >>     we could try to pick different vector types for the same statement.
> >
> > That only happens when we have multiple SLP instances though
> > (entries into the shared SLP graph).
>
> Yeah.
>
> > It probably makes sense to keep handling SLP instances sharing stmts
> > together for costing reasons but one issue is that for disjunct pieces
> > (in the same BB) disqualifying one cost-wise disqualifies all.  So at
> > some point during analysis (which should eventually cover more than a
> > single BB) we want to split the graph.  It probably doesn't help the
> > above case.
>
> Yeah, sounds like there are two issues: one with sharing stmt_vec_infos
> between multiple SLP nodes, and one with sharing SLP child nodes between
> multiple parent nodes.  (2) comes from the first, but I guess failing
> based on costs is more about the second.
>
> >>     This problem should go away with the move to doing everything on
> >>     SLP trees, where presumably we would attach the vector type to the
> >>     SLP node rather than the stmt_vec_info.  Until then, the patch just
> >>     uses a first-come, first-served approach.
> >
> > Yeah, I ran into not having vectype on SLP trees with invariants/externals
> > as well.  I suppose you didn't try simply adding that to the SLP tree
> > and pushing/popping it like we push/pop the def type?
>
> No, didn't try that.  Maybe it would be worth a go, but it seems like it
> could be a rabbit hole.
>
> > Assigning the vector types should really happen in vectorizable_*
> > and not during SLP build itself btw.
>
> Agree we need to improve the way this is handled, but delaying it
> to vectorizable_* sounds quite late.  Maybe it should be a more global
> decision, since the vector types for each vectorizable_* have to be
> compatible and it's not obvious which routine should get first choice.
>
> > Your update-all-shared-vectypes thing looks quadratic to me :/
>
> Should be amortised linear.  The statements in a DR group always
> have the same vectype.  When we want to change the vector type
> of one statement, we change it for all statements if possible
> or fail if we can't.

OK, let's go for it.

Thanks,
Richard.

> Thanks,
> Richard