Re: [v2] PR81136: ICE from inconsistent DR_MISALIGNMENTs

[Richard Biener <richard dot guenther at gmail dot com>]

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];
> +}