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 ;)
