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. Thanks, bin > > Richard. > -- Best Regards.