| Message ID | 87vang6y0j.fsf_-_@googlemail.com |
|---|---|
| State | New |
| Headers | show |
On Wed, Jun 28, 2017 at 3:29 PM, Richard Sandiford <rdsandiford@googlemail.com> wrote: > Richard Biener <richard.guenther@gmail.com> writes: >> On Mon, Jun 26, 2017 at 1:50 PM, Richard Sandiford >> <richard.sandiford@linaro.org> wrote: >>> Richard Biener <richard.guenther@gmail.com> writes: >>>> On Mon, Jun 26, 2017 at 1:14 PM, Richard Sandiford >>>> <richard.sandiford@linaro.org> wrote: >>>>> I don't think the problem is the lack of a cap. In the test case we >>>>> see that: >>>>> >>>>> 1. B is known at compile time to be X * vecsize + Y when considered in >>>>> isolation, because the base alignment derived from its DR_REF >= vecsize. >>>>> So DR_MISALIGNMENT (B) == Y. >>>>> >>>>> 2. A's misalignment wrt vecsize is not known at compile time when >>>>> considered in isolation, because no useful base alignment can be >>>>> derived from its DR_REF. (The DR_REF is to a plain int rather than >>>>> to a structure with a high alignment.) So DR_MISALIGNMENT (A) == -1. >>>>> >>>>> 3. A and B when considered as a pair trivially have the same misalignment >>>>> wrt vecsize, for the reasons above. >>>>> >>>>> Each of these results is individually correct. The problem is that the >>>>> assert is conflating two things: it's saying that if we know two datarefs >>>>> have the same misaligment, we must either be able to calculate a >>>>> compile-time misalignment for both datarefs in isolation, or we must >>>>> fail to calculate a compile-time misalignment for both datarefs in >>>>> isolation. That isn't true: it's valid to have situations in which the >>>>> compile-time misalignment is known for one dataref in isolation but not >>>>> for the other. >>>> >>>> True. So the assert should then become >>>> >>>> gcc_assert (! known_alignment_for_access_p (dr) >>>> || DR_MISALIGNMENT (dr) / dr_size == >>>> DR_MISALIGNMENT (dr_peel) / dr_peel_size); >>>> >>>> ? >>> >>> I think it would need to be: >>> >>> gcc_assert (!known_alignment_for_access_p (dr) >>> || !known_alignment_for_access_p (dr_peel) >>> || (DR_MISALIGNMENT (dr) / dr_size >>> == DR_MISALIGNMENT (dr_peel) / dr_peel_size)); >> >> I think for !known_alignment_for_access_p (dr_peel) the assert doesn't make >> any sense (DR_MISALIGNMENT is -1), so yes, you are right. >> >>> But yeah, that would work too. The idea with the assert in the patch was >>> that for unconditional references we probably *do* want to try to compute >>> the same compile-time misalignment, but for optimisation reasons rather >>> than correctness. Maybe that's more properly a gcc_checking_assert >>> though, since nothing goes wrong if it fails. So perhaps: >> >> We shouldn't have asserts for optimization reasons, even with checking >> IMHO. > > OK. > >>> gcc_checking_assert (DR_IS_CONDITIONAL_IN_STMT (dr) >>> || DR_IS_CONDITIONAL_IN_STMT (dr_peel) >>> || (known_alignment_for_access_p (dr) >>> == known_alignment_for_access_p (dr_peel))); >>> >>> as a follow-on assert. >>> >>> Should I split it into two patches, one to change the gcc_assert and >>> another to add the optimisation? >> >> Yes please. > > Here's the patch to relax the assert. I'll post the rest in a new thread. > > Tested as before. OK to install? Ok. Richard. > Thanks, > Richard > > > 2017-06-28 Richard Sandiford <richard.sandiford@linaro.org> > > gcc/ > PR tree-optimization/81136 > * tree-vect-data-refs.c (vect_update_misalignment_for_peel): Only > assert that two references with the same misalignment have the same > compile-time misalignment if those compile-time misalignments > are known. > > gcc/testsuite/ > PR tree-optimization/81136 > * gcc.dg/vect/pr81136.c: New test. > > Index: gcc/tree-vect-data-refs.c > =================================================================== > --- gcc/tree-vect-data-refs.c 2017-06-26 19:41:19.549571836 +0100 > +++ gcc/tree-vect-data-refs.c 2017-06-28 14:25:58.811888377 +0100 > @@ -906,8 +906,10 @@ vect_update_misalignment_for_peel (struc > { > if (current_dr != dr) > continue; > - gcc_assert (DR_MISALIGNMENT (dr) / dr_size == > - DR_MISALIGNMENT (dr_peel) / dr_peel_size); > + gcc_assert (!known_alignment_for_access_p (dr) > + || !known_alignment_for_access_p (dr_peel) > + || (DR_MISALIGNMENT (dr) / dr_size > + == DR_MISALIGNMENT (dr_peel) / dr_peel_size)); > SET_DR_MISALIGNMENT (dr, 0); > return; > } > Index: gcc/testsuite/gcc.dg/vect/pr81136.c > =================================================================== > --- /dev/null 2017-06-28 07:28:02.991792729 +0100 > +++ gcc/testsuite/gcc.dg/vect/pr81136.c 2017-06-28 14:25:58.810888422 +0100 > @@ -0,0 +1,16 @@ > +/* { dg-do compile } */ > + > +struct __attribute__((aligned (32))) > +{ > + char misaligner; > + int foo[100]; > + int bar[100]; > +} *a; > + > +void > +fn1 (int n) > +{ > + int *b = a->foo; > + for (int i = 0; i < n; i++) > + a->bar[i] = b[i]; > +}
Index: gcc/tree-vect-data-refs.c =================================================================== --- gcc/tree-vect-data-refs.c 2017-06-26 19:41:19.549571836 +0100 +++ gcc/tree-vect-data-refs.c 2017-06-28 14:25:58.811888377 +0100 @@ -906,8 +906,10 @@ vect_update_misalignment_for_peel (struc { if (current_dr != dr) continue; - gcc_assert (DR_MISALIGNMENT (dr) / dr_size == - DR_MISALIGNMENT (dr_peel) / dr_peel_size); + gcc_assert (!known_alignment_for_access_p (dr) + || !known_alignment_for_access_p (dr_peel) + || (DR_MISALIGNMENT (dr) / dr_size + == DR_MISALIGNMENT (dr_peel) / dr_peel_size)); SET_DR_MISALIGNMENT (dr, 0); return; } Index: gcc/testsuite/gcc.dg/vect/pr81136.c =================================================================== --- /dev/null 2017-06-28 07:28:02.991792729 +0100 +++ gcc/testsuite/gcc.dg/vect/pr81136.c 2017-06-28 14:25:58.810888422 +0100 @@ -0,0 +1,16 @@ +/* { dg-do compile } */ + +struct __attribute__((aligned (32))) +{ + char misaligner; + int foo[100]; + int bar[100]; +} *a; + +void +fn1 (int n) +{ + int *b = a->foo; + for (int i = 0; i < n; i++) + a->bar[i] = b[i]; +}