| Message ID | alpine.LNX.2.00.1107051134220.810@zhemvz.fhfr.qr |
|---|---|
| State | New |
| Headers | show |
Richard Guenther <rguenther@suse.de> wrote on 05/07/2011 12:35:24 PM: > > On Tue, 5 Jul 2011, Ira Rosen wrote: > > > 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. > > Ok. The following is what I have tested for this PR and also for > PR49628. > > Bootstrapped and tested on x86_64-unknown-linux-gnu, ok for trunk? Yes. Thanks, Ira > > Thanks, > Richard. > > 2011-07-04 Richard Guenther <rguenther@suse.de> > > PR tree-optimization/49518 > PR tree-optimization/49628 > * tree-vect-data-refs.c (vect_enhance_data_refs_alignment): Skip > irrelevant and invariant data-references. > (vect_analyze_data_ref_access): For invariant loads clear the > group association. > > * g++.dg/torture/pr49628.C: New testcase. > * gcc.dg/torture/pr49518.c: Likewise. > > Index: gcc/testsuite/g++.dg/torture/pr49628.C > =================================================================== > *** gcc/testsuite/g++.dg/torture/pr49628.C (revision 0) > --- gcc/testsuite/g++.dg/torture/pr49628.C (revision 0) > *************** > *** 0 **** > --- 1,37 ---- > + /* { dg-do compile } */ > + > + #include <vector> > + > + template <int rank, int dim> class Tensor; > + template <int dim> class Tensor<1,dim> { > + public: > + explicit Tensor (const bool initialize = true); > + Tensor (const Tensor<1,dim> &); > + Tensor<1,dim> & operator = (const Tensor<1,dim> &); > + double values[(dim!=0) ? (dim) : 1]; > + }; > + template <int dim> > + inline Tensor<1,dim> & Tensor<1,dim>::operator = (const Tensor<1,dim> &p) > + { > + for (unsigned int i=0; i<dim; ++i) > + values[i] = p.values[i]; > + }; > + template <int dim> class Quadrature { > + public: > + const unsigned int n_quadrature_points; > + }; > + class MappingQ1 > + { > + class InternalData { > + public: > + std::vector<Tensor<1,3> > shape_derivatives; > + unsigned int n_shape_functions; > + }; > + void compute_data (const Quadrature<3> &quadrature, InternalData &data) > + const; > + }; > + void MappingQ1::compute_data (const Quadrature<3> &q, InternalData > &data) const > + { > + const unsigned int n_q_points = q.n_quadrature_points; > + data.shape_derivatives.resize(data.n_shape_functions * n_q_points); > + } > Index: gcc/testsuite/gcc.dg/torture/pr49518.c > =================================================================== > *** gcc/testsuite/gcc.dg/torture/pr49518.c (revision 0) > --- gcc/testsuite/gcc.dg/torture/pr49518.c (revision 0) > *************** > *** 0 **** > --- 1,19 ---- > + /* { dg-do compile } */ > + > + int a, b; > + struct S { unsigned int s, t, u; } c, d = { 0, 1, 0 }; > + > + void > + test (unsigned char z) > + { > + char e[] = {0, 0, 0, 0, 1}; > + for (c.s = 1; c.s; c.s++) > + { > + b = e[c.s]; > + if (a) > + break; > + b = z >= c.u; > + if (d.t) > + break; > + } > + } > Index: gcc/tree-vect-data-refs.c > =================================================================== > --- gcc/tree-vect-data-refs.c (revision 175802) > +++ gcc/tree-vect-data-refs.c (working copy) > @@ -1495,12 +1495,19 @@ 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) > && GROUP_FIRST_ELEMENT (stmt_info) != stmt) > continue; > > + /* For invariant accesses there is nothing to enhance. */ > + if (integer_zerop (DR_STEP (dr))) > + continue; > + > supportable_dr_alignment = vect_supportable_dr_alignment (dr, true); > do_peeling = vector_alignment_reachable_p (dr); > if (do_peeling) > @@ -2304,7 +2311,10 @@ vect_analyze_data_ref_access (struct dat > > /* Allow invariant loads in loops. */ > if (loop_vinfo && dr_step == 0) > - return DR_IS_READ (dr); > + { > + GROUP_FIRST_ELEMENT (vinfo_for_stmt (stmt)) = NULL; > + return DR_IS_READ (dr); > + } > > if (loop && nested_in_vect_loop_p (loop, stmt)) > {
On Tue, Jul 5, 2011 at 2:35 AM, Richard Guenther <rguenther@suse.de> wrote: > On Tue, 5 Jul 2011, Ira Rosen wrote: > >> 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. > > Ok. The following is what I have tested for this PR and also for > PR49628. > > Bootstrapped and tested on x86_64-unknown-linux-gnu, ok for trunk? > > Thanks, > Richard. > > 2011-07-04 Richard Guenther <rguenther@suse.de> > > PR tree-optimization/49518 > PR tree-optimization/49628 > * tree-vect-data-refs.c (vect_enhance_data_refs_alignment): Skip > irrelevant and invariant data-references. > (vect_analyze_data_ref_access): For invariant loads clear the > group association. > > * g++.dg/torture/pr49628.C: New testcase. > * gcc.dg/torture/pr49518.c: Likewise. > This caused: http://gcc.gnu.org/bugzilla/show_bug.cgi?id=50017
Index: gcc/tree-vect-data-refs.c =================================================================== --- gcc/tree-vect-data-refs.c (revision 175802) +++ gcc/tree-vect-data-refs.c (working copy) @@ -1495,12 +1495,19 @@ 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) && GROUP_FIRST_ELEMENT (stmt_info) != stmt) continue; + /* For invariant accesses there is nothing to enhance. */ + if (integer_zerop (DR_STEP (dr))) + continue; + supportable_dr_alignment = vect_supportable_dr_alignment (dr, true); do_peeling = vector_alignment_reachable_p (dr); if (do_peeling) @@ -2304,7 +2311,10 @@ vect_analyze_data_ref_access (struct dat /* Allow invariant loads in loops. */ if (loop_vinfo && dr_step == 0) - return DR_IS_READ (dr); + { + GROUP_FIRST_ELEMENT (vinfo_for_stmt (stmt)) = NULL; + return DR_IS_READ (dr); + } if (loop && nested_in_vect_loop_p (loop, stmt)) {