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

["bin.cheng" <bin dot cheng at arm dot com>]

Updated as comments.

Thanks,
bin

> -----Original Message-----
> From: Richard Biener [mailto:richard.guenther@gmail.com]
> Sent: Tuesday, February 25, 2014 6:38 PM
> To: Bin Cheng
> Cc: GCC Patches
> Subject: Re: [PATCH GCC]Allow cfgcleanup to remove forwarder loop
> preheaders and latches
> 
> On Tue, Feb 25, 2014 at 6:12 AM, bin.cheng <bin.cheng@arm.com> wrote:
> > Hi,
> > This patch is to fix regression reported in PR60280 by removing
> > forward loop headers/latches in cfg cleanup if possible.  Several
> > tests are broken by this change since cfg cleanup is shared by all
> > optimizers.  Some tests has already been fixed by recent patches, I went
> through and fixed the others.
> > One case needs to be clarified is "gcc.dg/tree-prof/update-loopch.c".
> > When GCC removing a basic block, it checks profile information by
> > calling check_bb_profile after redirecting incoming edges of the bb.
> > This certainly results in warnings about invalid profile information
> > and causes the case to fail.  I will send a patch to skip checking
> > profile information for a removing basic block in stage 1 if it sounds
> > reasonable.  For now I just twisted the case itself.
> >
> > Bootstrap and tested on x86_64 and arm_a15.
> >
> > Is it OK?
> 
> Can you document the extra threading we do in pr21559.c?  The comment
> still talks about two threadings we should perform.
> 
> Also the ivopt_* adjustmens would be better done by matching
> "ivtmp.[0-9_]* = PHI" instead of matching ivtmp in one of the PHI
arguments.
> 
> @@ -497,6 +507,9 @@ remove_forwarder_block (basic_block bb)
>        set_immediate_dominator (CDI_DOMINATORS, dest, dom);
>      }
> 
> +  if (current_loops && bb->loop_father->latch == bb)
> +    bb->loop_father->latch = dest;
> +
>    /* And kill the forwarder block.  */
>    delete_basic_block (bb);
> 
> can you add a comment here?  I had
> 
> @@ -497,7 +500,12 @@ remove_forwarder_block (basic_block bb)
>        set_immediate_dominator (CDI_DOMINATORS, dest, dom);
>      }
> 
> -  /* And kill the forwarder block.  */
> +  /* 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);
> 
>    return true;
> 
> in my patch.
> 
> Thanks,
> Richard.
> 
> >
> > 2014-02-25  Bin Cheng  <bin.cheng@arm.com>
> >
> >         PR target/60280
> >         * tree-cfgcleanup.c (tree_forwarder_block_p): Protect loop
> >         preheaders and latches only if requested.  Fix latch if it
> >         is removed.
> >         * tree-ssa-dom.c (tree_ssa_dominator_optimize): Set
> >         LOOPS_HAVE_PREHEADERS.
> >
> > gcc/testsuite/ChangeLog
> > 2014-02-25  Bin Cheng  <bin.cheng@arm.com>
> >
> >         PR target/60280
> >         * gnat.dg/renaming5.adb: Change to two expected gotos.
> >         * gcc.dg/tree-ssa/pr21559.c: Change back to three expected
> >         jump threads.
> >         * gcc.dg/tree-prof/update-loopch.c: Check two "Invalid sum"
> >         messages for removed basic block.
> >         * gcc.dg/tree-ssa/ivopt_1.c: Fix unreliable scanning string.
> >         * gcc.dg/tree-ssa/ivopt_2.c: Ditto.
> >         * gcc.dg/tree-ssa/ivopt_3.c: Ditto.
> >         * gcc.dg/tree-ssa/ivopt_4.c: Ditto.

Attachment: remove-latch_preheader-20140225.txt
Description: Text document