Patchwork Fix PR49518

login
register
mail settings
Submitter Richard Guenther
Date July 4, 2011, 12:30 p.m.
Message ID <alpine.LNX.2.00.1107041430130.810@zhemvz.fhfr.qr>
Download mbox | patch
Permalink /patch/103095/
State New
Headers show

Comments

Richard Guenther - July 4, 2011, 12:30 p.m.
On Mon, 4 Jul 2011, Ira Rosen wrote:

> 
> 
> Richard Guenther <rguenther@suse.de> wrote on 04/07/2011 02:38:50 PM:
> 
> > Handling of negative steps broke one of the many asserts in
> > the vectorizer.  The following patch drops one that I can't
> > make sense of.  I think all asserts need comments - especially
> > this one would, as I can't see why using vf is correct to
> > test against and not nelements (and why <= vf and not < vf).
> 
> There is an explanation 10 rows above the assert. It doesn't make sense to
> peel more than vf iterations (and not nelements, since for the case of
> multiple types it may help to align more data-refs - see the comment in the
> code). IIRC <= is for the case of aligned access, but I am not sure about
> that, so maybe you are right.
> 
> I don't see how it is related to negative steps.
> 
> I think that the real reason for this failure is that the loads are
> actually irrelevant (hence, vf=4 that doesn't take char loads into
> account), but we don't check that when we analyze data-refs. So, in my
> opinion, the proper fix will add such check.

The following also works for me:


does that look better or do you propose to clean the datarefs
vector from those references?

Thanks,
Richard.
Ira Rosen - July 5, 2011, 5:51 a.m.
Richard Guenther <rguenther@suse.de> wrote on 04/07/2011 03:30:59 PM:
> >
> > Richard Guenther <rguenther@suse.de> wrote on 04/07/2011 02:38:50 PM:
> >
> > > Handling of negative steps broke one of the many asserts in
> > > the vectorizer.  The following patch drops one that I can't
> > > make sense of.  I think all asserts need comments - especially
> > > this one would, as I can't see why using vf is correct to
> > > test against and not nelements (and why <= vf and not < vf).
> >
> > There is an explanation 10 rows above the assert. It doesn't make sense
to
> > peel more than vf iterations (and not nelements, since for the case of
> > multiple types it may help to align more data-refs - see the comment in
the
> > code). IIRC <= is for the case of aligned access, but I am not sure
about
> > that, so maybe you are right.
> >
> > I don't see how it is related to negative steps.
> >
> > I think that the real reason for this failure is that the loads are
> > actually irrelevant (hence, vf=4 that doesn't take char loads into
> > account), but we don't check that when we analyze data-refs. So, in my
> > opinion, the proper fix will add such check.
>
> The following also works for me:
>
> Index: tree-vect-data-refs.c
> ===================================================================
> --- tree-vect-data-refs.c       (revision 175802)
> +++ tree-vect-data-refs.c       (working copy)
> @@ -1495,6 +1495,9 @@ vect_enhance_data_refs_alignment (loop_v
>        stmt = DR_STMT (dr);
>        stmt_info = vinfo_for_stmt (stmt);
>
> +      if (!STMT_VINFO_RELEVANT (stmt_info))
> +       continue;
> +
>        /* For interleaving, only the alignment of the first access
>           matters.  */
>        if (STMT_VINFO_STRIDED_ACCESS (stmt_info)
>
> does that look better or do you propose to clean the datarefs
> vector from those references?

Well, this is certainly enough to fix the PR.
I am not sure if we can just remove these data-refs from the dependence
checks. After that all the alignment and access checks are at least
redundant.

Thanks,
Ira

>
> Thanks,
> Richard.

Patch

Index: tree-vect-data-refs.c
===================================================================
--- tree-vect-data-refs.c       (revision 175802)
+++ tree-vect-data-refs.c       (working copy)
@@ -1495,6 +1495,9 @@  vect_enhance_data_refs_alignment (loop_v
       stmt = DR_STMT (dr);
       stmt_info = vinfo_for_stmt (stmt);
 
+      if (!STMT_VINFO_RELEVANT (stmt_info))
+       continue;
+
       /* For interleaving, only the alignment of the first access
          matters.  */
       if (STMT_VINFO_STRIDED_ACCESS (stmt_info)