Re: [PATCH] Fixing PR60000: A bug in the vectorizer.
Wrong format. Send it again.
On Thu, Jan 30, 2014 at 4:57 PM, Cong Hou wrote:
>
> Hi
>
> PR6 (http://gcc.gnu.org/bugzilla/show_bug.cgi?id=6) is caused by GCC
> vectorizer. The bug appears when handling vectorization patterns. When a
> pattern statement has additional new statements stored in pattern_def_seq in
> vect_transform_loop(), those statements are vectorized before the pattern
> statement. Once all those statements are handled, pattern_def_seq is set to
> NULL. However, if the pattern statement is a store, pattern_def_seq will not
> be set to NULL. In consequence, the next pattern statement will not have the
> correct pattern_def_seq.
>
> This bug can be fixed by nullifying pattern_def_seq before checking if the
> vectorized statement is a store. The patch is pasted below. Bootstrapped and
> tested on x86_64.
>
>
> thanks,
> Cong
>
>
>
> diff --git a/gcc/ChangeLog b/gcc/ChangeLog
> index 95a324c..9df0d34 100644
> --- a/gcc/ChangeLog
> +++ b/gcc/ChangeLog
> @@ -1,3 +1,10 @@
> +2014-01-30 Cong Hou
> +
> + PR tree-optimization/6
> + * tree-vect-loop.c (vect_transform_loop): Set pattern_def_seq to NULL
> + before checking if the vectorized statement is a store. A store
> + statement can be a pattern one.
> +
> 2014-01-27 Jakub Jelinek
>
> PR bootstrap/59934
> diff --git a/gcc/testsuite/ChangeLog b/gcc/testsuite/ChangeLog
> index fa61d5c..f2ce70f 100644
> --- a/gcc/testsuite/ChangeLog
> +++ b/gcc/testsuite/ChangeLog
> @@ -1,3 +1,8 @@
> +2014-01-30 Cong Hou
> +
> + PR tree-optimization/6
> + * g++.dg/vect/pr6.cc: New test.
> +
> 2014-01-27 Christian Bruel
>
> * gcc.target/sh/torture/strncmp.c: New tests.
> diff --git a/gcc/testsuite/g++.dg/vect/pr6.cc
> b/gcc/testsuite/g++.dg/vect/pr6.cc
> new file mode 100644
> index 000..8a8bd22
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/vect/pr6.cc
> @@ -0,0 +1,13 @@
> +/* { dg-do compile } */
> +/* { dg-additional-options "-fno-tree-vrp" } */
> +
> +void foo (bool* a, int* b)
> +{
> + for (int i = 0; i < 1000; ++i)
> +{
> + a[i] = i % 2;
> + b[i] = i % 3;
> +}
> +}
> +
> +/* { dg-final { cleanup-tree-dump "vect" } } */
> diff --git a/gcc/tree-vect-loop.c b/gcc/tree-vect-loop.c
> index 69c8d21..8c8bece 100644
> --- a/gcc/tree-vect-loop.c
> +++ b/gcc/tree-vect-loop.c
> @@ -6044,6 +6044,10 @@ vect_transform_loop (loop_vec_info loop_vinfo)
>
> grouped_store = false;
> is_store = vect_transform_stmt (stmt, &si, &grouped_store, NULL,
> NULL);
> +
> + if (!transform_pattern_stmt && gsi_end_p (pattern_def_si))
> + pattern_def_seq = NULL;
> +
>if (is_store)
> {
> if (STMT_VINFO_GROUPED_ACCESS (stmt_info))
> @@ -6068,10 +6072,7 @@ vect_transform_loop (loop_vec_info loop_vinfo)
> }
>
> if (!transform_pattern_stmt && gsi_end_p (pattern_def_si))
> - {
> - pattern_def_seq = NULL;
> - gsi_next (&si);
> - }
> + gsi_next (&si);
> } /* stmts in BB */
> } /* BBs in loop */
>
>
>
Re: [PATCH] Fixing PR60000: A bug in the vectorizer.
On Thu, 30 Jan 2014, Cong Hou wrote:
> Hi
>
> PR6 (http://gcc.gnu.org/bugzilla/show_bug.cgi?id=6) is caused by
> GCC vectorizer. The bug appears when handling vectorization patterns. When
> a pattern statement has additional new statements stored in pattern_def_seq
> in vect_transform_loop(), those statements are vectorized before the
> pattern statement. Once all those statements are handled, pattern_def_seq
> is set to NULL. However, if the pattern statement is a store,
> pattern_def_seq will not be set to NULL. In consequence, the next pattern
> statement will not have the correct pattern_def_seq
Is that because si and pattern_def_si point to the same stmts? Then
I'd prefer to do
if (is_store)
{
...
pattern_def_seq = NULL;
}
else if (!transform_pattern_stmt && gsi_end_p (pattern_def_si))
{
pattern_def_seq = NULL;
gsi_next (&si);
}
Richard.
>
> This bug can be fixed by nullifying pattern_def_seq before checking if the
> vectorized statement is a store. The patch is pasted below. Bootstrapped
> and tested on x86_64.
>
>
> thanks,
> Cong
>
>
>
> diff --git a/gcc/ChangeLog b/gcc/ChangeLog
> index 95a324c..9df0d34 100644
> --- a/gcc/ChangeLog
> +++ b/gcc/ChangeLog
> @@ -1,3 +1,10 @@
> +2014-01-30 Cong Hou
> +
> + PR tree-optimization/6
> + * tree-vect-loop.c (vect_transform_loop): Set pattern_def_seq to
> NULL
> + before checking if the vectorized statement is a store. A store
> + statement can be a pattern one.
> +
> 2014-01-27 Jakub Jelinek
>
> PR bootstrap/59934
> diff --git a/gcc/testsuite/ChangeLog b/gcc/testsuite/ChangeLog
> index fa61d5c..f2ce70f 100644
> --- a/gcc/testsuite/ChangeLog
> +++ b/gcc/testsuite/ChangeLog
> @@ -1,3 +1,8 @@
> +2014-01-30 Cong Hou
> +
> + PR tree-optimization/6
> + * g++.dg/vect/pr6.cc: New test.
> +
> 2014-01-27 Christian Bruel
>
> * gcc.target/sh/torture/strncmp.c: New tests.
> diff --git a/gcc/testsuite/g++.dg/vect/pr6.cc
> b/gcc/testsuite/g++.dg/vect/pr6.cc
> new file mode 100644
> index 000..8a8bd22
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/vect/pr6.cc
> @@ -0,0 +1,13 @@
> +/* { dg-do compile } */
> +/* { dg-additional-options "-fno-tree-vrp" } */
> +
> +void foo (bool* a, int* b)
> +{
> + for (int i = 0; i < 1000; ++i)
> +{
> + a[i] = i % 2;
> + b[i] = i % 3;
> +}
> +}
> +
> +/* { dg-final { cleanup-tree-dump "vect" } } */
> diff --git a/gcc/tree-vect-loop.c b/gcc/tree-vect-loop.c
> index 69c8d21..8c8bece 100644
> --- a/gcc/tree-vect-loop.c
> +++ b/gcc/tree-vect-loop.c
> @@ -6044,6 +6044,10 @@ vect_transform_loop (loop_vec_info loop_vinfo)
>
> grouped_store = false;
> is_store = vect_transform_stmt (stmt, &si, &grouped_store, NULL,
> NULL);
> +
> + if (!transform_pattern_stmt && gsi_end_p (pattern_def_si))
> + pattern_def_seq = NULL;
> +
>if (is_store)
> {
> if (STMT_VINFO_GROUPED_ACCESS (stmt_info))
> @@ -6068,10 +6072,7 @@ vect_transform_loop (loop_vec_info loop_vinfo)
> }
>
> if (!transform_pattern_stmt && gsi_end_p (pattern_def_si))
> - {
> - pattern_def_seq = NULL;
> - gsi_next (&si);
> - }
> + gsi_next (&si);
> } /* stmts in BB */
> } /* BBs in loop */
>
--
Richard Biener
SUSE / SUSE Labs
SUSE LINUX Products GmbH - Nuernberg - AG Nuernberg - HRB 16746
GF: Jeff Hawn, Jennifer Guild, Felix Imend"orffer
Re: [PATCH] Fixing PR60000: A bug in the vectorizer.
On Fri, Jan 31, 2014 at 09:41:59AM +0100, Richard Biener wrote:
> Is that because si and pattern_def_si point to the same stmts? Then
> I'd prefer to do
>
> if (is_store)
>{
> ...
> pattern_def_seq = NULL;
>}
> else if (!transform_pattern_stmt && gsi_end_p (pattern_def_si))
>{
> pattern_def_seq = NULL;
> gsi_next (&si);
>}
Yeah, I think stores can only appear at the end of patterns, so IMHO it should
be
safe to just clear pattern_def_seq always in that case. Right now the code
has continue; separately for STMT_VINFO_GROUPED_ACCESS (stmt_info) and
for !STMT_VINFO_GROUPED_ACCESS (stmt_info) stores, but I guess you should
just move them at the end of if (is_store) and clear pattern_def_seq there
before the continue. Add gcc_assert (!transform_pattern_stmt); too?
Jakub
Re: [PATCH] Fixing PR60000: A bug in the vectorizer.
On Fri, Jan 31, 2014 at 5:06 AM, Jakub Jelinek wrote:
>
> On Fri, Jan 31, 2014 at 09:41:59AM +0100, Richard Biener wrote:
> > Is that because si and pattern_def_si point to the same stmts? Then
> > I'd prefer to do
> >
> > if (is_store)
> >{
> > ...
> > pattern_def_seq = NULL;
> >}
> > else if (!transform_pattern_stmt && gsi_end_p (pattern_def_si))
> >{
> > pattern_def_seq = NULL;
> > gsi_next (&si);
> >}
>
> Yeah, I think stores can only appear at the end of patterns, so IMHO it
> should be
> safe to just clear pattern_def_seq always in that case. Right now the code
> has continue; separately for STMT_VINFO_GROUPED_ACCESS (stmt_info) and
> for !STMT_VINFO_GROUPED_ACCESS (stmt_info) stores, but I guess you should
> just move them at the end of if (is_store) and clear pattern_def_seq there
> before the continue. Add gcc_assert (!transform_pattern_stmt); too?
I agree. I have updated the patch accordingly. Bootstrapped and tested
on x86_64. OK for the trunk?
thanks,
Cong
diff --git a/gcc/ChangeLog b/gcc/ChangeLog
index 95a324c..cabcaf8 100644
--- a/gcc/ChangeLog
+++ b/gcc/ChangeLog
@@ -1,3 +1,10 @@
+2014-01-30 Cong Hou
+
+ PR tree-optimization/6
+ * tree-vect-loop.c (vect_transform_loop): Set pattern_def_seq to NULL
+ if the vectorized statement is a store. A store statement can only
+ appear at the end of pattern statements.
+
2014-01-27 Jakub Jelinek
PR bootstrap/59934
diff --git a/gcc/testsuite/ChangeLog b/gcc/testsuite/ChangeLog
index fa61d5c..f2ce70f 100644
--- a/gcc/testsuite/ChangeLog
+++ b/gcc/testsuite/ChangeLog
@@ -1,3 +1,8 @@
+2014-01-30 Cong Hou
+
+ PR tree-optimization/6
+ * g++.dg/vect/pr6.cc: New test.
+
2014-01-27 Christian Bruel
* gcc.target/sh/torture/strncmp.c: New tests.
diff --git a/gcc/testsuite/g++.dg/vect/pr6.cc
b/gcc/testsuite/g++.dg/vect/pr6.cc
new file mode 100644
index 000..fe39d6a
--- /dev/null
+++ b/gcc/testsuite/g++.dg/vect/pr6.cc
@@ -0,0 +1,13 @@
+/* { dg-do compile } */
+/* { dg-additional-options "-fno-tree-vrp" } */
+
+void foo (bool* a, int* b)
+{
+ for (int i = 0; i < 1000; ++i)
+{
+ a[i] = i % 2;
+ b[i] = i % 3;
+}
+}
+
+/* { dg-final { cleanup-tree-dump "vect" } } */
diff --git a/gcc/tree-vect-loop.c b/gcc/tree-vect-loop.c
index 69c8d21..0e162cb 100644
--- a/gcc/tree-vect-loop.c
+++ b/gcc/tree-vect-loop.c
@@ -6053,7 +6053,6 @@ vect_transform_loop (loop_vec_info loop_vinfo)
the chain. */
gsi_next (&si);
vect_remove_stores (GROUP_FIRST_ELEMENT (stmt_info));
- continue;
}
else
{
@@ -6063,11 +6062,13 @@ vect_transform_loop (loop_vec_info loop_vinfo)
unlink_stmt_vdef (store);
gsi_remove (&si, true);
release_defs (store);
- continue;
}
- }
- if (!transform_pattern_stmt && gsi_end_p (pattern_def_si))
+ /* Stores can only appear at the end of pattern statements. */
+ gcc_assert (!transform_pattern_stmt);
+ pattern_def_seq = NULL;
+ }
+ else if (!transform_pattern_stmt && gsi_end_p (pattern_def_si))
{
pattern_def_seq = NULL;
gsi_next (&si);
>
>
> Jakub
Re: [PATCH] Fixing PR60000: A bug in the vectorizer.
On February 1, 2014 12:06:55 AM GMT+01:00, Cong Hou wrote:
>On Fri, Jan 31, 2014 at 5:06 AM, Jakub Jelinek
>wrote:
>>
>> On Fri, Jan 31, 2014 at 09:41:59AM +0100, Richard Biener wrote:
>> > Is that because si and pattern_def_si point to the same stmts?
>Then
>> > I'd prefer to do
>> >
>> > if (is_store)
>> >{
>> > ...
>> > pattern_def_seq = NULL;
>> >}
>> > else if (!transform_pattern_stmt && gsi_end_p (pattern_def_si))
>> >{
>> > pattern_def_seq = NULL;
>> > gsi_next (&si);
>> >}
>>
>> Yeah, I think stores can only appear at the end of patterns, so IMHO
>it should be
>> safe to just clear pattern_def_seq always in that case. Right now
>the code
>> has continue; separately for STMT_VINFO_GROUPED_ACCESS (stmt_info)
>and
>> for !STMT_VINFO_GROUPED_ACCESS (stmt_info) stores, but I guess you
>should
>> just move them at the end of if (is_store) and clear pattern_def_seq
>there
>> before the continue. Add gcc_assert (!transform_pattern_stmt); too?
>
>
>I agree. I have updated the patch accordingly. Bootstrapped and tested
>on x86_64. OK for the trunk?
Ok.
Thanks,
Richard.
>thanks,
>Cong
>
>
>
>
>diff --git a/gcc/ChangeLog b/gcc/ChangeLog
>index 95a324c..cabcaf8 100644
>--- a/gcc/ChangeLog
>+++ b/gcc/ChangeLog
>@@ -1,3 +1,10 @@
>+2014-01-30 Cong Hou
>+
>+ PR tree-optimization/6
>+ * tree-vect-loop.c (vect_transform_loop): Set pattern_def_seq
>to NULL
>+ if the vectorized statement is a store. A store statement can
>only
>+ appear at the end of pattern statements.
>+
> 2014-01-27 Jakub Jelinek
>
>PR bootstrap/59934
>diff --git a/gcc/testsuite/ChangeLog b/gcc/testsuite/ChangeLog
>index fa61d5c..f2ce70f 100644
>--- a/gcc/testsuite/ChangeLog
>+++ b/gcc/testsuite/ChangeLog
>@@ -1,3 +1,8 @@
>+2014-01-30 Cong Hou
>+
>+ PR tree-optimization/6
>+ * g++.dg/vect/pr6.cc: New test.
>+
> 2014-01-27 Christian Bruel
>
>* gcc.target/sh/torture/strncmp.c: New tests.
>diff --git a/gcc/testsuite/g++.dg/vect/pr6.cc
>b/gcc/testsuite/g++.dg/vect/pr6.cc
>new file mode 100644
>index 000..fe39d6a
>--- /dev/null
>+++ b/gcc/testsuite/g++.dg/vect/pr6.cc
>@@ -0,0 +1,13 @@
>+/* { dg-do compile } */
>+/* { dg-additional-options "-fno-tree-vrp" } */
>+
>+void foo (bool* a, int* b)
>+{
>+ for (int i = 0; i < 1000; ++i)
>+{
>+ a[i] = i % 2;
>+ b[i] = i % 3;
>+}
>+}
>+
>+/* { dg-final { cleanup-tree-dump "vect" } } */
>diff --git a/gcc/tree-vect-loop.c b/gcc/tree-vect-loop.c
>index 69c8d21..0e162cb 100644
>--- a/gcc/tree-vect-loop.c
>+++ b/gcc/tree-vect-loop.c
>@@ -6053,7 +6053,6 @@ vect_transform_loop (loop_vec_info loop_vinfo)
> the chain. */
> gsi_next (&si);
> vect_remove_stores (GROUP_FIRST_ELEMENT (stmt_info));
>- continue;
>}
> else
>{
>@@ -6063,11 +6062,13 @@ vect_transform_loop (loop_vec_info loop_vinfo)
> unlink_stmt_vdef (store);
> gsi_remove (&si, true);
> release_defs (store);
>- continue;
>}
>- }
>
>- if (!transform_pattern_stmt && gsi_end_p (pattern_def_si))
>+ /* Stores can only appear at the end of pattern
>statements. */
>+ gcc_assert (!transform_pattern_stmt);
>+ pattern_def_seq = NULL;
>+ }
>+ else if (!transform_pattern_stmt && gsi_end_p
>(pattern_def_si))
>{
> pattern_def_seq = NULL;
> gsi_next (&si);
>
>
>
>
>
>>
>>
>> Jakub
