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

[Richard Biener <richard dot guenther at gmail dot com>]

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.