Re: [PATCH] Fix PR tree-optimization/53636 (SLP generates invalid misaligned access)
On Tue, Jun 19, 2012 at 11:36 PM, Mikael Pettersson mi...@it.uu.se wrote: Richard Guenther writes: On Fri, Jun 15, 2012 at 5:00 PM, Ulrich Weigand uweig...@de.ibm.com wrote: Richard Guenther wrote: On Fri, Jun 15, 2012 at 3:13 PM, Ulrich Weigand uweig...@de.ibm.com wrote: However, there is a second case where we need to check every pass: if we're not actually vectorizing any loop, but are performing basic-block SLP. In this case, it would appear that we need the same check as described in the comment above, i.e. to verify that the stride is a multiple of the vector size. The patch below adds this check, and this indeed fixes the invalid access I was seeing in the test case (in the final assembler, we now get a vld1.16 instead of vldr). Tested on arm-linux-gnueabi with no regressions. OK for mainline? Ok. Thanks for the quick review; I've checked this in to mainline now. I just noticed that the test case also crashes on 4.7, but not on 4.6. Would a backport to 4.7 also be OK, once testing passes? Yes. Please leave it on mainline a few days to catch fallout from autotesters. This patch caused FAIL: gcc.dg/vect/bb-slp-16.c scan-tree-dump-times slp basic block vectorized using SLP 1 on sparc64-linux. Comparing the pre and post patch dumps for that file shows 22: vect_compute_data_ref_alignment: 22: misalign = 4 bytes of ref MEM[(unsigned int *)pout_90 + 28B] 22: vect_compute_data_ref_alignment: -22: force alignment of arr[i_87] -22: misalign = 0 bytes of ref arr[i_87] +22: SLP: step doesn't divide the vector-size. +22: Unknown alignment for access: arr (lots of stuff that's simply gone) -22: BASIC BLOCK VECTORIZED - -22: basic block vectorized using SLP +22: not vectorized: unsupported unaligned store.arr[i_87] +22: not vectorized: unsupported alignment in basic block. In this testcase the alignment of arr[i] should be irrelevant - it is not part of the stmts that are going to be vectorized. But of course this may be simply an odering issue in how we analyze data-references / statements in basic-block vectorization (thus we possibly did not yet declare the arr[i] = i statement as not taking part in the vectorization). The line -22: force alignment of arr[i_87] is odd, too - as said we do not need to touch arr when vectorizing the basic-block. Ulrich, can you look into this or do you want me to take a look here? Mikael - please open a bugreport for this. Thanks, Richard. /Mikael
Re: [PATCH] Fix PR tree-optimization/53636 (SLP generates invalid misaligned access)
Richard Guenther writes: On Tue, Jun 19, 2012 at 11:36 PM, Mikael Pettersson mi...@it.uu.se wrote: Richard Guenther writes: On Fri, Jun 15, 2012 at 5:00 PM, Ulrich Weigand uweig...@de.ibm.com wrote: Richard Guenther wrote: On Fri, Jun 15, 2012 at 3:13 PM, Ulrich Weigand uweig...@de.ibm.com wrote: However, there is a second case where we need to check every pass: if we're not actually vectorizing any loop, but are performing basic-block SLP. In this case, it would appear that we need the same check as described in the comment above, i.e. to verify that the stride is a multiple of the vector size. The patch below adds this check, and this indeed fixes the invalid access I was seeing in the test case (in the final assembler, we now get a vld1.16 instead of vldr). Tested on arm-linux-gnueabi with no regressions. OK for mainline? Ok. Thanks for the quick review; I've checked this in to mainline now. I just noticed that the test case also crashes on 4.7, but not on 4.6. Would a backport to 4.7 also be OK, once testing passes? Yes. Please leave it on mainline a few days to catch fallout from autotesters. This patch caused FAIL: gcc.dg/vect/bb-slp-16.c scan-tree-dump-times slp basic block vectorized using SLP 1 on sparc64-linux. Comparing the pre and post patch dumps for that file shows 22: vect_compute_data_ref_alignment: 22: misalign = 4 bytes of ref MEM[(unsigned int *)pout_90 + 28B] 22: vect_compute_data_ref_alignment: -22: force alignment of arr[i_87] -22: misalign = 0 bytes of ref arr[i_87] +22: SLP: step doesn't divide the vector-size. +22: Unknown alignment for access: arr (lots of stuff that's simply gone) -22: BASIC BLOCK VECTORIZED - -22: basic block vectorized using SLP +22: not vectorized: unsupported unaligned store.arr[i_87] +22: not vectorized: unsupported alignment in basic block. In this testcase the alignment of arr[i] should be irrelevant - it is not part of the stmts that are going to be vectorized. But of course this may be simply an odering issue in how we analyze data-references / statements in basic-block vectorization (thus we possibly did not yet declare the arr[i] = i statement as not taking part in the vectorization). The line -22: force alignment of arr[i_87] is odd, too - as said we do not need to touch arr when vectorizing the basic-block. Ulrich, can you look into this or do you want me to take a look here? Mikael - please open a bugreport for this. I opened PR53729 for this, with an update saying that powerpc64-linux also has this regression. /Mikael
Re: [PATCH] Fix PR tree-optimization/53636 (SLP generates invalid misaligned access)
Richard Guenther wrote: In this testcase the alignment of arr[i] should be irrelevant - it is not part of the stmts that are going to be vectorized. Agreed. But of course this may be simply an odering issue in how we analyze data-references / statements in basic-block vectorization (thus we possibly did not yet declare the arr[i] = i statement as not taking part in the vectorization). The line -22: force alignment of arr[i_87] is odd, too - as said we do not need to touch arr when vectorizing the basic-block. Ulrich, can you look into this or do you want me to take a look here? I'll have a look. Bye, Ulrich -- Dr. Ulrich Weigand GNU Toolchain for Linux on System z and Cell BE ulrich.weig...@de.ibm.com
Re: [PATCH] Fix PR tree-optimization/53636 (SLP generates invalid misaligned access)
Richard Guenther writes: On Fri, Jun 15, 2012 at 5:00 PM, Ulrich Weigand uweig...@de.ibm.com wrote: Richard Guenther wrote: On Fri, Jun 15, 2012 at 3:13 PM, Ulrich Weigand uweig...@de.ibm.com wrote: However, there is a second case where we need to check every pass: if we're not actually vectorizing any loop, but are performing basic-block SLP. In this case, it would appear that we need the same check as described in the comment above, i.e. to verify that the stride is a multiple of the vector size. The patch below adds this check, and this indeed fixes the invalid access I was seeing in the test case (in the final assembler, we now get a vld1.16 instead of vldr). Tested on arm-linux-gnueabi with no regressions. OK for mainline? Ok. Thanks for the quick review; I've checked this in to mainline now. I just noticed that the test case also crashes on 4.7, but not on 4.6. Would a backport to 4.7 also be OK, once testing passes? Yes. Please leave it on mainline a few days to catch fallout from autotesters. This patch caused FAIL: gcc.dg/vect/bb-slp-16.c scan-tree-dump-times slp basic block vectorized using SLP 1 on sparc64-linux. Comparing the pre and post patch dumps for that file shows 22: vect_compute_data_ref_alignment: 22: misalign = 4 bytes of ref MEM[(unsigned int *)pout_90 + 28B] 22: vect_compute_data_ref_alignment: -22: force alignment of arr[i_87] -22: misalign = 0 bytes of ref arr[i_87] +22: SLP: step doesn't divide the vector-size. +22: Unknown alignment for access: arr (lots of stuff that's simply gone) -22: BASIC BLOCK VECTORIZED - -22: basic block vectorized using SLP +22: not vectorized: unsupported unaligned store.arr[i_87] +22: not vectorized: unsupported alignment in basic block. /Mikael
Re: [PATCH] Fix PR tree-optimization/53636 (SLP generates invalid misaligned access)
On Fri, Jun 15, 2012 at 5:00 PM, Ulrich Weigand uweig...@de.ibm.com wrote: Richard Guenther wrote: On Fri, Jun 15, 2012 at 3:13 PM, Ulrich Weigand uweig...@de.ibm.com wrote: However, there is a second case where we need to check every pass: if we're not actually vectorizing any loop, but are performing basic-block SLP. In this case, it would appear that we need the same check as described in the comment above, i.e. to verify that the stride is a multiple of the vector size. The patch below adds this check, and this indeed fixes the invalid access I was seeing in the test case (in the final assembler, we now get a vld1.16 instead of vldr). Tested on arm-linux-gnueabi with no regressions. OK for mainline? Ok. Thanks for the quick review; I've checked this in to mainline now. I just noticed that the test case also crashes on 4.7, but not on 4.6. Would a backport to 4.7 also be OK, once testing passes? Yes. Please leave it on mainline a few days to catch fallout from autotesters. Thanks, Richard. Thanks, Ulrich -- Dr. Ulrich Weigand GNU Toolchain for Linux on System z and Cell BE ulrich.weig...@de.ibm.com
[PATCH] Fix PR tree-optimization/53636 (SLP generates invalid misaligned access)
Hello,
PR tree-optimization/53636 is a crash due to an invalid unaligned access
generated by the vectorizer.
The problem is that vect_compute_data_ref_alignment uses DR_ALIGNED_TO
as computed by the default data-ref analysis to decide whether an access
is sufficiently aligned for the vectorizer.
However, this analysis computes the scalar evolution relative to the
innermost loop in which the access takes place; DR_ALIGNED_TO only
reflects the known alignmnent of the *base* address according to
that evolution.
Now, if we're actually about to vectorize this particular loop, then
just checking the alignment of the base is exactly what we need to do
(subsequent accesses will usually be misaligned, but that's OK since
we're transforming those into a vector access).
However, if we're actually currently vectorizing something else, this
test is not sufficient. The code currently already checks for the
case where we're performing outer-loop vectorization. In this case,
we need to check alignment of the access on *every* pass through the
inner loop, as the comment states:
/* In case the dataref is in an inner-loop of the loop that is being
vectorized (LOOP), we use the base and misalignment information
relative to the outer-loop (LOOP). This is ok only if the misalignment
stays the same throughout the execution of the inner-loop, which is why
we have to check that the stride of the dataref in the inner-loop evenly
divides by the vector size. */
However, there is a second case where we need to check every pass: if
we're not actually vectorizing any loop, but are performing basic-block
SLP. In this case, it would appear that we need the same check as
described in the comment above, i.e. to verify that the stride is a
multiple of the vector size.
The patch below adds this check, and this indeed fixes the invalid access
I was seeing in the test case (in the final assembler, we now get a vld1.16
instead of vldr).
Tested on arm-linux-gnueabi with no regressions.
OK for mainline?
Bye,
Ulrich
ChangeLog:
gcc/
PR tree-optimization/53636
* tree-vect-data-refs.c (vect_compute_data_ref_alignment): Verify
stride when doing basic-block vectorization.
gcc/testsuite/
PR tree-optimization/53636
* gcc.target/arm/pr53636.c: New test.
=== added file 'gcc/testsuite/gcc.target/arm/pr53636.c'
--- gcc/testsuite/gcc.target/arm/pr53636.c 1970-01-01 00:00:00 +
+++ gcc/testsuite/gcc.target/arm/pr53636.c 2012-06-11 17:31:41 +
@@ -0,0 +1,48 @@
+/* { dg-do run } */
+/* { dg-require-effective-target arm_neon_hw } */
+/* { dg-options -O -ftree-vectorize } */
+/* { dg-add-options arm_neon } */
+
+void fill (short *buf) __attribute__ ((noinline));
+void fill (short *buf)
+{
+ int i;
+
+ for (i = 0; i 11 * 8; i++)
+buf[i] = i;
+}
+
+void test (unsigned char *dst) __attribute__ ((noinline));
+void test (unsigned char *dst)
+{
+ short tmp[11 * 8], *tptr;
+ int i;
+
+ fill (tmp);
+
+ tptr = tmp;
+ for (i = 0; i 8; i++)
+{
+ dst[0] = (-tptr[0] + 9 * tptr[0 + 1] + 9 * tptr[0 + 2] - tptr[0 + 3])
7;
+ dst[1] = (-tptr[1] + 9 * tptr[1 + 1] + 9 * tptr[1 + 2] - tptr[1 + 3])
7;
+ dst[2] = (-tptr[2] + 9 * tptr[2 + 1] + 9 * tptr[2 + 2] - tptr[2 + 3])
7;
+ dst[3] = (-tptr[3] + 9 * tptr[3 + 1] + 9 * tptr[3 + 2] - tptr[3 + 3])
7;
+ dst[4] = (-tptr[4] + 9 * tptr[4 + 1] + 9 * tptr[4 + 2] - tptr[4 + 3])
7;
+ dst[5] = (-tptr[5] + 9 * tptr[5 + 1] + 9 * tptr[5 + 2] - tptr[5 + 3])
7;
+ dst[6] = (-tptr[6] + 9 * tptr[6 + 1] + 9 * tptr[6 + 2] - tptr[6 + 3])
7;
+ dst[7] = (-tptr[7] + 9 * tptr[7 + 1] + 9 * tptr[7 + 2] - tptr[7 + 3])
7;
+
+ dst += 8;
+ tptr += 11;
+}
+}
+
+int main (void)
+{
+ char buf [8 * 8];
+
+ test (buf);
+
+ return 0;
+}
+
=== modified file 'gcc/tree-vect-data-refs.c'
--- gcc/tree-vect-data-refs.c 2012-05-31 08:46:10 +
+++ gcc/tree-vect-data-refs.c 2012-06-11 17:31:41 +
@@ -845,6 +845,24 @@
}
}
+ /* Similarly, if we're doing basic-block vectorization, we can only use
+ base and misalignment information relative to an innermost loop if the
+ misalignment stays the same throughout the execution of the loop.
+ As above, this is the case if the stride of the dataref evenly divides
+ by the vector size. */
+ if (!loop)
+{
+ tree step = DR_STEP (dr);
+ HOST_WIDE_INT dr_step = TREE_INT_CST_LOW (step);
+
+ if (dr_step % GET_MODE_SIZE (TYPE_MODE (vectype)) != 0)
+ {
+ if (vect_print_dump_info (REPORT_ALIGNMENT))
+ fprintf (vect_dump, SLP: step doesn't divide the vector-size.);
+ misalign = NULL_TREE;
+ }
+}
+
base = build_fold_indirect_ref (base_addr);
alignment = ssize_int (TYPE_ALIGN (vectype)/BITS_PER_UNIT);
--
Dr. Ulrich Weigand
GNU Toolchain for Linux on System z and Cell BE
ulrich.weig...@de.ibm.com
Re: [PATCH] Fix PR tree-optimization/53636 (SLP generates invalid misaligned access)
On Fri, Jun 15, 2012 at 3:13 PM, Ulrich Weigand uweig...@de.ibm.com wrote:
Hello,
PR tree-optimization/53636 is a crash due to an invalid unaligned access
generated by the vectorizer.
The problem is that vect_compute_data_ref_alignment uses DR_ALIGNED_TO
as computed by the default data-ref analysis to decide whether an access
is sufficiently aligned for the vectorizer.
However, this analysis computes the scalar evolution relative to the
innermost loop in which the access takes place; DR_ALIGNED_TO only
reflects the known alignmnent of the *base* address according to
that evolution.
Now, if we're actually about to vectorize this particular loop, then
just checking the alignment of the base is exactly what we need to do
(subsequent accesses will usually be misaligned, but that's OK since
we're transforming those into a vector access).
However, if we're actually currently vectorizing something else, this
test is not sufficient. The code currently already checks for the
case where we're performing outer-loop vectorization. In this case,
we need to check alignment of the access on *every* pass through the
inner loop, as the comment states:
/* In case the dataref is in an inner-loop of the loop that is being
vectorized (LOOP), we use the base and misalignment information
relative to the outer-loop (LOOP). This is ok only if the misalignment
stays the same throughout the execution of the inner-loop, which is why
we have to check that the stride of the dataref in the inner-loop evenly
divides by the vector size. */
However, there is a second case where we need to check every pass: if
we're not actually vectorizing any loop, but are performing basic-block
SLP. In this case, it would appear that we need the same check as
described in the comment above, i.e. to verify that the stride is a
multiple of the vector size.
The patch below adds this check, and this indeed fixes the invalid access
I was seeing in the test case (in the final assembler, we now get a vld1.16
instead of vldr).
Tested on arm-linux-gnueabi with no regressions.
OK for mainline?
Ok.
Thanks,
Richard.
Bye,
Ulrich
ChangeLog:
gcc/
PR tree-optimization/53636
* tree-vect-data-refs.c (vect_compute_data_ref_alignment): Verify
stride when doing basic-block vectorization.
gcc/testsuite/
PR tree-optimization/53636
* gcc.target/arm/pr53636.c: New test.
=== added file 'gcc/testsuite/gcc.target/arm/pr53636.c'
--- gcc/testsuite/gcc.target/arm/pr53636.c 1970-01-01 00:00:00 +
+++ gcc/testsuite/gcc.target/arm/pr53636.c 2012-06-11 17:31:41 +
@@ -0,0 +1,48 @@
+/* { dg-do run } */
+/* { dg-require-effective-target arm_neon_hw } */
+/* { dg-options -O -ftree-vectorize } */
+/* { dg-add-options arm_neon } */
+
+void fill (short *buf) __attribute__ ((noinline));
+void fill (short *buf)
+{
+ int i;
+
+ for (i = 0; i 11 * 8; i++)
+ buf[i] = i;
+}
+
+void test (unsigned char *dst) __attribute__ ((noinline));
+void test (unsigned char *dst)
+{
+ short tmp[11 * 8], *tptr;
+ int i;
+
+ fill (tmp);
+
+ tptr = tmp;
+ for (i = 0; i 8; i++)
+ {
+ dst[0] = (-tptr[0] + 9 * tptr[0 + 1] + 9 * tptr[0 + 2] - tptr[0 + 3])
7;
+ dst[1] = (-tptr[1] + 9 * tptr[1 + 1] + 9 * tptr[1 + 2] - tptr[1 + 3])
7;
+ dst[2] = (-tptr[2] + 9 * tptr[2 + 1] + 9 * tptr[2 + 2] - tptr[2 + 3])
7;
+ dst[3] = (-tptr[3] + 9 * tptr[3 + 1] + 9 * tptr[3 + 2] - tptr[3 + 3])
7;
+ dst[4] = (-tptr[4] + 9 * tptr[4 + 1] + 9 * tptr[4 + 2] - tptr[4 + 3])
7;
+ dst[5] = (-tptr[5] + 9 * tptr[5 + 1] + 9 * tptr[5 + 2] - tptr[5 + 3])
7;
+ dst[6] = (-tptr[6] + 9 * tptr[6 + 1] + 9 * tptr[6 + 2] - tptr[6 + 3])
7;
+ dst[7] = (-tptr[7] + 9 * tptr[7 + 1] + 9 * tptr[7 + 2] - tptr[7 + 3])
7;
+
+ dst += 8;
+ tptr += 11;
+ }
+}
+
+int main (void)
+{
+ char buf [8 * 8];
+
+ test (buf);
+
+ return 0;
+}
+
=== modified file 'gcc/tree-vect-data-refs.c'
--- gcc/tree-vect-data-refs.c 2012-05-31 08:46:10 +
+++ gcc/tree-vect-data-refs.c 2012-06-11 17:31:41 +
@@ -845,6 +845,24 @@
}
}
+ /* Similarly, if we're doing basic-block vectorization, we can only use
+ base and misalignment information relative to an innermost loop if the
+ misalignment stays the same throughout the execution of the loop.
+ As above, this is the case if the stride of the dataref evenly divides
+ by the vector size. */
+ if (!loop)
+ {
+ tree step = DR_STEP (dr);
+ HOST_WIDE_INT dr_step = TREE_INT_CST_LOW (step);
+
+ if (dr_step % GET_MODE_SIZE (TYPE_MODE (vectype)) != 0)
+ {
+ if (vect_print_dump_info (REPORT_ALIGNMENT))
+ fprintf (vect_dump, SLP: step doesn't divide the vector-size.);
+ misalign = NULL_TREE;
+ }
+ }
+
base = build_fold_indirect_ref
Re: [PATCH] Fix PR tree-optimization/53636 (SLP generates invalid misaligned access)
Richard Guenther wrote: On Fri, Jun 15, 2012 at 3:13 PM, Ulrich Weigand uweig...@de.ibm.com wrote: However, there is a second case where we need to check every pass: if we're not actually vectorizing any loop, but are performing basic-block SLP. In this case, it would appear that we need the same check as described in the comment above, i.e. to verify that the stride is a multiple of the vector size. The patch below adds this check, and this indeed fixes the invalid access I was seeing in the test case (in the final assembler, we now get a vld1.16 instead of vldr). Tested on arm-linux-gnueabi with no regressions. OK for mainline? Ok. Thanks for the quick review; I've checked this in to mainline now. I just noticed that the test case also crashes on 4.7, but not on 4.6. Would a backport to 4.7 also be OK, once testing passes? Thanks, Ulrich -- Dr. Ulrich Weigand GNU Toolchain for Linux on System z and Cell BE ulrich.weig...@de.ibm.com
