Patchwork Fix PR52881, more loop preserving fallout with RTL optimizers

login
register
mail settings
Submitter Richard Guenther
Date April 10, 2012, 2:56 p.m.
Message ID <Pine.LNX.4.64.1204101653130.17575@jbgna.fhfr.qr>
Download mbox | patch
Permalink /patch/151610/
State New
Headers show

Comments

Richard Guenther - April 10, 2012, 2:56 p.m.
RTL optimizers perform CFG adjustments themselves rather than relying
on cleanup_cfg (as the gimple level does).  This makes it necessary
to handle things all-over-the-place.  This case is if-conversion
performing forwarder block removal (well, it calls it "speculation"),
removing empty loop latches.  That's unwanted, thus the following
patch makes sure we do not do this.

I'm sure more RTL optimiziation fallout will pop up - and I wonder
if we should simply avoid modifying the CFG all over the place and
instead do that in cleanup_cfg.  Thus, in the if-conversion case,
simply do the speculation and leave the empty forwarder blocks
to be cleaned up by cleanup_cfg (or for CSE to leave unconditional
jumps conditional, as if (0/1), for example).

Bootstrapped on x86_64-unknown-linux-gnu, testing in progress.

Richard.

2012-04-10  Richard Guenther  <rguenther@suse.de>

	PR rtl-optimization/52881
	* ifcvt.c (find_if_case_2): Avoid speculating loop latches.

	* gcc.dg/torture/pr52881.c: New testcase.
	* gcc.dg/torture/pr52913.c: Likewise.

Index: gcc/ifcvt.c
===================================================================
*** gcc/ifcvt.c	(revision 186274)
--- gcc/ifcvt.c	(working copy)
*************** find_if_case_2 (basic_block test_bb, edg
*** 3927,3932 ****
--- 3927,3937 ----
    edge else_succ;
    int then_prob, else_prob;
  
+   /* We do not want to speculate (empty) loop latches.  */
+   if (current_loops
+       && else_bb->loop_father->latch == else_bb)
+     return FALSE;
+ 
    /* If we are partitioning hot/cold basic blocks, we don't want to
       mess up unconditional or indirect jumps that cross between hot
       and cold sections.
Index: gcc/testsuite/gcc.dg/torture/pr52881.c
===================================================================
*** gcc/testsuite/gcc.dg/torture/pr52881.c	(revision 0)
--- gcc/testsuite/gcc.dg/torture/pr52881.c	(revision 0)
***************
*** 0 ****
--- 1,35 ----
+ /* { dg-do compile } */
+ 
+ int a, b, c, d, e, f, h, i, j, k, l, m, n, o;
+ static int g;
+ int
+ fn1 () {
+     for (;; ++f)
+       if (e)
+ 	break;
+     return 0;
+ }
+ unsigned char fn2 ();
+ void
+ fn3 () {
+ lbl_220:
+     if (j) {
+ lbl_221:
+ 	l = (g || b) <= fn1 ();
+ 	for (;;) {
+ 	    g = 0;
+ 	    fn2 ();
+ 	    if (k)
+ 	      goto lbl_220;
+ 	    break;
+ 	}
+ 	if (l)
+ 	  goto lbl_221;
+     }
+ }
+ unsigned char
+ fn2 () {
+     o = d ? 0 : c;
+     h = m | a % o != n;
+     return i;
+ }
Steven Bosscher - April 10, 2012, 3:46 p.m.
On Tue, Apr 10, 2012 at 4:56 PM, Richard Guenther <rguenther@suse.de> wrote:
> I'm sure more RTL optimiziation fallout will pop up - and I wonder
> if we should simply avoid modifying the CFG all over the place and
> instead do that in cleanup_cfg.  Thus, in the if-conversion case,
> simply do the speculation and leave the empty forwarder blocks
> to be cleaned up by cleanup_cfg (or for CSE to leave unconditional
> jumps conditional, as if (0/1), for example).

The problem with that could be that if(0|1) is not a valid instruction
for every machine. You may need to set a register (pseudo, or hard
reg) that may not be available (e.g. if it is live).

Ciao!
Steven
Richard Guenther - April 11, 2012, 8:08 a.m.
On Tue, 10 Apr 2012, Steven Bosscher wrote:

> On Tue, Apr 10, 2012 at 4:56 PM, Richard Guenther <rguenther@suse.de> wrote:
> > I'm sure more RTL optimiziation fallout will pop up - and I wonder
> > if we should simply avoid modifying the CFG all over the place and
> > instead do that in cleanup_cfg.  Thus, in the if-conversion case,
> > simply do the speculation and leave the empty forwarder blocks
> > to be cleaned up by cleanup_cfg (or for CSE to leave unconditional
> > jumps conditional, as if (0/1), for example).
> 
> The problem with that could be that if(0|1) is not a valid instruction
> for every machine. You may need to set a register (pseudo, or hard
> reg) that may not be available (e.g. if it is live).

Hmm.  But we'd have such if(0|1) only in the short period between
the pass changing it to that and the next cleanup_cfg run, which would
ideally be right after that pass.  Alternatively passes could leave
the CFG not updated (thus, keep dead edges) but update the conditional
jump instructions only.

That said, currently I'm following the idea of fixing up all places
that pop up ...

Richard.

Patch

Index: gcc/testsuite/gcc.dg/torture/pr52913.c
===================================================================
--- gcc/testsuite/gcc.dg/torture/pr52913.c	(revision 0)
+++ gcc/testsuite/gcc.dg/torture/pr52913.c	(revision 0)
@@ -0,0 +1,18 @@ 
+/* { dg-do compile } */
+
+int a, b, c, d, e;
+void
+fn1 ()
+{
+lbl_101:
+  e = 0;
+lbl_274:
+  for (c = 0; c < 1; c = a)
+    if (d)
+      if (b)
+	goto lbl_101;
+      else
+	break;
+  d = 1;
+  goto lbl_274;
+}