On Wed, 19 Feb 2014, Bin.Cheng wrote:
> On Wed, Feb 19, 2014 at 10:06 PM, Richard Biener <rguent...@suse.de> wrote:
> > 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.
> OK, I will file a PR about it.
>
> >
> >> >
> >> > 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 ;))
>
> Well, it can't. From what I observed, it is pass_ch that modifies the
> loop header with some specific control flow graph. Doesn't matter if
> the LOOPS_NEED_FIXUP is set before pass_ch, since it calls
> loop_optimizer_init to fix loop structure before starting work. In
> another word, pass_ch always starts with LOOPS_NEED_FIXUP cleared.
>
> I will do further investigation on pass_ch.
Ok. With a latent bug in replace_uses_by fixed the patch passed
bootstrap & regtest on x86_64-unknown-linux-gnu for me with
the following regressions:
FAIL: gcc.dg/tree-prof/crossmodule-indircall-1.c execution,
-fprofile-use -D_
PROFILE_USE
FAIL: gcc.dg/tree-prof/update-loopch.c scan-tree-dump-not optimized
"Invalid sum
"
FAIL: gcc.dg/tree-ssa/pr21417.c scan-tree-dump-times dom2 "Threaded jump"
1
FAIL: gcc.dg/tree-ssa/pr21559.c scan-tree-dump-times vrp1 "Threaded jump"
2
FAIL: gnat.dg/renaming5.adb scan-tree-dump-times optimized "goto" 3
you've already fixed crossmodule-indircall-1.c and pr21417.c is due
to (several) overly conservative bail-outs in jump-threading
(can be fixed by computing loops with pre-headers in DOM or by
fixing those bail-outs).
Richard.
