* [PATCH PR63530] Fix the pointer alignment in vectorization
@ 2014-10-17 18:04 Carrot Wei
2014-10-20 8:23 ` Richard Biener
0 siblings, 1 reply; 5+ messages in thread
From: Carrot Wei @ 2014-10-17 18:04 UTC (permalink / raw)
To: gcc-patches; +Cc: Cong Hou, Luis Lozano, Caroline Tice
[-- Attachment #1: Type: text/plain, Size: 591 bytes --]
Hi
In current vectorization pass, when a new vector pointer is created,
its alignment is not set correctly. We should use DR_MISALIGNMENT (dr)
since only this alignment is adjusted when loop peeling or multi
version is occurred.
This patch passed following tests:
x86_64 bootstrap.
x86_64 regression test.
armv7 regression test.
OK for trunk and 4.9 branch?
thanks
Guozhi Wei
2014-10-17 Guozhi Wei <carrot@google.com>
PR tree-optimization/63530
tree-vect-data-refs.c (vect_create_addr_base_for_vector_ref): Set
pointer alignment according to DR_MISALIGNMENT.
[-- Attachment #2: patch --]
[-- Type: application/octet-stream, Size: 706 bytes --]
Index: tree-vect-data-refs.c
===================================================================
--- tree-vect-data-refs.c (revision 216341)
+++ tree-vect-data-refs.c (working copy)
@@ -3957,8 +3957,12 @@
&& TREE_CODE (addr_base) == SSA_NAME)
{
duplicate_ssa_name_ptr_info (addr_base, DR_PTR_INFO (dr));
- if (offset)
+ unsigned int align = TYPE_ALIGN_UNIT (STMT_VINFO_VECTYPE (stmt_info));
+ int misalign = DR_MISALIGNMENT (dr);
+ if (offset || (misalign == -1))
mark_ptr_info_alignment_unknown (SSA_NAME_PTR_INFO (addr_base));
+ else if (misalign)
+ set_ptr_info_alignment (SSA_NAME_PTR_INFO (addr_base), align, misalign);
}
if (dump_enabled_p ())
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH PR63530] Fix the pointer alignment in vectorization
2014-10-17 18:04 [PATCH PR63530] Fix the pointer alignment in vectorization Carrot Wei
@ 2014-10-20 8:23 ` Richard Biener
2014-10-20 20:11 ` Carrot Wei
0 siblings, 1 reply; 5+ messages in thread
From: Richard Biener @ 2014-10-20 8:23 UTC (permalink / raw)
To: Carrot Wei; +Cc: gcc-patches, Cong Hou, Luis Lozano, Caroline Tice
On Fri, Oct 17, 2014 at 7:58 PM, Carrot Wei <carrot@google.com> wrote:
> Hi
>
> In current vectorization pass, when a new vector pointer is created,
> its alignment is not set correctly. We should use DR_MISALIGNMENT (dr)
> since only this alignment is adjusted when loop peeling or multi
> version is occurred.
>
> This patch passed following tests:
> x86_64 bootstrap.
> x86_64 regression test.
> armv7 regression test.
>
> OK for trunk and 4.9 branch?
I miss a testcase. I also miss a comment before this code explaining
why DR_MISALIGNMENT if not -1 is valid and why it is not valid if
'offset' is supplied (what about 'byte_offset' btw?). Also if peeling
for alignment aligned this ref (misalign == 0) you don't set the alignment.
Thus you may fix a bug (not sure without a testcase) but the new code
certainly doesn't look 100% correct.
That said, I would have expected that we can unconditionally do
set_ptr_info_alignment (..., align, misalign)
if misalign is != -1 and if we adjust misalign by offset * step + byte_offset
(usually both are constants).
Also we can still trust the alignment copied from addr_base modulo
vector element size even if DR_MISALIGN is -1. This may matter
for targets that require element-alignment for vector accesses.
Thanks,
Richard.
> thanks
> Guozhi Wei
>
> 2014-10-17 Guozhi Wei <carrot@google.com>
>
> PR tree-optimization/63530
> tree-vect-data-refs.c (vect_create_addr_base_for_vector_ref): Set
> pointer alignment according to DR_MISALIGNMENT.
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH PR63530] Fix the pointer alignment in vectorization
2014-10-20 8:23 ` Richard Biener
@ 2014-10-20 20:11 ` Carrot Wei
2014-10-21 8:12 ` Richard Biener
0 siblings, 1 reply; 5+ messages in thread
From: Carrot Wei @ 2014-10-20 20:11 UTC (permalink / raw)
To: Richard Biener; +Cc: gcc-patches, Cong Hou, Luis Lozano, Caroline Tice
Hi Richard
An arm testcase that can reproduce this bug is attached.
2014-10-20 Guozhi Wei <carrot@google.com>
PR tree-optimization/63530
gcc.target/arm/pr63530.c: New testcase.
Index: pr63530.c
===================================================================
--- pr63530.c (revision 0)
+++ pr63530.c (revision 0)
@@ -0,0 +1,21 @@
+/* { dg-do compile } */
+/* { dg-require-effective-target arm_neon } */
+/* { dg-options "-march=armv7-a -mfloat-abi=hard -mfpu=neon -marm -O2
-ftree-vectorize -funroll-loops --param
\"max-completely-peeled-insns=400\"" } */
+
+typedef struct {
+ unsigned char map[256];
+ int i;
+} A, *AP;
+
+void* calloc(int, int);
+
+AP foo (int n)
+{
+ AP b = (AP)calloc (1, sizeof (A));
+ int i;
+ for (i = n; i < 256; i++)
+ b->map[i] = i;
+ return b;
+}
+
+/* { dg-final { scan-assembler-not "vst1.64" } } */
On Mon, Oct 20, 2014 at 1:19 AM, Richard Biener
<richard.guenther@gmail.com> wrote:
> On Fri, Oct 17, 2014 at 7:58 PM, Carrot Wei <carrot@google.com> wrote:
>
> I miss a testcase. I also miss a comment before this code explaining
> why DR_MISALIGNMENT if not -1 is valid and why it is not valid if
DR_MISALIGNMENT (dr) == -1 means some unknown misalignment, otherwise
it means some known misalignment.
See the usage in file tree-vect-stmts.c.
> 'offset' is supplied (what about 'byte_offset' btw?). Also if peeling
It is for conservative, so it doesn't change the logic when offset is supplied.
I've checked that most of the passed in offset are caused by negative
step, its impact to DR_MISALIGNMENT should have already be considered
in function vect_update_misalignment_for_peel, but the comments of
vect_create_addr_base_for_vector_ref does not guarantee this usage of
offset.
The usage of byte_offset is quite broken, many direct or indirect
callers don't provide the parameters. So only the author can comment
this.
> for alignment aligned this ref (misalign == 0) you don't set the alignment.
>
I assume if no misalignment is specified, the natural alignment of the
vector type is used, and caused the wrong code in our case, is it
right?
> Thus you may fix a bug (not sure without a testcase) but the new code
> certainly doesn't look 100% correct.
>
> That said, I would have expected that we can unconditionally do
>
> set_ptr_info_alignment (..., align, misalign)
>
> if misalign is != -1 and if we adjust misalign by offset * step + byte_offset
> (usually both are constants).
>
> Also we can still trust the alignment copied from addr_base modulo
> vector element size even if DR_MISALIGN is -1. This may matter
> for targets that require element-alignment for vector accesses.
>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH PR63530] Fix the pointer alignment in vectorization
2014-10-20 20:11 ` Carrot Wei
@ 2014-10-21 8:12 ` Richard Biener
2014-10-22 16:09 ` Carrot Wei
0 siblings, 1 reply; 5+ messages in thread
From: Richard Biener @ 2014-10-21 8:12 UTC (permalink / raw)
To: Carrot Wei; +Cc: gcc-patches, Cong Hou, Luis Lozano, Caroline Tice
On Mon, Oct 20, 2014 at 10:10 PM, Carrot Wei <carrot@google.com> wrote:
> Hi Richard
>
> An arm testcase that can reproduce this bug is attached.
>
> 2014-10-20 Guozhi Wei <carrot@google.com>
>
> PR tree-optimization/63530
> gcc.target/arm/pr63530.c: New testcase.
>
>
> Index: pr63530.c
> ===================================================================
> --- pr63530.c (revision 0)
> +++ pr63530.c (revision 0)
> @@ -0,0 +1,21 @@
> +/* { dg-do compile } */
> +/* { dg-require-effective-target arm_neon } */
> +/* { dg-options "-march=armv7-a -mfloat-abi=hard -mfpu=neon -marm -O2
> -ftree-vectorize -funroll-loops --param
> \"max-completely-peeled-insns=400\"" } */
> +
> +typedef struct {
> + unsigned char map[256];
> + int i;
> +} A, *AP;
> +
> +void* calloc(int, int);
> +
> +AP foo (int n)
> +{
> + AP b = (AP)calloc (1, sizeof (A));
> + int i;
> + for (i = n; i < 256; i++)
> + b->map[i] = i;
> + return b;
> +}
> +
> +/* { dg-final { scan-assembler-not "vst1.64" } } */
Can you make it a runtime testcase that fails? This way it would be
less target specific.
> On Mon, Oct 20, 2014 at 1:19 AM, Richard Biener
> <richard.guenther@gmail.com> wrote:
>> On Fri, Oct 17, 2014 at 7:58 PM, Carrot Wei <carrot@google.com> wrote:
>>
>> I miss a testcase. I also miss a comment before this code explaining
>> why DR_MISALIGNMENT if not -1 is valid and why it is not valid if
>
> DR_MISALIGNMENT (dr) == -1 means some unknown misalignment, otherwise
> it means some known misalignment.
> See the usage in file tree-vect-stmts.c.
I know that.
>> 'offset' is supplied (what about 'byte_offset' btw?). Also if peeling
>
> It is for conservative, so it doesn't change the logic when offset is supplied.
> I've checked that most of the passed in offset are caused by negative
> step, its impact to DR_MISALIGNMENT should have already be considered
> in function vect_update_misalignment_for_peel, but the comments of
> vect_create_addr_base_for_vector_ref does not guarantee this usage of
> offset.
>
> The usage of byte_offset is quite broken, many direct or indirect
> callers don't provide the parameters. So only the author can comment
> this.
Well - please make it consistent at least, (offset || byte_offset).
>> for alignment aligned this ref (misalign == 0) you don't set the alignment.
>>
> I assume if no misalignment is specified, the natural alignment of the
> vector type is used, and caused the wrong code in our case, is it
> right?
No, DR_MISALIGNMENT == 0 means "aligned".
OTOH it's quite unnecessary to do all the dance with the alignment
part of the SSA name info (unnecessary for the actual memory references
created by the vectorizer). The type of the access ultimatively provides
the larger alignment - the SSA name info only may enlarge it even
further (thus it's dangerous to specify larger than valid there).
So if you don't want to make it really optimal wrt offset/byte_offset please
do
if (offset || byte_offset || misalign == -1)
mark_ptr_info_alignment_unknown (...)
else
set_ptr_info_alignment (..., align, misalign);
The patch is ok with this change and the testcase turned into a runtime one
and moved to gcc.dg/vect/
Thanks,
Richard.
>> Thus you may fix a bug (not sure without a testcase) but the new code
>> certainly doesn't look 100% correct.
>>
>> That said, I would have expected that we can unconditionally do
>>
>> set_ptr_info_alignment (..., align, misalign)
>>
>> if misalign is != -1 and if we adjust misalign by offset * step + byte_offset
>> (usually both are constants).
>>
>> Also we can still trust the alignment copied from addr_base modulo
>> vector element size even if DR_MISALIGN is -1. This may matter
>> for targets that require element-alignment for vector accesses.
>>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH PR63530] Fix the pointer alignment in vectorization
2014-10-21 8:12 ` Richard Biener
@ 2014-10-22 16:09 ` Carrot Wei
0 siblings, 0 replies; 5+ messages in thread
From: Carrot Wei @ 2014-10-22 16:09 UTC (permalink / raw)
To: Richard Biener; +Cc: gcc-patches, Cong Hou, Luis Lozano, Caroline Tice
[-- Attachment #1: Type: text/plain, Size: 4454 bytes --]
Thanks for the review.
Following patch has been committed. I will port them to 4.9 branch
several days later.
2014-10-22 Guozhi Wei <carrot@google.com>
PR tree-optimization/63530
tree-vect-data-refs.c (vect_create_addr_base_for_vector_ref): Set
pointer alignment according to DR_MISALIGNMENT.
2014-10-22 Guozhi Wei <carrot@google.com>
PR tree-optimization/63530
gcc.dg/vect/pr63530.c: New testcase.
On Tue, Oct 21, 2014 at 1:04 AM, Richard Biener
<richard.guenther@gmail.com> wrote:
> On Mon, Oct 20, 2014 at 10:10 PM, Carrot Wei <carrot@google.com> wrote:
>> Hi Richard
>>
>> An arm testcase that can reproduce this bug is attached.
>>
>> 2014-10-20 Guozhi Wei <carrot@google.com>
>>
>> PR tree-optimization/63530
>> gcc.target/arm/pr63530.c: New testcase.
>>
>>
>> Index: pr63530.c
>> ===================================================================
>> --- pr63530.c (revision 0)
>> +++ pr63530.c (revision 0)
>> @@ -0,0 +1,21 @@
>> +/* { dg-do compile } */
>> +/* { dg-require-effective-target arm_neon } */
>> +/* { dg-options "-march=armv7-a -mfloat-abi=hard -mfpu=neon -marm -O2
>> -ftree-vectorize -funroll-loops --param
>> \"max-completely-peeled-insns=400\"" } */
>> +
>> +typedef struct {
>> + unsigned char map[256];
>> + int i;
>> +} A, *AP;
>> +
>> +void* calloc(int, int);
>> +
>> +AP foo (int n)
>> +{
>> + AP b = (AP)calloc (1, sizeof (A));
>> + int i;
>> + for (i = n; i < 256; i++)
>> + b->map[i] = i;
>> + return b;
>> +}
>> +
>> +/* { dg-final { scan-assembler-not "vst1.64" } } */
>
> Can you make it a runtime testcase that fails? This way it would be
> less target specific.
>
>> On Mon, Oct 20, 2014 at 1:19 AM, Richard Biener
>> <richard.guenther@gmail.com> wrote:
>>> On Fri, Oct 17, 2014 at 7:58 PM, Carrot Wei <carrot@google.com> wrote:
>>>
>>> I miss a testcase. I also miss a comment before this code explaining
>>> why DR_MISALIGNMENT if not -1 is valid and why it is not valid if
>>
>> DR_MISALIGNMENT (dr) == -1 means some unknown misalignment, otherwise
>> it means some known misalignment.
>> See the usage in file tree-vect-stmts.c.
>
> I know that.
>
>>> 'offset' is supplied (what about 'byte_offset' btw?). Also if peeling
>>
>> It is for conservative, so it doesn't change the logic when offset is supplied.
>> I've checked that most of the passed in offset are caused by negative
>> step, its impact to DR_MISALIGNMENT should have already be considered
>> in function vect_update_misalignment_for_peel, but the comments of
>> vect_create_addr_base_for_vector_ref does not guarantee this usage of
>> offset.
>>
>> The usage of byte_offset is quite broken, many direct or indirect
>> callers don't provide the parameters. So only the author can comment
>> this.
>
> Well - please make it consistent at least, (offset || byte_offset).
>
>>> for alignment aligned this ref (misalign == 0) you don't set the alignment.
>>>
>> I assume if no misalignment is specified, the natural alignment of the
>> vector type is used, and caused the wrong code in our case, is it
>> right?
>
> No, DR_MISALIGNMENT == 0 means "aligned".
>
> OTOH it's quite unnecessary to do all the dance with the alignment
> part of the SSA name info (unnecessary for the actual memory references
> created by the vectorizer). The type of the access ultimatively provides
> the larger alignment - the SSA name info only may enlarge it even
> further (thus it's dangerous to specify larger than valid there).
>
> So if you don't want to make it really optimal wrt offset/byte_offset please
> do
>
> if (offset || byte_offset || misalign == -1)
> mark_ptr_info_alignment_unknown (...)
> else
> set_ptr_info_alignment (..., align, misalign);
>
> The patch is ok with this change and the testcase turned into a runtime one
> and moved to gcc.dg/vect/
>
> Thanks,
> Richard.
>
>>> Thus you may fix a bug (not sure without a testcase) but the new code
>>> certainly doesn't look 100% correct.
>>>
>>> That said, I would have expected that we can unconditionally do
>>>
>>> set_ptr_info_alignment (..., align, misalign)
>>>
>>> if misalign is != -1 and if we adjust misalign by offset * step + byte_offset
>>> (usually both are constants).
>>>
>>> Also we can still trust the alignment copied from addr_base modulo
>>> vector element size even if DR_MISALIGN is -1. This may matter
>>> for targets that require element-alignment for vector accesses.
>>>
[-- Attachment #2: patch --]
[-- Type: application/octet-stream, Size: 707 bytes --]
Index: tree-vect-data-refs.c
===================================================================
--- tree-vect-data-refs.c (revision 216561)
+++ tree-vect-data-refs.c (working copy)
@@ -3960,8 +3960,12 @@
&& TREE_CODE (addr_base) == SSA_NAME)
{
duplicate_ssa_name_ptr_info (addr_base, DR_PTR_INFO (dr));
- if (offset)
+ unsigned int align = TYPE_ALIGN_UNIT (STMT_VINFO_VECTYPE (stmt_info));
+ int misalign = DR_MISALIGNMENT (dr);
+ if (offset || byte_offset || (misalign == -1))
mark_ptr_info_alignment_unknown (SSA_NAME_PTR_INFO (addr_base));
+ else
+ set_ptr_info_alignment (SSA_NAME_PTR_INFO (addr_base), align, misalign);
}
if (dump_enabled_p ())
[-- Attachment #3: patch2 --]
[-- Type: application/octet-stream, Size: 917 bytes --]
Index: testsuite/gcc.dg/vect/pr63530.c
===================================================================
--- testsuite/gcc.dg/vect/pr63530.c (revision 0)
+++ testsuite/gcc.dg/vect/pr63530.c (revision 0)
@@ -0,0 +1,30 @@
+/* { dg-options "-O2 -ftree-vectorize -funroll-loops --param \"max-completely-peeled-insns=400\"" } */
+
+/* PR tree-optimization/63530 */
+/* On armv7 hardware, following options cause run time failure */
+/* -march=armv7-a -mfloat-abi=hard -mfpu=neon -marm -O2 -ftree-vectorize */
+/* -funroll-loops --param "max-completely-peeled-insns=400" */
+
+#include <stdlib.h>
+
+typedef struct {
+ unsigned char map[256];
+ int i;
+} A, *AP;
+
+AP __attribute__ ((noinline))
+foo (int n)
+{
+ AP b = (AP)calloc (1, sizeof (A));
+ int i;
+ for (i = n; i < 256; i++)
+ b->map[i] = i;
+ return b;
+}
+
+int
+main()
+{
+ AP p = foo(3);
+ return p->map[30] - p->map[20] - p->map[10];
+}
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2014-10-22 16:02 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2014-10-17 18:04 [PATCH PR63530] Fix the pointer alignment in vectorization Carrot Wei
2014-10-20 8:23 ` Richard Biener
2014-10-20 20:11 ` Carrot Wei
2014-10-21 8:12 ` Richard Biener
2014-10-22 16:09 ` Carrot Wei
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).