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, Mar 26, 2010 at 12:42:55PM +0100, Richard Guenther wrote:
> 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.
Was another alternative short-term fix discussed earlier that could be
conditional on flag_graphite? PR43464 should be updated to reflect the
consensus on what really needs to be done here.
Jack
>
> > 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