Fix PR56466

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

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
> 
>

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

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