Allow cfgcleanup to remove forwarder loop preheaders and latches

On Mon, Feb 24, 2014 at 9:12 PM, 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?
>
>
> 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.
>

This change:

        if (dest->loop_father->header == dest)
-  return false;
+  {
+    if (loops_state_satisfies_p (LOOPS_HAVE_PREHEADERS)
+        && bb->loop_father->header != dest)
+      return false;
+
+    if (loops_state_satisfies_p (LOOPS_HAVE_SIMPLE_LATCHES)
+        && bb->loop_father->header == dest)
+      return false;
+  }
     }

miscompiled 435.gromacs in SPEC CPU 2006 on x32 with

-O3 -funroll-loops -ffast-math -fwhole-program -flto=jobserver
-fuse-linker-plugin

This patch changes loops without LOOPS_HAVE_PREHEADERS
nor LOOPS_HAVE_SIMPLE_LATCHES from returning false to returning
true.  I don't have a small testcase.  But this patch:

fixes the regression.  Does it make any senses?

Thanks for reporting this, I will look into it.

Thanks,
bin

On Fri, Feb 28, 2014 at 8:52 AM, H.J. Lu <hjl.tools@gmail.com> wrote:
> On Mon, Feb 24, 2014 at 9:12 PM, 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?
>>
>>
>> 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.
>>
>
> This change:
>
>         if (dest->loop_father->header == dest)
> -  return false;
> +  {
> +    if (loops_state_satisfies_p (LOOPS_HAVE_PREHEADERS)
> +        && bb->loop_father->header != dest)
> +      return false;
> +
> +    if (loops_state_satisfies_p (LOOPS_HAVE_SIMPLE_LATCHES)
> +        && bb->loop_father->header == dest)
> +      return false;
> +  }
>      }
>
> miscompiled 435.gromacs in SPEC CPU 2006 on x32 with
>
> -O3 -funroll-loops -ffast-math -fwhole-program -flto=jobserver
> -fuse-linker-plugin
>
> This patch changes loops without LOOPS_HAVE_PREHEADERS
> nor LOOPS_HAVE_SIMPLE_LATCHES from returning false to returning
> true.  I don't have a small testcase.  But this patch:
>
> diff --git a/gcc/tree-cfgcleanup.c b/gcc/tree-cfgcleanup.c
> index b5c384b..2ba673c 100644
> --- a/gcc/tree-cfgcleanup.c
> +++ b/gcc/tree-cfgcleanup.c
> @@ -323,6 +323,10 @@ tree_forwarder_block_p (basic_block bb, bool phi_wanted)
>      if (loops_state_satisfies_p (LOOPS_HAVE_SIMPLE_LATCHES)
>          && bb->loop_father->header == dest)
>        return false;
> +
> +    if (!loops_state_satisfies_p (LOOPS_HAVE_PREHEADERS)
> +        && !loops_state_satisfies_p (LOOPS_HAVE_SIMPLE_LATCHES))
> +      return false;
>    }
>      }
>
> fixes the regression.  Does it make any senses?
>
>
> --
> H.J.