On Fri, Feb 28, 2014 at 1:52 AM, H.J. Lu <hjl.to...@gmail.com> wrote:
> On Mon, Feb 24, 2014 at 9:12 PM, bin.cheng <bin.ch...@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.ch...@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?
I think the preheader test isn't fully correct (bb may be in an inner loop
for example). So a more conservative variant would be
Index: gcc/tree-cfgcleanup.c
===================================================================
--- gcc/tree-cfgcleanup.c (revision 208169)
+++ gcc/tree-cfgcleanup.c (working copy)
@@ -316,13 +316,13 @@ tree_forwarder_block_p (basic_block bb,
/* Protect loop preheaders and latches if requested. */
if (dest->loop_father->header == dest)
{
- 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;
+ if (bb->loop_father == dest->loop_father)
+ return !loops_state_satisfies_p (LOOPS_HAVE_SIMPLE_LATCHES);
+ else if (bb->loop_father == loop_outer (dest->loop_father))
+ return !loops_state_satisfies_p (LOOPS_HAVE_PREHEADERS);
+ /* Always preserve other edges into loop headers that are
+ not simple latches or preheaders. */
+ return false;
}
}
that makes sure we can properly update loop information. It's also
a more conservative change at this point which should still successfully
remove simple latches and preheaders created by loop discovery.
Does it fix 435.gromacs?
Thanks,
Richard.
>
> --
> H.J.
