This is the mail archive of the
gcc-patches@gcc.gnu.org
mailing list for the GCC project.
Re: [patch] Fix PR43464: update loop closed SSA form once copy prop is done
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