On Fri, Feb 28, 2014 at 9:25 AM, H.J. Lu <hjl.to...@gmail.com> wrote: > On Fri, Feb 28, 2014 at 8:11 AM, H.J. Lu <hjl.to...@gmail.com> wrote: >> On Fri, Feb 28, 2014 at 2:09 AM, Richard Biener >> <richard.guent...@gmail.com> wrote: >>> 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 tried revision 208222 and it doesn't fix 435.gromacs. > > Remove > > else if (bb->loop_father == loop_outer (dest->loop_father)) > return !loops_state_satisfies_p (LOOPS_HAVE_PREHEADERS);
Should we also check other loop state, like LOOPS_HAVE_SIMPLE_LATCHES or LOOPS_MAY_HAVE_MULTIPLE_LATCHES here? > fixes 435.gromacs. -- H.J.