[Bug middle-end/43464] copy prop breaks loop closed SSA form

[spop at gcc dot gnu dot org]

--- Comment #15 from spop at gcc dot gnu dot org  2010-03-31 18:38 ---
Subject: Bug 43464

Author: spop
Date: Wed Mar 31 18:37:41 2010
New Revision: 157889

URL: http://gcc.gnu.org/viewcvs?root=gccview=revrev=157889
Log:
Fix PR43464: copyprop should maintain loop close phi nodes with multiple
arguments.

2010-03-30  Richard Guenther  rguent...@suse.de
Zdenek Dvorak  o...@ucw.cz
Sebastian Pop  sebastian@amd.com

PR middle-end/43464
* tree-ssa-copy.c (init_copy_prop): Handle loop close phi nodes
with multiple arguments.
(execute_copy_prop): Remove call to rewrite_into_loop_closed_ssa.

Modified:
trunk/gcc/ChangeLog.graphite
trunk/gcc/tree-ssa-copy.c


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=43464

[Bug middle-end/43464] copy prop breaks loop closed SSA form

[spop at gcc dot gnu dot org]

--- Comment #16 from spop at gcc dot gnu dot org  2010-03-31 18:48 ---
Fixed.


-- 

spop at gcc dot gnu dot org changed:

   What|Removed |Added

 Status|NEW |RESOLVED
 Resolution||FIXED


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=43464

[Bug middle-end/43464] copy prop breaks loop closed SSA form

[spop at gcc dot gnu dot org]

--- Comment #14 from spop at gcc dot gnu dot org  2010-03-30 16:57 ---
Subject: Bug 43464

Author: spop
Date: Tue Mar 30 16:56:49 2010
New Revision: 157828

URL: http://gcc.gnu.org/viewcvs?root=gccview=revrev=157828
Log:
Fix PR43464: copyprop should maintain loop close phi nodes with multiple
arguments.

2010-03-30  Richard Guenther  rguent...@suse.de
Zdenek Dvorak  o...@ucw.cz
Sebastian Pop  sebastian@amd.com

PR middle-end/43464
* tree-ssa-copy.c (init_copy_prop): Handle loop close phi nodes
with multiple arguments.
(execute_copy_prop): Remove call to rewrite_into_loop_closed_ssa.

Modified:
branches/graphite/gcc/ChangeLog.graphite
branches/graphite/gcc/tree-ssa-copy.c


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=43464

[Bug middle-end/43464] copy prop breaks loop closed SSA form

[spop at gcc dot gnu dot org]

--- Comment #13 from spop at gcc dot gnu dot org  2010-03-22 15:47 ---
Note that 

   * tree-ssa-copy.c (init_copy_prop): Loop closed phi nodes can
contain more than one argument.

breaks both 464.h264ref and 416.gamess with -O3.  See for example
http://groups.google.com/group/gcc-graphite-test/browse_thread/thread/f5b0d912e90b598f


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=43464

[Bug middle-end/43464] copy prop breaks loop closed SSA form

[spop at gcc dot gnu dot org]

--- Comment #1 from spop at gcc dot gnu dot org  2010-03-21 07:01 ---
Created an attachment (id=20148)
 -- (http://gcc.gnu.org/bugzilla/attachment.cgi?id=20148action=view)
pr43464.i


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=43464

[Bug middle-end/43464] copy prop breaks loop closed SSA form

[spop at gcc dot gnu dot org]

--- Comment #2 from spop at gcc dot gnu dot org  2010-03-21 07:06 ---
Created an attachment (id=20149)
 -- (http://gcc.gnu.org/bugzilla/attachment.cgi?id=20149action=view)
proposed fix

The proposed fix is to recompute the loop closed SSA form after copy
propagation.


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=43464

[Bug middle-end/43464] copy prop breaks loop closed SSA form

[spop at gcc dot gnu dot org]

--- Comment #3 from spop at gcc dot gnu dot org  2010-03-21 07:32 ---
Subject: Bug 43464

Author: spop
Date: Sun Mar 21 07:32:43 2010
New Revision: 157602

URL: http://gcc.gnu.org/viewcvs?root=gccview=revrev=157602
Log:
Fix PR43464: update loop close SSA once copy prop is done.

2010-03-21  Sebastian Pop  sebastian@amd.com

PR middle-end/43464
* tree-ssa-copy.c (execute_copy_prop): Call
rewrite_into_loop_closed_ssa
and verify_loop_closed_ssa when copy prop is executed in the LNO.

* gcc.dg/graphite/id-pr43464.c: New.

Added:
branches/graphite/gcc/testsuite/gcc.dg/graphite/id-pr43464.c
Modified:
branches/graphite/gcc/ChangeLog.graphite
branches/graphite/gcc/tree-ssa-copy.c


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=43464

[Bug middle-end/43464] copy prop breaks loop closed SSA form

[steven at gcc dot gnu dot org]

--- Comment #4 from steven at gcc dot gnu dot org  2010-03-21 09:54 ---
Why such a big hammer? You should be able to figure out which copy props are
allowed and which should be disallowed in loop-closed SSA form.

Is if (current_loops) the right test here? This will break if Zdenek's
patches to keep loops around throughout ever makes it to the trunk.


-- 

steven at gcc dot gnu dot org changed:

   What|Removed |Added

 Status|UNCONFIRMED |NEW
 Ever Confirmed|0   |1
   Last reconfirmed|-00-00 00:00:00 |2010-03-21 09:54:56
   date||


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=43464

[Bug middle-end/43464] copy prop breaks loop closed SSA form

[sebpop at gmail dot com]

--- Comment #5 from sebpop at gmail dot com  2010-03-21 16:08 ---
Subject: Re:  copy prop breaks loop closed SSA form

On Sun, Mar 21, 2010 at 04:54, steven at gcc dot gnu dot org
gcc-bugzi...@gcc.gnu.org wrote:
 Why such a big hammer?

'cause I don't want to add more bugs,
and no I don't think compile time matters.

 You should be able to figure out which copy props are
 allowed and which should be disallowed in loop-closed SSA form.

patches are welcome.

 Is if (current_loops) the right test here? This will break if Zdenek's
 patches to keep loops around throughout ever makes it to the trunk.

please propose a better patch.

Thanks,
Sebastian


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=43464

[Bug middle-end/43464] copy prop breaks loop closed SSA form

[spop at gcc dot gnu dot org]

--- Comment #6 from spop at gcc dot gnu dot org  2010-03-21 17:01 ---
Further reduced testcase:

typedef struct regnode
{
  char flags;
} regnode;
extern const unsigned char A[];

char *foo (regnode *c, char *s, int norun)
{
  int uskip;
  while (s + (uskip = A[*s]))
{
  if ((c-flags || bar (c))  norun)
goto got_it;
  s += uskip;
}
 got_it:
  return s;
}

Needs an extra patch to trigger the ICE:

diff --git a/gcc/passes.c b/gcc/passes.c
index 1ac8694..620487f 100644
--- a/gcc/passes.c
+++ b/gcc/passes.c
@@ -894,6 +894,10 @@ init_optimization_passes (void)
  NEXT_PASS (pass_check_data_deps);
  NEXT_PASS (pass_loop_distribution);
  NEXT_PASS (pass_linear_transform);
+ NEXT_PASS (pass_copy_prop);
+ NEXT_PASS (pass_dce_loop);
+ NEXT_PASS (pass_lim);
+
  NEXT_PASS (pass_graphite_transforms);
{
  struct opt_pass **p = pass_graphite_transforms.pass.sub;


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=43464

[Bug middle-end/43464] copy prop breaks loop closed SSA form

[spop at gcc dot gnu dot org]

--- Comment #7 from spop at gcc dot gnu dot org  2010-03-21 17:17 ---
The problem is that copyprop does this change:

@@ -25,7 +25,7 @@
   bb 12:
 # .MEM_16 = PHI .MEM_18(10), .MEM_20(11)
 # s_66 = PHI s_1(10), s_1(11)
-s_13 = s_66;
+s_13 = s_1;
 goto bb 16 (got_it);

   }

then verify_loop_closed_ssa () complains about the fact that s_1 is
defined in loop_1 and used outside loop_1 in a non close-phi node.


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=43464

[Bug middle-end/43464] copy prop breaks loop closed SSA form

[rguenth at gcc dot gnu dot org]

--- Comment #8 from rguenth at gcc dot gnu dot org  2010-03-21 23:05 ---
(In reply to comment #7)
 The problem is that copyprop does this change:
 
 @@ -25,7 +25,7 @@
bb 12:
  # .MEM_16 = PHI .MEM_18(10), .MEM_20(11)
  # s_66 = PHI s_1(10), s_1(11)
 -s_13 = s_66;
 +s_13 = s_1;
  goto bb 16 (got_it);
 
}
 
 then verify_loop_closed_ssa () complains about the fact that s_1 is
 defined in loop_1 and used outside loop_1 in a non close-phi node.

Ah, copyprop avoids _single_ arg PHIs because that may break loop-closed
SSA form.  Why does loop closed SSA form suddenly have double-arg
loop-closed PHI nodes?

Indeed Sebastians patch is a non-suitable hammer.  Copyprop already tries
to avoid breaking loop-closed SSA form.


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=43464

[Bug middle-end/43464] copy prop breaks loop closed SSA form

[rguenth at gcc dot gnu dot org]

--- Comment #9 from rguenth at gcc dot gnu dot org  2010-03-21 23:07 ---
(In reply to comment #8)
 (In reply to comment #7)
  The problem is that copyprop does this change:
  
  @@ -25,7 +25,7 @@
 bb 12:
   # .MEM_16 = PHI .MEM_18(10), .MEM_20(11)
   # s_66 = PHI s_1(10), s_1(11)
  -s_13 = s_66;
  +s_13 = s_1;
   goto bb 16 (got_it);
  
 }
  
  then verify_loop_closed_ssa () complains about the fact that s_1 is
  defined in loop_1 and used outside loop_1 in a non close-phi node.
 
 Ah, copyprop avoids _single_ arg PHIs because that may break loop-closed
 SSA form.  Why does loop closed SSA form suddenly have double-arg
 loop-closed PHI nodes?
 
 Indeed Sebastians patch is a non-suitable hammer.  Copyprop already tries
 to avoid breaking loop-closed SSA form.

  /* In loop-closed SSA form do not copy-propagate through
 PHI nodes.  Technically this is only needed for loop
 exit PHIs, but this is difficult to query.  */
  || (current_loops
   gimple_phi_num_args (phi) == 1
   loops_state_satisfies_p (LOOP_CLOSED_SSA)))

Thus it seems that  gimple_phi_num_args (phi) == 1 should be removed.


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=43464

[Bug middle-end/43464] copy prop breaks loop closed SSA form

[spop at gcc dot gnu dot org]

--- Comment #10 from spop at gcc dot gnu dot org  2010-03-22 00:17 ---
Yes, this patch does fix the problem.

diff --git a/gcc/tree-ssa-copy.c b/gcc/tree-ssa-copy.c
index 57c6558..61e32cc 100644
--- a/gcc/tree-ssa-copy.c
+++ b/gcc/tree-ssa-copy.c
@@ -797,7 +797,6 @@ init_copy_prop (void)
 PHI nodes.  Technically this is only needed for loop
 exit PHIs, but this is difficult to query.  */
  || (current_loops
-  gimple_phi_num_args (phi) == 1
   loops_state_satisfies_p (LOOP_CLOSED_SSA)))
 prop_set_simulate_again (phi, false);
  else

I will bootstrap and test this again.


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=43464

[Bug middle-end/43464] copy prop breaks loop closed SSA form

[spop at gcc dot gnu dot org]

--- Comment #11 from spop at gcc dot gnu dot org  2010-03-22 00:26 ---
I would still like to see some extra checking after copyprop:
would this extra check be ok to commit with the fix?

diff --git a/gcc/tree-ssa-copy.c b/gcc/tree-ssa-copy.c
index 61e32cc..011a80a 100644
--- a/gcc/tree-ssa-copy.c
+++ b/gcc/tree-ssa-copy.c
@@ -967,6 +967,13 @@ execute_copy_prop (void)
   init_copy_prop ();
   ssa_propagate (copy_prop_visit_stmt, copy_prop_visit_phi_node);
   fini_copy_prop ();
+
+#ifdef ENABLE_CHECKING
+  if (current_loops
+   loops_state_satisfies_p (LOOP_CLOSED_SSA))
+verify_loop_closed_ssa ();
+#endif
+
   return 0;
 }



-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=43464

[Bug middle-end/43464] copy prop breaks loop closed SSA form

[spop at gcc dot gnu dot org]

--- Comment #12 from spop at gcc dot gnu dot org  2010-03-22 01:29 ---
Subject: Bug 43464

Author: spop
Date: Mon Mar 22 01:28:51 2010
New Revision: 157617

URL: http://gcc.gnu.org/viewcvs?root=gccview=revrev=157617
Log:
Fix PR43464: loop close phi nodes can contain more than one argument.

2010-03-21  Sebastian Pop  sebastian@amd.com
Richard Guenther  rguent...@suse.de

PR middle-end/43464
* tree-ssa-copy.c (init_copy_prop): Loop closed phi nodes can
contain more than one argument.
(execute_copy_prop): Revert the previous change: do not call
rewrite_into_loop_closed_ssa.

* gcc.dg/graphite/id-pr43464.c: Remove compile warning.
* gcc.dg/graphite/id-pr43464-1.c: New.

Added:
branches/graphite/gcc/testsuite/gcc.dg/graphite/id-pr43464-1.c
Modified:
branches/graphite/gcc/ChangeLog.graphite
branches/graphite/gcc/testsuite/gcc.dg/graphite/id-pr43464.c
branches/graphite/gcc/tree-ssa-copy.c


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=43464