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.