On Fri, Feb 28, 2014 at 10:09 AM, Richard Biener
<richard.guent...@gmail.com> wrote:
> 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.
I think the patch makes sense anyway and thus I'll install it once it
passed bootstrap / regtesting.
Another fix that may make sense is to restrict it to
!loops_state_satisfies_p (LOOPS_NEED_FIXUP), though cfgcleanup
itself can end up setting that ... which we eventually should fix if it
still happens. That is, check if
Index: gcc/tree-cfgcleanup.c
===================================================================
--- gcc/tree-cfgcleanup.c (revision 208169)
+++ gcc/tree-cfgcleanup.c (working copy)
@@ -729,8 +729,9 @@ cleanup_tree_cfg_noloop (void)
timevar_pop (TV_TREE_CLEANUP_CFG);
- if (changed && current_loops)
- loops_state_set (LOOPS_NEED_FIXUP);
+ if (changed && current_loops
+ && !loops_state_satisfies_p (LOOPS_NEED_FIXUP))
+ verify_loop_structure ();
return changed;
}
trips anywhere (and apply fixes). That's of course not appropriate at
this stage.
> Does it fix 435.gromacs?
I can't see the failure on our testers (x86_64, i?86, with/without LTO). How
can I reproduce it?
Thanks,
Richard.
