Re: [PATCH] Allow cfgcleanup to remove forwarder loop preheaders and latches

["Bin.Cheng" <amker dot cheng at gmail dot com>]

On Wed, Feb 19, 2014 at 10:06 PM, Richard Biener <rguenther@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 <rguenther@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  <rguenther@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.