Patchwork Fix PR50183

login
register
mail settings
Submitter William J. Schmidt
Date Sept. 13, 2011, 10:55 p.m.
Message ID <1315954554.27725.26.camel@gnopaine>
Download mbox | patch
Permalink /patch/114577/
State New
Headers show

Comments

William J. Schmidt - Sept. 13, 2011, 10:55 p.m.
Greetings,

The code to build scops (static control parts) for graphite first
rewrites loops into canonical loop-closed SSA form.  PR50183 identifies
a scenario where the results do not fulfill all required invariants of
this form.  In particular, a value defined inside a loop and used
outside that loop must reach exactly one definition, which must be a
single-argument PHI node called a close-phi.  When nested loops exist,
it is possible that, following the rewrite, a definition may reach two
close-phis.  This patch corrects that problem.

The problem arises because loops are processed from outside in.  While
processing a loop, duplicate close-phis are eliminated.  However,
eliminating duplicate close-phis for an inner loop can sometimes create
duplicate close-phis for an already-processed outer loop.  This patch
detects when this may have occurred and repeats the removal of duplicate
close-phis as necessary.

The problem was noted on ibm/4_6-branch and 4_6-branch; it is apparently
latent on trunk.  The same patch can be applied to all three branches.

Bootstrapped and regression-tested on powerpc64-linux.  OK to commit to
these three branches?

Thanks,
Bill


2011-09-13  Bill Schmidt  <wschmidt@linux.vnet.ibm.com>

	* graphite-scop-detection.c (make_close_phi_nodes_unique):  New
	forward declaration.
	(remove_duplicate_close_phi): Detect and repair creation of
	duplicate close-phis for a containing loop.
William J. Schmidt - Sept. 28, 2011, 10:10 p.m.
Hi there,

Ping.  I'm seeking approval for this fix on trunk and 4_6-branch.
Thanks!

Bill

On Tue, 2011-09-13 at 17:55 -0500, William J. Schmidt wrote:
> Greetings,
> 
> The code to build scops (static control parts) for graphite first
> rewrites loops into canonical loop-closed SSA form.  PR50183 identifies
> a scenario where the results do not fulfill all required invariants of
> this form.  In particular, a value defined inside a loop and used
> outside that loop must reach exactly one definition, which must be a
> single-argument PHI node called a close-phi.  When nested loops exist,
> it is possible that, following the rewrite, a definition may reach two
> close-phis.  This patch corrects that problem.
> 
> The problem arises because loops are processed from outside in.  While
> processing a loop, duplicate close-phis are eliminated.  However,
> eliminating duplicate close-phis for an inner loop can sometimes create
> duplicate close-phis for an already-processed outer loop.  This patch
> detects when this may have occurred and repeats the removal of duplicate
> close-phis as necessary.
> 
> The problem was noted on ibm/4_6-branch and 4_6-branch; it is apparently
> latent on trunk.  The same patch can be applied to all three branches.
> 
> Bootstrapped and regression-tested on powerpc64-linux.  OK to commit to
> these three branches?
> 
> Thanks,
> Bill
> 
> 
> 2011-09-13  Bill Schmidt  <wschmidt@linux.vnet.ibm.com>
> 
> 	* graphite-scop-detection.c (make_close_phi_nodes_unique):  New
> 	forward declaration.
> 	(remove_duplicate_close_phi): Detect and repair creation of
> 	duplicate close-phis for a containing loop.
> 
> 
> Index: gcc/graphite-scop-detection.c
> ===================================================================
> --- gcc/graphite-scop-detection.c	(revision 178829)
> +++ gcc/graphite-scop-detection.c	(working copy)
> @@ -30,6 +30,9 @@ along with GCC; see the file COPYING3.  If not see
>  #include "tree-pass.h"
>  #include "sese.h"
> 
> +/* Forward declarations.  */
> +static void make_close_phi_nodes_unique (basic_block);
> +
>  #ifdef HAVE_cloog
>  #include "ppl_c.h"
>  #include "graphite-ppl.h"
> @@ -1231,6 +1234,13 @@ remove_duplicate_close_phi (gimple phi, gimple_stm
>  	SET_USE (use_p, res);
> 
>        update_stmt (use_stmt);
> +      
> +      /* It is possible that we just created a duplicate close-phi
> +	 for an already-processed containing loop.  Check for this
> +	 case and clean it up.  */
> +      if (gimple_code (use_stmt) == GIMPLE_PHI
> +	  && gimple_phi_num_args (use_stmt) == 1)
> +	make_close_phi_nodes_unique (gimple_bb (use_stmt));
>      }
> 
>    remove_phi_node (gsi, true);
> 
>
Richard Guenther - Sept. 29, 2011, 8:58 a.m.
On Thu, Sep 29, 2011 at 12:10 AM, William J. Schmidt
<wschmidt@linux.vnet.ibm.com> wrote:
> Hi there,
>
> Ping.  I'm seeking approval for this fix on trunk and 4_6-branch.
> Thanks!

Ok.

Thanks,
Richard.

> Bill
>
> On Tue, 2011-09-13 at 17:55 -0500, William J. Schmidt wrote:
>> Greetings,
>>
>> The code to build scops (static control parts) for graphite first
>> rewrites loops into canonical loop-closed SSA form.  PR50183 identifies
>> a scenario where the results do not fulfill all required invariants of
>> this form.  In particular, a value defined inside a loop and used
>> outside that loop must reach exactly one definition, which must be a
>> single-argument PHI node called a close-phi.  When nested loops exist,
>> it is possible that, following the rewrite, a definition may reach two
>> close-phis.  This patch corrects that problem.
>>
>> The problem arises because loops are processed from outside in.  While
>> processing a loop, duplicate close-phis are eliminated.  However,
>> eliminating duplicate close-phis for an inner loop can sometimes create
>> duplicate close-phis for an already-processed outer loop.  This patch
>> detects when this may have occurred and repeats the removal of duplicate
>> close-phis as necessary.
>>
>> The problem was noted on ibm/4_6-branch and 4_6-branch; it is apparently
>> latent on trunk.  The same patch can be applied to all three branches.
>>
>> Bootstrapped and regression-tested on powerpc64-linux.  OK to commit to
>> these three branches?
>>
>> Thanks,
>> Bill
>>
>>
>> 2011-09-13  Bill Schmidt  <wschmidt@linux.vnet.ibm.com>
>>
>>       * graphite-scop-detection.c (make_close_phi_nodes_unique):  New
>>       forward declaration.
>>       (remove_duplicate_close_phi): Detect and repair creation of
>>       duplicate close-phis for a containing loop.
>>
>>
>> Index: gcc/graphite-scop-detection.c
>> ===================================================================
>> --- gcc/graphite-scop-detection.c     (revision 178829)
>> +++ gcc/graphite-scop-detection.c     (working copy)
>> @@ -30,6 +30,9 @@ along with GCC; see the file COPYING3.  If not see
>>  #include "tree-pass.h"
>>  #include "sese.h"
>>
>> +/* Forward declarations.  */
>> +static void make_close_phi_nodes_unique (basic_block);
>> +
>>  #ifdef HAVE_cloog
>>  #include "ppl_c.h"
>>  #include "graphite-ppl.h"
>> @@ -1231,6 +1234,13 @@ remove_duplicate_close_phi (gimple phi, gimple_stm
>>       SET_USE (use_p, res);
>>
>>        update_stmt (use_stmt);
>> +
>> +      /* It is possible that we just created a duplicate close-phi
>> +      for an already-processed containing loop.  Check for this
>> +      case and clean it up.  */
>> +      if (gimple_code (use_stmt) == GIMPLE_PHI
>> +       && gimple_phi_num_args (use_stmt) == 1)
>> +     make_close_phi_nodes_unique (gimple_bb (use_stmt));
>>      }
>>
>>    remove_phi_node (gsi, true);
>>
>>
>
>
>
Tobias Grosser - Sept. 29, 2011, 9:03 a.m.
On 09/29/2011 09:58 AM, Richard Guenther wrote:
> On Thu, Sep 29, 2011 at 12:10 AM, William J. Schmidt
> <wschmidt@linux.vnet.ibm.com>  wrote:
>> Hi there,
>>
>> Ping.  I'm seeking approval for this fix on trunk and 4_6-branch.
>> Thanks!
>
> Ok.
Yes, also looks good from me. Though, you may want to move the forward
declaration after the "#ifdef HAVE_CLOOG". This makes it clearer that 
the whole file is not compiled, if cloog is not available.

Cheers
Tobi
William J. Schmidt - Sept. 29, 2011, 12:22 p.m.
On Thu, 2011-09-29 at 10:03 +0100, Tobias Grosser wrote:
> On 09/29/2011 09:58 AM, Richard Guenther wrote:
> > On Thu, Sep 29, 2011 at 12:10 AM, William J. Schmidt
> > <wschmidt@linux.vnet.ibm.com>  wrote:
> >> Hi there,
> >>
> >> Ping.  I'm seeking approval for this fix on trunk and 4_6-branch.
> >> Thanks!
> >
> > Ok.
> Yes, also looks good from me. Though, you may want to move the forward
> declaration after the "#ifdef HAVE_CLOOG". This makes it clearer that 
> the whole file is not compiled, if cloog is not available.

Good point.  I'll make that change.

Thanks!
Bill

> 
> Cheers
> Tobi

Patch

Index: gcc/graphite-scop-detection.c
===================================================================
--- gcc/graphite-scop-detection.c	(revision 178829)
+++ gcc/graphite-scop-detection.c	(working copy)
@@ -30,6 +30,9 @@  along with GCC; see the file COPYING3.  If not see
 #include "tree-pass.h"
 #include "sese.h"
 
+/* Forward declarations.  */
+static void make_close_phi_nodes_unique (basic_block);
+
 #ifdef HAVE_cloog
 #include "ppl_c.h"
 #include "graphite-ppl.h"
@@ -1231,6 +1234,13 @@  remove_duplicate_close_phi (gimple phi, gimple_stm
 	SET_USE (use_p, res);
 
       update_stmt (use_stmt);
+      
+      /* It is possible that we just created a duplicate close-phi
+	 for an already-processed containing loop.  Check for this
+	 case and clean it up.  */
+      if (gimple_code (use_stmt) == GIMPLE_PHI
+	  && gimple_phi_num_args (use_stmt) == 1)
+	make_close_phi_nodes_unique (gimple_bb (use_stmt));
     }
 
   remove_phi_node (gsi, true);