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.
