[patch] Fix PR43464: update loop closed SSA form once copy prop is done

[Richard Guenther]

On Fri, 26 Mar 2010, Jack Howarth wrote:

> On Fri, Mar 26, 2010 at 10:51:12AM +0100, Richard Guenther wrote:
> > On Wed, 24 Mar 2010, Sebastian Pop wrote:
> > 
> > > On Tue, Mar 23, 2010 at 22:40, Kenneth Zadeck
> > > <Kenneth.Zadeck@naturalbridge.com> wrote:
> > > > Let me try to bring some perspective back into this discussion,
> > > > because it is clear that this is getting out of hand.
> > > >
> > > > There are two solutions that must be considered: a short term one and
> > > > a long term one.
> > > >
> > > > At this stage, a rewrite of a pass of the compiler is not acceptable,
> > > > so calling that rewrite_into_closed_loop_ssa form in a place
> > > > immediately after the pass that messed it up is the correct short term
> > > > solution. Â And it is purely Richard's call as to how careful that
> > > > patch needs to be about checking if things get messed up.
> > > >
> > > 
> > > Richi, would the patch below be ok for 4.5?
> > > This passes bootstrap and test on amd64-linux,
> > > and the SPEC 2006 benchmarks with the all the
> > > graphite transforms enabled.
> > > 
> > > I am proposing to keep the PR43464 open and mark it
> > > high priority for 4.6.
> > > 
> > > Thanks,
> > > Sebastian
> > > 
> > > ----------------------------- gcc/tree-ssa-copy.c -----------------------------
> > > index 4b8d0b9..683ea64 100644
> > > @@ -968,6 +968,21 @@ execute_copy_prop (void)
> > >    init_copy_prop ();
> > >    ssa_propagate (copy_prop_visit_stmt, copy_prop_visit_phi_node);
> > >    fini_copy_prop ();
> > > +
> > > +  /* FIXME: The call to rewrite_into_loop_closed_ssa should be
> > > +     removed: this code is a temporary work around needed to maintain
> > > +     the loop closed SSA form when copy propagation is called after
> > > +     loop unswitching pass.  The second call to copy propagation is
> > > +     done as a sub-pass of the graphite pass, and that's why the
> > > +     flag_graphite is tested here.  */
> > > +  if (current_loops && flag_graphite)
> > 
> > But we already analyzed that copyprop is _not_ the problem.  The
> > problem is the pass that introduces the two-arg loop-closed PHI
> > node.
> > 
> > Anyway, the condition should be current_loops && loops_state_satisfies_p 
> > (LOOP_CLOSED_SSA).  And the checking is definitely not useful.
> > 
> 
> So are you saying that...
> 
> Index: gcc/tree-ssa-copy.c
> ===================================================================
> --- gcc/tree-ssa-copy.c	(revision 157746)
> +++ gcc/tree-ssa-copy.c	(working copy)
> @@ -968,6 +968,18 @@
>    init_copy_prop ();
>    ssa_propagate (copy_prop_visit_stmt, copy_prop_visit_phi_node);
>    fini_copy_prop ();
> +
> + /* FIXME: The call to rewrite_into_loop_closed_ssa should be
> +    removed: this code is a temporary work around needed to maintain
> +    the loop closed SSA form when copy propagation is called after
> +    loop unswitching pass.  The second call to copy propagation is
> +    done as a sub-pass of the graphite pass, and that's why the
> +    flag_graphite is tested here.  */
> + if (current_loops && loops_state_satisfies_p(LOOP_CLOSED_SSA) && flag_graphite)
> +   {
> +     rewrite_into_loop_closed_ssa (NULL, TODO_update_ssa);
> +   }
> +
>    return 0;
>  }
>  
> Index: gcc/passes.c
> ===================================================================
> --- gcc/passes.c	(revision 157746)
> +++ gcc/passes.c	(working copy)
> @@ -897,6 +897,7 @@
>  	  NEXT_PASS (pass_graphite_transforms);
>  	    {
>  	      struct opt_pass **p = &pass_graphite_transforms.pass.sub;
> +	      NEXT_PASS (pass_copy_prop);
>  	      NEXT_PASS (pass_dce_loop);
>  	      NEXT_PASS (pass_lim);
>  	    }
> 
> would be okay?

No.

Richard.

>            Jack
> 
> > The patch is not ok.
> > 
> > Thanks,
> > Richard.
> > 
> > > +    {
> > > +      rewrite_into_loop_closed_ssa (NULL, TODO_update_ssa);
> > > +#ifdef ENABLE_CHECKING
> > > +      verify_loop_closed_ssa ();
> > > +#endif
> > > +    }
> > > +
> > >    return 0;
> > >  }
> > > 
> > > --------------------------------- gcc/passes.c ---------------------------------
> > > index 9d0c55a..1ac8694 100644
> > > @@ -897,6 +897,7 @@ init_optimization_passes (void)
> > >  	  NEXT_PASS (pass_graphite_transforms);
> > >  	    {
> > >  	      struct opt_pass **p = &pass_graphite_transforms.pass.sub;
> > > +	      NEXT_PASS (pass_copy_prop);
> > >  	      NEXT_PASS (pass_dce_loop);
> > >  	      NEXT_PASS (pass_lim);
> > >  	    }
> > > 
> > > 
> > 
> > -- 
> > Richard Guenther <rguenther@suse.de>
> > Novell / SUSE Labs
> > SUSE LINUX Products GmbH - Nuernberg - AG Nuernberg - HRB 16746 - GF: Markus Rex
> 
> 

-- 
Richard Guenther <rguenther@suse.de>
Novell / SUSE Labs
SUSE LINUX Products GmbH - Nuernberg - AG Nuernberg - HRB 16746 - GF: Markus Rex