This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [SVE] PR86753


On Wed, Aug 28, 2019 at 11:02 AM Richard Sandiford
<richard.sandiford@arm.com> wrote:
>
> Prathamesh Kulkarni <prathamesh.kulkarni@linaro.org> writes:
> > On Tue, 27 Aug 2019 at 21:14, Richard Sandiford
> > <richard.sandiford@arm.com> wrote:
> >>
> >> Richard should have the final say, but some comments...
> >>
> >> Prathamesh Kulkarni <prathamesh.kulkarni@linaro.org> writes:
> >> > diff --git a/gcc/tree-vect-stmts.c b/gcc/tree-vect-stmts.c
> >> > index 1e2dfe5d22d..862206b3256 100644
> >> > --- a/gcc/tree-vect-stmts.c
> >> > +++ b/gcc/tree-vect-stmts.c
> >> > @@ -1989,17 +1989,31 @@ check_load_store_masking (loop_vec_info loop_vinfo, tree vectype,
> >> >
> >> >  static tree
> >> >  prepare_load_store_mask (tree mask_type, tree loop_mask, tree vec_mask,
> >> > -                      gimple_stmt_iterator *gsi)
> >> > +                      gimple_stmt_iterator *gsi, tree mask,
> >> > +                      cond_vmask_map_type *cond_to_vec_mask)
> >>
> >> "scalar_mask" might be a better name.  But maybe we should key off the
> >> vector mask after all, now that we're relying on the code having no
> >> redundancies.
> >>
> >> Passing the vinfo would be better than passing the cond_vmask_map_type
> >> directly.
> >>
> >> >  {
> >> >    gcc_assert (useless_type_conversion_p (mask_type, TREE_TYPE (vec_mask)));
> >> >    if (!loop_mask)
> >> >      return vec_mask;
> >> >
> >> >    gcc_assert (TREE_TYPE (loop_mask) == mask_type);
> >> > +
> >> > +  tree *slot = 0;
> >> > +  if (cond_to_vec_mask)
> >>
> >> The pointer should never be null in this context.
> > Disabling check for NULL results in segfault with cond_arith_4.c because we
> > reach prepare_load_store_mask via vect_schedule_slp, called from
> > here in vect_transform_loop:
> >  /* Schedule the SLP instances first, then handle loop vectorization
> >      below.  */
> >   if (!loop_vinfo->slp_instances.is_empty ())
> >     {
> >       DUMP_VECT_SCOPE ("scheduling SLP instances");
> >       vect_schedule_slp (loop_vinfo);
> >     }
> >
> > which is before bb processing loop.
>
> We want this optimisation to be applied to SLP too though.  Especially
> since non-SLP will be going away at some point.
>
> But as Richard says, the problem with SLP is that the statements aren't
> traversed in block order, so I guess we can't do the on-the-fly
> redundancy elimination there...

And the current patch AFAICS can generate wrong SSA for this reason.

> Maybe an alternative would be to record during the analysis phase which
> scalar conditions need which loop masks.  Statements that need a loop
> mask currently do:
>
>       vect_record_loop_mask (loop_vinfo, masks, ncopies, vectype);
>
> If we also pass the scalar condition, we can maintain a hash_set of
> <condition, ncopies> pairs, representing the conditions that have
> loop masks applied at some point in the vectorised code.  The COND_EXPR
> code can use that set to decide whether to apply the loop mask or not.

Yeah, that sounds better.

Note that I don't like the extra "helpers" in fold-const.c/h, they do not look
useful in general so put them into vectorizer private code.  The decomposing
also doesn't look too nice, instead prepare_load_store_mask could get
such decomposed representation - possibly quite natural with the suggestion
from Richard above.

Richard.

> Trying to avoid duplicate ANDs with the loop mask would then become a
> separate follow-on change.  Not sure whether it's worth it on its own.
>
> Thanks,
> Richard


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]