[v2] PR81136: ICE from inconsistent DR_MISALIGNMENTs

[Richard Sandiford <rdsandiford at googlemail dot com>]

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.

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