Patchwork Fix PR56466

login
register
mail settings
Submitter Marek Polacek
Date Feb. 27, 2013, 2:47 p.m.
Message ID <20130227144759.GB15445@redhat.com>
Download mbox | patch
Permalink /patch/223621/
State New
Headers show

Comments

Marek Polacek - Feb. 27, 2013, 2:47 p.m.
This patch does two things: it fixes PR56466 by calling fix_loop_structure
in case we're changing a loop, and secondly, it makes verifiyng
loops in the unroller cheaper: there's no need to verify each loop,
we can do it once after all transformations (verify_loop_structure is
being called via fix_loop_structure).

Regtested/bootstrapped on x86_64-linux, ok for trunk?

2013-02-27  Marek Polacek  <polacek@redhat.com>

	PR rtl-optimization/56466
	* loop-unroll.c (unroll_and_peel_loops): Call fix_loop_structure
	if we're changing a loop.
	(peel_loops_completely): Likewise.

	* gcc.dg/pr56466.c: New test.


	Marek
Richard Guenther - Feb. 27, 2013, 3:21 p.m.
On Wed, 27 Feb 2013, Marek Polacek wrote:

> This patch does two things: it fixes PR56466 by calling fix_loop_structure
> in case we're changing a loop, and secondly, it makes verifiyng
> loops in the unroller cheaper: there's no need to verify each loop,
> we can do it once after all transformations (verify_loop_structure is
> being called via fix_loop_structure).
> 
> Regtested/bootstrapped on x86_64-linux, ok for trunk?

Ok.

Thanks,
Richard.

> 2013-02-27  Marek Polacek  <polacek@redhat.com>
> 
> 	PR rtl-optimization/56466
> 	* loop-unroll.c (unroll_and_peel_loops): Call fix_loop_structure
> 	if we're changing a loop.
> 	(peel_loops_completely): Likewise.
> 
> 	* gcc.dg/pr56466.c: New test.
> 
> --- gcc/loop-unroll.c.mp	2013-02-27 11:13:01.915430193 +0100
> +++ gcc/loop-unroll.c	2013-02-27 14:57:09.873168342 +0100
> @@ -207,7 +207,7 @@ void
>  unroll_and_peel_loops (int flags)
>  {
>    struct loop *loop;
> -  bool check;
> +  bool changed = false;
>    loop_iterator li;
>  
>    /* First perform complete loop peeling (it is almost surely a win,
> @@ -220,7 +220,6 @@ unroll_and_peel_loops (int flags)
>    /* Scan the loops, inner ones first.  */
>    FOR_EACH_LOOP (li, loop, LI_FROM_INNERMOST)
>      {
> -      check = true;
>        /* And perform the appropriate transformations.  */
>        switch (loop->lpt_decision.decision)
>  	{
> @@ -229,30 +228,33 @@ unroll_and_peel_loops (int flags)
>  	  gcc_unreachable ();
>  	case LPT_PEEL_SIMPLE:
>  	  peel_loop_simple (loop);
> +	  changed |= true;
>  	  break;
>  	case LPT_UNROLL_CONSTANT:
>  	  unroll_loop_constant_iterations (loop);
> +	  changed |= true;
>  	  break;
>  	case LPT_UNROLL_RUNTIME:
>  	  unroll_loop_runtime_iterations (loop);
> +	  changed |= true;
>  	  break;
>  	case LPT_UNROLL_STUPID:
>  	  unroll_loop_stupid (loop);
> +	  changed |= true;
>  	  break;
>  	case LPT_NONE:
> -	  check = false;
>  	  break;
>  	default:
>  	  gcc_unreachable ();
>  	}
> -      if (check)
> -	{
> -#ifdef ENABLE_CHECKING
> -	  verify_loop_structure ();
> -#endif
> -	}
>      }
>  
> +    if (changed)
> +      {
> +	calculate_dominance_info (CDI_DOMINATORS);
> +	fix_loop_structure (NULL);
> +      }
> +
>    iv_analysis_done ();
>  }
>  
> @@ -283,6 +285,7 @@ peel_loops_completely (int flags)
>  {
>    struct loop *loop;
>    loop_iterator li;
> +  bool changed = false;
>  
>    /* Scan the loops, the inner ones first.  */
>    FOR_EACH_LOOP (li, loop, LI_FROM_INNERMOST)
> @@ -306,11 +309,15 @@ peel_loops_completely (int flags)
>  	{
>  	  report_unroll_peel (loop, locus);
>  	  peel_loop_completely (loop);
> -#ifdef ENABLE_CHECKING
> -	  verify_loop_structure ();
> -#endif
> +	  changed = true;
>  	}
>      }
> +
> +    if (changed)
> +      {
> +	calculate_dominance_info (CDI_DOMINATORS);
> +	fix_loop_structure (NULL);
> +      }
>  }
>  
>  /* Decide whether unroll or peel loops (depending on FLAGS) and how much.  */
> --- gcc/testsuite/gcc.dg/pr56466.c.mp	2013-02-27 12:43:44.468886756 +0100
> +++ gcc/testsuite/gcc.dg/pr56466.c	2013-02-27 12:46:36.385390735 +0100
> @@ -0,0 +1,31 @@
> +/* PR rtl-optimization/56466 */
> +/* { dg-do compile } */
> +/* { dg-options "-O2 -w -funroll-loops" } */
> +
> +int a, b, c;
> +
> +void
> +f (void)
> +{
> +  for (; b; b++)
> +    {
> +      if (0)
> +	for (; b < 0; b++)
> +	  if (1 % 0)
> +	    {
> +	      while (1)
> +		{
> +		  a = 0;
> +		lbl1:
> +		  c++;
> +		}
> +	    lbl2:
> +	      ;
> +	    }
> +
> +      goto lbl1;
> +    }
> +
> +  a = 0;
> +  goto lbl2;
> +}
> 
> 	Marek
> 
>
Miles Bader - Feb. 28, 2013, 5:32 a.m.
Marek Polacek <polacek@redhat.com> writes:
> +  bool changed = false;
> +	  changed |= true;
> +	  changed |= true;
> +	  changed |= true;
> +	  changed |= true;
> +    if (changed)

Why do you use "|=" ...?  Isn't it equivalent to just "=" (which is
more clear) for a boolean?

Thanks,

-miles
Marek Polacek - Feb. 28, 2013, 9:49 a.m.
On Thu, Feb 28, 2013 at 02:32:02PM +0900, Miles Bader wrote:
> Marek Polacek <polacek@redhat.com> writes:
> > +  bool changed = false;
> > +	  changed |= true;
> > +	  changed |= true;
> > +	  changed |= true;
> > +	  changed |= true;
> > +    if (changed)
> 
> Why do you use "|=" ...?  Isn't it equivalent to just "=" (which is
> more clear) for a boolean?

Ah, yeah, it's a remnant; I had "changed = false;" there as well.
Will adjust, thanks,

	Marek

Patch

--- gcc/loop-unroll.c.mp	2013-02-27 11:13:01.915430193 +0100
+++ gcc/loop-unroll.c	2013-02-27 14:57:09.873168342 +0100
@@ -207,7 +207,7 @@  void
 unroll_and_peel_loops (int flags)
 {
   struct loop *loop;
-  bool check;
+  bool changed = false;
   loop_iterator li;
 
   /* First perform complete loop peeling (it is almost surely a win,
@@ -220,7 +220,6 @@  unroll_and_peel_loops (int flags)
   /* Scan the loops, inner ones first.  */
   FOR_EACH_LOOP (li, loop, LI_FROM_INNERMOST)
     {
-      check = true;
       /* And perform the appropriate transformations.  */
       switch (loop->lpt_decision.decision)
 	{
@@ -229,30 +228,33 @@  unroll_and_peel_loops (int flags)
 	  gcc_unreachable ();
 	case LPT_PEEL_SIMPLE:
 	  peel_loop_simple (loop);
+	  changed |= true;
 	  break;
 	case LPT_UNROLL_CONSTANT:
 	  unroll_loop_constant_iterations (loop);
+	  changed |= true;
 	  break;
 	case LPT_UNROLL_RUNTIME:
 	  unroll_loop_runtime_iterations (loop);
+	  changed |= true;
 	  break;
 	case LPT_UNROLL_STUPID:
 	  unroll_loop_stupid (loop);
+	  changed |= true;
 	  break;
 	case LPT_NONE:
-	  check = false;
 	  break;
 	default:
 	  gcc_unreachable ();
 	}
-      if (check)
-	{
-#ifdef ENABLE_CHECKING
-	  verify_loop_structure ();
-#endif
-	}
     }
 
+    if (changed)
+      {
+	calculate_dominance_info (CDI_DOMINATORS);
+	fix_loop_structure (NULL);
+      }
+
   iv_analysis_done ();
 }
 
@@ -283,6 +285,7 @@  peel_loops_completely (int flags)
 {
   struct loop *loop;
   loop_iterator li;
+  bool changed = false;
 
   /* Scan the loops, the inner ones first.  */
   FOR_EACH_LOOP (li, loop, LI_FROM_INNERMOST)
@@ -306,11 +309,15 @@  peel_loops_completely (int flags)
 	{
 	  report_unroll_peel (loop, locus);
 	  peel_loop_completely (loop);
-#ifdef ENABLE_CHECKING
-	  verify_loop_structure ();
-#endif
+	  changed = true;
 	}
     }
+
+    if (changed)
+      {
+	calculate_dominance_info (CDI_DOMINATORS);
+	fix_loop_structure (NULL);
+      }
 }
 
 /* Decide whether unroll or peel loops (depending on FLAGS) and how much.  */
--- gcc/testsuite/gcc.dg/pr56466.c.mp	2013-02-27 12:43:44.468886756 +0100
+++ gcc/testsuite/gcc.dg/pr56466.c	2013-02-27 12:46:36.385390735 +0100
@@ -0,0 +1,31 @@ 
+/* PR rtl-optimization/56466 */
+/* { dg-do compile } */
+/* { dg-options "-O2 -w -funroll-loops" } */
+
+int a, b, c;
+
+void
+f (void)
+{
+  for (; b; b++)
+    {
+      if (0)
+	for (; b < 0; b++)
+	  if (1 % 0)
+	    {
+	      while (1)
+		{
+		  a = 0;
+		lbl1:
+		  c++;
+		}
+	    lbl2:
+	      ;
+	    }
+
+      goto lbl1;
+    }
+
+  a = 0;
+  goto lbl2;
+}