Patchwork Fix PR tree-optimization/49318

login
register
mail settings
Submitter Ira Rosen
Date June 10, 2011, 7:19 a.m.
Message ID <BANLkTi=zPnDtoXNgD8S3RqtPqAYgZYTYmw@mail.gmail.com>
Download mbox | patch
Permalink /patch/99852/
State New
Headers show

Comments

Ira Rosen - June 10, 2011, 7:19 a.m.
Hi,

The test in PR 49318 fails because the vectorizer recognizes address
computation sequence as a widening-multiplication pattern, while such
sequence is not relevant to vectorization. The problem is that the
vectorizer doesn't check if a statement is going to be vectorized
before replacing it with a pattern. Moreover, the vectorizer first
detects the patterns and only after that looks for relevant
statements. Changing the order is not a good option, since statements
relevance is defined also by their belonging to a pattern.

This patch solves the problem by removing pattern statements that were
created for statements that are not supposed to be vectorized.

Bootstrapped with vectorization enabled on powerpc64-suse-linux and
tested on powerpc64-suse-linux and x86_64-suse-linux.
Committed.

Ira

ChangeLog:

     PR tree-optimization/49318
     * tree-vect-loop.c (vect_determine_vectorization_factor):
     Remove irrelevant pattern statements.

testsuite/ChangeLog:

     PR tree-optimization/49318
     * gcc.dg/vect/pr49318.c: New test.
Richard Guenther - June 10, 2011, 9:14 a.m.
On Fri, Jun 10, 2011 at 9:19 AM, Ira Rosen <ira.rosen@linaro.org> wrote:
> Hi,
>
> The test in PR 49318 fails because the vectorizer recognizes address
> computation sequence as a widening-multiplication pattern, while such
> sequence is not relevant to vectorization. The problem is that the
> vectorizer doesn't check if a statement is going to be vectorized
> before replacing it with a pattern. Moreover, the vectorizer first
> detects the patterns and only after that looks for relevant
> statements. Changing the order is not a good option, since statements
> relevance is defined also by their belonging to a pattern.
>
> This patch solves the problem by removing pattern statements that were
> created for statements that are not supposed to be vectorized.
>
> Bootstrapped with vectorization enabled on powerpc64-suse-linux and
> tested on powerpc64-suse-linux and x86_64-suse-linux.
> Committed.

Ick, yeah.  I remember running into this ordering issue when doing the
multi-vector size reorgs...

In the end I think we should not generate the pattern stmt during
pattern matching but only mark the relevant statements with a
pattern kind.  Say, for each pattern we have a "main" statement
that has related stmts belonging to the pattern that define uses
of the "main" statement - mark those to refer to that "main" statement.
For that "main" statement simply record an enum value, like,
widening_mult.  Then only at vectorized statement
generation time actually generate the vectorized form of the
pattern statement.

Thus, the non-vectorized pattern statements would never be generated.

So, separate analysis and transform more properly.

Of course I didn't get around to implement the above ... (we'd have
a new vectorizable_pattern worker).

Richard.

> Ira
>
> ChangeLog:
>
>     PR tree-optimization/49318
>     * tree-vect-loop.c (vect_determine_vectorization_factor):
>     Remove irrelevant pattern statements.
>
> testsuite/ChangeLog:
>
>     PR tree-optimization/49318
>     * gcc.dg/vect/pr49318.c: New test.
>
Ira Rosen - June 10, 2011, 11:16 a.m.
On 10 June 2011 12:14, Richard Guenther <richard.guenther@gmail.com> wrote:
> On Fri, Jun 10, 2011 at 9:19 AM, Ira Rosen <ira.rosen@linaro.org> wrote:
>> Hi,
>>
>> The test in PR 49318 fails because the vectorizer recognizes address
>> computation sequence as a widening-multiplication pattern, while such
>> sequence is not relevant to vectorization. The problem is that the
>> vectorizer doesn't check if a statement is going to be vectorized
>> before replacing it with a pattern. Moreover, the vectorizer first
>> detects the patterns and only after that looks for relevant
>> statements. Changing the order is not a good option, since statements
>> relevance is defined also by their belonging to a pattern.
>>
>> This patch solves the problem by removing pattern statements that were
>> created for statements that are not supposed to be vectorized.
>>
>> Bootstrapped with vectorization enabled on powerpc64-suse-linux and
>> tested on powerpc64-suse-linux and x86_64-suse-linux.
>> Committed.
>
> Ick, yeah.  I remember running into this ordering issue when doing the
> multi-vector size reorgs...
>
> In the end I think we should not generate the pattern stmt during
> pattern matching but only mark the relevant statements with a
> pattern kind.  Say, for each pattern we have a "main" statement
> that has related stmts belonging to the pattern that define uses
> of the "main" statement - mark those to refer to that "main" statement.
> For that "main" statement simply record an enum value, like,
> widening_mult.  Then only at vectorized statement
> generation time actually generate the vectorized form of the
> pattern statement.
>
> Thus, the non-vectorized pattern statements would never be generated.
>
> So, separate analysis and transform more properly.
>
> Of course I didn't get around to implement the above ... (we'd have
> a new vectorizable_pattern worker).

Sounds like a good idea. I'll give it a try.

Thanks,
Ira

>
> Richard.
>
>> Ira
>>
>> ChangeLog:
>>
>>     PR tree-optimization/49318
>>     * tree-vect-loop.c (vect_determine_vectorization_factor):
>>     Remove irrelevant pattern statements.
>>
>> testsuite/ChangeLog:
>>
>>     PR tree-optimization/49318
>>     * gcc.dg/vect/pr49318.c: New test.
>>
>

Patch

Index: ChangeLog
===================================================================
--- ChangeLog	(revision 174889)
+++ ChangeLog	(working copy)
@@ -1,3 +1,9 @@ 
+2011-06-10  Ira Rosen  <ira.rosen@linaro.org>
+
+	PR tree-optimization/49318
+	* tree-vect-loop.c (vect_determine_vectorization_factor): Remove
+	irrelevant pattern statements.
+
 2011-06-10  Hans-Peter Nilsson  <hp@axis.com>
 
 	* system.h (SETJMP_VIA_SAVE_AREA): Poison.
Index: testsuite/gcc.dg/vect/pr49318.c
===================================================================
--- testsuite/gcc.dg/vect/pr49318.c	(revision 0)
+++ testsuite/gcc.dg/vect/pr49318.c	(revision 0)
@@ -0,0 +1,22 @@ 
+/* { dg-do compile } */
+/* { dg-require-effective-target vect_int } */
+/* { dg-require-effective-target vect_float } */
+
+typedef enum { GL_FALSE } GLenum;
+typedef unsigned char GLboolean;
+typedef int GLint;
+typedef unsigned int GLuint;
+typedef float GLfloat;
+typedef double GLdouble;
+typedef struct gl_context GLcontext;
+struct gl_context {
+  GLfloat TextureMatrix[16];
+  GLenum Primitive;
+};
+void gl_GetDoublev( GLcontext *ctx, GLenum pname, GLdouble *params ) {
+  GLuint i;
+  for (i=0; i<16; i++)
+    params[i] = (GLint) ctx->TextureMatrix[i];
+}
+
+/* { dg-final { cleanup-tree-dump "vect" } } */
Index: testsuite/ChangeLog
===================================================================
--- testsuite/ChangeLog	(revision 174889)
+++ testsuite/ChangeLog	(working copy)
@@ -1,3 +1,8 @@ 
+2011-06-10  Ira Rosen  <ira.rosen@linaro.org>
+
+	PR tree-optimization/49318
+	* gcc.dg/vect/pr49318.c: New test.
+
 2011-06-09  David Krauss  <potswa@mac.com>
 
 	* g++.dg/template/arrow1.C: New.
Index: tree-vect-loop.c
===================================================================
--- tree-vect-loop.c	(revision 174889)
+++ tree-vect-loop.c	(working copy)
@@ -255,10 +255,20 @@  vect_determine_vectorization_factor (loop_vec_info
 
 	  gcc_assert (stmt_info);
 
-	  /* skip stmts which do not need to be vectorized.  */
+	  /* Skip stmts which do not need to be vectorized.  */
 	  if (!STMT_VINFO_RELEVANT_P (stmt_info)
 	      && !STMT_VINFO_LIVE_P (stmt_info))
 	    {
+              if (is_pattern_stmt_p (stmt_info))
+                {
+                   /* We are not going to vectorize this pattern statement,
+                      therefore, remove it.  */
+                  gimple_stmt_iterator tmp_gsi = gsi_for_stmt (stmt);
+                  STMT_VINFO_RELATED_STMT (stmt_info) = NULL;
+                  gsi_remove (&tmp_gsi, true);
+                  free_stmt_vec_info (stmt);
+                }
+
 	      if (vect_print_dump_info (REPORT_DETAILS))
 	        fprintf (vect_dump, "skip.");
 	      continue;