[v2] PR81136: ICE from inconsistent DR_MISALIGNMENTs
diff mbox

Message ID 87vang6y0j.fsf_-_@googlemail.com
State New
Headers show

Commit Message

Richard Sandiford June 28, 2017, 1:29 p.m. UTC
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?

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.

Comments

Richard Biener June 29, 2017, 10:44 a.m. UTC | #1
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];
> +}

Patch
diff mbox

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