Re: [PATCH] Fix PR tree-optimization/77550

[Richard Biener <rguenther@suse.de>]

On Tue, 20 Sep 2016, Bernd Edlinger wrote:

> On 09/20/16 09:41, Richard Biener wrote:
> > On Mon, 19 Sep 2016, Bernd Edlinger wrote:
> >
> >> On 09/19/16 11:25, Richard Biener wrote:
> >>> On Sun, 18 Sep 2016, Bernd Edlinger wrote:
> >>>
> >>>> Hi,
> >>>>
> >>>> this PR shows that in vectorizable_store and vectorizable_load
> >>>> as well, the vector access always uses the first dr as the alias
> >>>> type for the whole access.  But that is not right, if they are
> >>>> different types, like in this example.
> >>>>
> >>>> So I tried to replace all reference_alias_ptr_type (DR_REF (first_dr))
> >>>> by an alias type that is correct for all references in the whole
> >>>> access group.  With this patch we fall back to ptr_type_node, which
> >>>> can alias anything, if the group consists of different alias sets.
> >>>>
> >>>>
> >>>> Bootstrapped and reg-tested on x86_64-pc-linux-gnu.
> >>>> Is it OK for trunk and gcc-6-branch?
> >>>
> >>> +/* Function get_group_alias_ptr_type.
> >>> +
> >>> +   Return the alias type for the group starting at FIRST_STMT
> >>> +   containing GROUP_SIZE elements.  */
> >>> +
> >>> +static tree
> >>> +get_group_alias_ptr_type (gimple *first_stmt, int group_size)
> >>> +{
> >>> +  struct data_reference *first_dr, *next_dr;
> >>> +  gimple *next_stmt;
> >>> +  int i;
> >>> +
> >>> +  first_dr = STMT_VINFO_DATA_REF (vinfo_for_stmt (first_stmt));
> >>> +  next_stmt = GROUP_NEXT_ELEMENT (vinfo_for_stmt (first_stmt));
> >>> +  for (i = 1; i < group_size && next_stmt; i++)
> >>> +    {
> >>>
> >>>
> >>> there is no need to pass in group_size, it's enough to walk
> >>> GROUP_NEXT_ELEMENT until it becomes NULL.
> >>>
> >>> Ok with removing the redundant arg.
> >>>
> >>> Thanks,
> >>> Richard.
> >>>
> >>
> >> Hmmm, I'm afraid this needs one more iteration.
> >>
> >>
> >> I tried first to check if there are no stmts after the group_size
> >> and noticed there are cases when group_size is 0,
> >> for instance in gcc.dg/torture/pr36244.c.
> >>
> >> I think there is a bug in vectorizable_load, here:
> >>
> >>    if (grouped_load)
> >>      {
> >>        first_stmt = GROUP_FIRST_ELEMENT (stmt_info);
> >>         /* For SLP vectorization we directly vectorize a subchain
> >>            without permutation.  */
> >>         if (slp && ! SLP_TREE_LOAD_PERMUTATION (slp_node).exists ())
> >>           first_stmt = ;
> >>
> >>         group_size = GROUP_SIZE (vinfo_for_stmt (first_stmt));
> >>
> >>         = 0, and even worse:
> >>
> >>         group_gap_adj = vf * group_size - nunits * vec_num;
> >>
> >>         = -4 !
> >>
> >> apparently GROUP_SIZE is only valid on the GROUP_FIRST_ELEMENT,
> >
> > Yes.  I'm not sure group_size or group_gap_adj are used in the
> > slp && ! SLP_TREE_LOAD_PERMUTATION (slp_node).exists () case but moving
> > the computation up before we re-set first_stmt is probably a good idea.
> >
> >> while it may be 0 on SLP_TREE_SCALAR_STMTS (slp_node)[0]
> >>
> >> moving the GROUP_SIZE up before first_stmt is overwritten
> >> results in no different code....
> >
> > See above - it's eventually unused.  The load/store vectorization code
> > is quite twisted ;)
> >
> 
> Agreed.
> 
> Here is the new version of the patch:
> 
> Moved the goups_size up, and everything works fine.
> 
> Removed the parameter from get_group_alias_ptr_type.
> 
> I think in the case, where first_stmt is not set to
> GROUP_FIRST_ELEMENT (stmt_info) but directly to stmt,
> it is likely somewhere in a list, so it is not necessary
> to walk the GROUP_NEXT_ELEMENT, so I would like to call
> reference_alias_ptr_type directly in that case.
> 
> 
> Bootstrapped and reg-tested on x86_64-pc-linux-gnu.
> Is it OK for trunk and gcc-6 branch?

Ok for trunk and gcc-6 branch after a while.

Richard.