| Message ID | 1315954554.27725.26.camel@gnopaine |
|---|---|
| State | New |
| Headers | show |
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); > >
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); >> >> > > >
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
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
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);