On Wed, 19 Feb 2014, Bin.Cheng wrote:
> Hi Richard,
> Does this have to wait for stage 1? Or I will try to work out a full
> patch with loop recreating issue fixed.
If it is a regression and there is a bugzilla about it it doesn't
have to wait.
The patch should be complete (but is untested yet)
> On Wed, Feb 19, 2014 at 7:57 PM, Richard Biener <rguent...@suse.de> wrote:
> >
> > This allows cfgcleanup to remove some of the extra CFG that exists
> > just for loop analysis passes convenience (those can be and are
> > easily re-created by passes doing loop_optimizer_init ()).
> >
> > It may fix a regression uncovered in private communication.
> It's the regression about the code size checks in
> gcc.target/arm/ivopts.c and gcc.target/arm/ivopts-2.c
I see. So it is a regression then.
> >
> > Untested - my original idea how to fix this (tree_forwarder_block_p
> > hunk) ran into the issue in remove_forwarder_block which causes
> > loops to be removed / re-discovered (losing meta-data).
> >
> > Richard.
> >
> > 2014-02-19 Richard Biener <rguent...@suse.de>
> >
> > * tree-cfgcleanup.c (tree_forwarder_block_p): Protect
> > latches and preheaders only if requested.
> > (remove_forwarder_block): Update loop structure if we
> > removed a forwarder that is a loop latch.
> >
> > Index: gcc/tree-cfgcleanup.c
> > ===================================================================
> > *** gcc/tree-cfgcleanup.c (revision 207878)
> > --- gcc/tree-cfgcleanup.c (working copy)
> > *************** tree_forwarder_block_p (basic_block bb,
> > *** 307,321 ****
> >
> > if (current_loops)
> > {
> > ! basic_block dest;
> > ! /* Protect loop latches, headers and preheaders. */
> > if (bb->loop_father->header == bb)
> > return false;
> > - dest = EDGE_SUCC (bb, 0)->dest;
> >
> > ! if (dest->loop_father->header == dest)
> > return false;
> > }
> > return true;
> > }
> >
> > --- 307,324 ----
> >
> > if (current_loops)
> > {
> > ! /* Protect loop headers. */
> > if (bb->loop_father->header == bb)
> > return false;
> >
> > ! /* Protect loop latches and preheaders if requested. */
> > ! basic_block dest = EDGE_SUCC (bb, 0)->dest;
> > ! if (dest->loop_father->header == dest
> > ! && (loops_state_satisfies_p (LOOPS_HAVE_PREHEADERS)
> > ! || loops_state_satisfies_p (LOOPS_HAVE_SIMPLE_LATCHES)))
> > return false;
> > }
> Sorry for being nit-picking here. There is a scenario that bb is
> pre-header and loop prop is set to (!LOOPS_HAVE_PREHEADERS &&
> LOOPS_HAVE_SIMPLE_LATCHES). Of course, this may not happen anyway.
Yeah, but then we'd have to detect whether this is a preheader
forwarder or a latch forwarder. I doubt it happens right now
so I thought it doesn't matter to do it like above.
> > +
> > return true;
> > }
> >
> > *************** remove_forwarder_block (basic_block bb)
> > *** 497,503 ****
> > set_immediate_dominator (CDI_DOMINATORS, dest, dom);
> > }
> >
> > ! /* And kill the forwarder block. */
> > delete_basic_block (bb);
> >
> > return true;
> > --- 500,511 ----
> > set_immediate_dominator (CDI_DOMINATORS, dest, dom);
> > }
> >
> > ! /* And kill the forwarder block, but first adjust its parent loop
> > ! latch info as otherwise the cfg hook has a hard time not to
> > ! kill the loop. */
> > ! if (current_loops
> > ! && bb->loop_father->latch == bb)
> > ! bb->loop_father->latch = dest;
> > delete_basic_block (bb);
> By this code, do you mean to prevent cfgcleanup from
> removing/rebuilding the loop struct?
Yes. It prevents triggering cfghooks.c:
void
delete_basic_block (basic_block bb)
{
...
/* If we remove the header or the latch of a loop, mark the loop for
removal by setting its header and latch to NULL. */
if (loop->latch == bb
|| loop->header == bb)
{
loop->header = NULL;
loop->latch = NULL;
loops_state_set (LOOPS_NEED_FIXUP);
generally delete_basic_block has too little context to work out
anything more specific than the above (so it's a very bad tool ;))
Richard.
> >
> > return true;
>
> Thanks,
> bin
>
