Re: [PATCH GCC]Allow cfgcleanup to remove forwarder loop preheaders and latches

2014-03-01 Thread Bin.Cheng 
Hi H.J,
Sorry that I will be out of office next week, and don't have chance to
reproduce it until back.  BTW, does x32 refer to x86 32 bit?

Thanks,
bin

On Sat, Mar 1, 2014 at 2:23 AM, H.J. Lu hjl.to...@gmail.com wrote:
 On Fri, Feb 28, 2014 at 9:42 AM, H.J. Lu hjl.to...@gmail.com wrote:
 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

Re: [PATCH GCC]Allow cfgcleanup to remove forwarder loop preheaders and latches

2014-02-28 Thread Richard Biener 
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.

Re: [PATCH GCC]Allow cfgcleanup to remove forwarder loop preheaders and latches

2014-02-28 Thread Richard Biener 
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

Re: [PATCH GCC]Allow cfgcleanup to remove forwarder loop preheaders and latches

2014-02-28 Thread H.J. Lu 
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

Re: [PATCH GCC]Allow cfgcleanup to remove forwarder loop preheaders and latches

2014-02-28 Thread H.J. Lu 
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

Re: [PATCH GCC]Allow cfgcleanup to remove forwarder loop preheaders and latches

2014-02-28 Thread H.J. Lu 
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

Re: [PATCH GCC]Allow cfgcleanup to remove forwarder loop preheaders and latches

2014-02-28 Thread H.J. Lu 
On Fri, Feb 28, 2014 at 9:42 AM, H.J. Lu hjl.to...@gmail.com wrote:
 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

Re: [PATCH GCC]Allow cfgcleanup to remove forwarder loop preheaders and latches

2014-02-27 Thread H.J. Lu 
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?


-- 
H.J.

Re: [PATCH GCC]Allow cfgcleanup to remove forwarder loop preheaders and latches

2014-02-27 Thread Hans-Peter Nilsson 
On Tue, 25 Feb 2014, bin.cheng 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.

 gcc/testsuite/ChangeLog
 2014-02-25  Bin Cheng  bin.ch...@arm.com

   PR target/60280
   * gnat.dg/renaming5.adb: Change to two expected gotos.
   * gcc.dg/tree-ssa/pr21559.c: Change back to three expected
   jump threads.
   * gcc.dg/tree-prof/update-loopch.c: Check two Invalid sum
   messages for removed basic block.
   * gcc.dg/tree-ssa/ivopt_1.c: Fix unreliable scanning string.
   * gcc.dg/tree-ssa/ivopt_2.c: Ditto.
   * gcc.dg/tree-ssa/ivopt_3.c: Ditto.
   * gcc.dg/tree-ssa/ivopt_4.c: Ditto.

Do you need to also update gcc.dg/tree-ssa/ssa-dom-thread-4.c,
at least for logical_op_short_circuit targets?
(There's a nice analysis comment there by Richard S which may
also have to be updated.)

This caused a regression for logical_op_short_circuit targets, I
entered PR60363 for convenience.

brgds, H-P

Re: [PATCH GCC]Allow cfgcleanup to remove forwarder loop preheaders and latches

2014-02-27 Thread Bin.Cheng 
Thanks for reporting this, I will look into it.

Thanks,
bin

On Fri, Feb 28, 2014 at 8: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?


 --
 H.J.



-- 
Best Regards.

Re: [PATCH GCC]Allow cfgcleanup to remove forwarder loop preheaders and latches

2014-02-27 Thread Bin.Cheng 
Sorry, I didn't test it against logical_op_short_circuit target.  I
will look into this PR.

Thanks,
bin

On Fri, Feb 28, 2014 at 9:34 AM, Hans-Peter Nilsson h...@bitrange.com wrote:
 On Tue, 25 Feb 2014, bin.cheng 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.

 gcc/testsuite/ChangeLog
 2014-02-25  Bin Cheng  bin.ch...@arm.com

   PR target/60280
   * gnat.dg/renaming5.adb: Change to two expected gotos.
   * gcc.dg/tree-ssa/pr21559.c: Change back to three expected
   jump threads.
   * gcc.dg/tree-prof/update-loopch.c: Check two Invalid sum
   messages for removed basic block.
   * gcc.dg/tree-ssa/ivopt_1.c: Fix unreliable scanning string.
   * gcc.dg/tree-ssa/ivopt_2.c: Ditto.
   * gcc.dg/tree-ssa/ivopt_3.c: Ditto.
   * gcc.dg/tree-ssa/ivopt_4.c: Ditto.

 Do you need to also update gcc.dg/tree-ssa/ssa-dom-thread-4.c,
 at least for logical_op_short_circuit targets?
 (There's a nice analysis comment there by Richard S which may
 also have to be updated.)

 This caused a regression for logical_op_short_circuit targets, I
 entered PR60363 for convenience.

 brgds, H-P



-- 
Best Regards.

Re: [PATCH GCC]Allow cfgcleanup to remove forwarder loop preheaders and latches

2014-02-25 Thread Richard Biener 
On Tue, Feb 25, 2014 at 6:12 AM, 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?

Can you document the extra threading we do in pr21559.c?  The
comment still talks about two threadings we should perform.

Also the ivopt_* adjustmens would be better done by matching
ivtmp.[0-9_]* = PHI instead of matching ivtmp in one of the PHI
arguments.

@@ -497,6 +507,9 @@ remove_forwarder_block (basic_block bb)
   set_immediate_dominator (CDI_DOMINATORS, dest, dom);
 }

+  if (current_loops  bb-loop_father-latch == bb)
+bb-loop_father-latch = dest;
+
   /* And kill the forwarder block.  */
   delete_basic_block (bb);

can you add a comment here?  I had

@@ -497,7 +500,12 @@ remove_forwarder_block (basic_block bb)
   set_immediate_dominator (CDI_DOMINATORS, dest, dom);
 }

-  /* And kill the forwarder block.  */
+  /* And kill the forwarder block, but first adjust its parent loop
+ latch info as otherwise the cfg hook has a hard time not to
+ kill the loop.  */
+  if (current_loops
+   bb-loop_father-latch == bb)
+bb-loop_father-latch = dest;
   delete_basic_block (bb);

   return true;

in my patch.

Thanks,
Richard.


 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.

 gcc/testsuite/ChangeLog
 2014-02-25  Bin Cheng  bin.ch...@arm.com

 PR target/60280
 * gnat.dg/renaming5.adb: Change to two expected gotos.
 * gcc.dg/tree-ssa/pr21559.c: Change back to three expected
 jump threads.
 * gcc.dg/tree-prof/update-loopch.c: Check two Invalid sum
 messages for removed basic block.
 * gcc.dg/tree-ssa/ivopt_1.c: Fix unreliable scanning string.
 * gcc.dg/tree-ssa/ivopt_2.c: Ditto.
 * gcc.dg/tree-ssa/ivopt_3.c: Ditto.
 * gcc.dg/tree-ssa/ivopt_4.c: Ditto.

RE: [PATCH GCC]Allow cfgcleanup to remove forwarder loop preheaders and latches

2014-02-25 Thread bin.cheng 
Updated as comments.

Thanks,
bin

 -Original Message-
 From: Richard Biener [mailto:richard.guent...@gmail.com]
 Sent: Tuesday, February 25, 2014 6:38 PM
 To: Bin Cheng
 Cc: GCC Patches
 Subject: Re: [PATCH GCC]Allow cfgcleanup to remove forwarder loop
 preheaders and latches
 
 On Tue, Feb 25, 2014 at 6:12 AM, 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?
 
 Can you document the extra threading we do in pr21559.c?  The comment
 still talks about two threadings we should perform.
 
 Also the ivopt_* adjustmens would be better done by matching
 ivtmp.[0-9_]* = PHI instead of matching ivtmp in one of the PHI
arguments.
 
 @@ -497,6 +507,9 @@ remove_forwarder_block (basic_block bb)
set_immediate_dominator (CDI_DOMINATORS, dest, dom);
  }
 
 +  if (current_loops  bb-loop_father-latch == bb)
 +bb-loop_father-latch = dest;
 +
/* And kill the forwarder block.  */
delete_basic_block (bb);
 
 can you add a comment here?  I had
 
 @@ -497,7 +500,12 @@ remove_forwarder_block (basic_block bb)
set_immediate_dominator (CDI_DOMINATORS, dest, dom);
  }
 
 -  /* And kill the forwarder block.  */
 +  /* And kill the forwarder block, but first adjust its parent loop
 + latch info as otherwise the cfg hook has a hard time not to
 + kill the loop.  */
 +  if (current_loops
 +   bb-loop_father-latch == bb)
 +bb-loop_father-latch = dest;
delete_basic_block (bb);
 
return true;
 
 in my patch.
 
 Thanks,
 Richard.
 
 
  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.
 
  gcc/testsuite/ChangeLog
  2014-02-25  Bin Cheng  bin.ch...@arm.com
 
  PR target/60280
  * gnat.dg/renaming5.adb: Change to two expected gotos.
  * gcc.dg/tree-ssa/pr21559.c: Change back to three expected
  jump threads.
  * gcc.dg/tree-prof/update-loopch.c: Check two Invalid sum
  messages for removed basic block.
  * gcc.dg/tree-ssa/ivopt_1.c: Fix unreliable scanning string.
  * gcc.dg/tree-ssa/ivopt_2.c: Ditto.
  * gcc.dg/tree-ssa/ivopt_3.c: Ditto.
  * gcc.dg/tree-ssa/ivopt_4.c: Ditto.
Index: gcc/tree-cfgcleanup.c
===
--- gcc/tree-cfgcleanup.c   (revision 207938)
+++ gcc/tree-cfgcleanup.c   (working copy)
@@ -308,14 +308,24 @@ tree_forwarder_block_p (basic_block bb, bool phi_w
   if (current_loops)
 {
   basic_block dest;
-  /* Protect loop latches, headers and preheaders.  */
+  /* Protect loop headers.  */
   if (bb-loop_father-header == bb)
return false;
+
   dest = EDGE_SUCC (bb, 0)-dest;
+  /* 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 (dest-loop_father-header == dest)
-   return false;
+ if (loops_state_satisfies_p (LOOPS_HAVE_SIMPLE_LATCHES)
+  bb-loop_father-header == dest)
+   return false;
+   }
 }
+
   return true;
 }
 
@@ -497,6 +507,11 @@ remove_forwarder_block (basic_block bb)
   set_immediate_dominator (CDI_DOMINATORS, dest, dom);
 }
 
+  /* Adjust latch infomation of BB's parent loop as otherwise
+ the cfg hook has a hard time not to kill the loop.  */
+  if (current_loops  bb-loop_father-latch == bb)
+bb-loop_father-latch = dest;
+
   /* And kill the forwarder block.  */
   delete_basic_block (bb);
 
Index: gcc/tree-ssa-dom.c
===
--- gcc/tree-ssa-dom.c  (revision 207938)
+++ gcc/tree-ssa-dom.c  (working copy)
@@ -849,9 +849,15 @@ tree_ssa_dominator_optimize (void)
   /* We need to know loop structures in order to avoid destroying them
  in jump threading.  Note

Re: [PATCH GCC]Allow cfgcleanup to remove forwarder loop preheaders and latches

2014-02-25 Thread Richard Biener 
On Tue, Feb 25, 2014 at 12:20 PM, bin.cheng bin.ch...@arm.com wrote:
 Updated as comments.

Ok.

Thanks,
Richard.

 Thanks,
 bin

 -Original Message-
 From: Richard Biener [mailto:richard.guent...@gmail.com]
 Sent: Tuesday, February 25, 2014 6:38 PM
 To: Bin Cheng
 Cc: GCC Patches
 Subject: Re: [PATCH GCC]Allow cfgcleanup to remove forwarder loop
 preheaders and latches

 On Tue, Feb 25, 2014 at 6:12 AM, 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?

 Can you document the extra threading we do in pr21559.c?  The comment
 still talks about two threadings we should perform.

 Also the ivopt_* adjustmens would be better done by matching
 ivtmp.[0-9_]* = PHI instead of matching ivtmp in one of the PHI
 arguments.

 @@ -497,6 +507,9 @@ remove_forwarder_block (basic_block bb)
set_immediate_dominator (CDI_DOMINATORS, dest, dom);
  }

 +  if (current_loops  bb-loop_father-latch == bb)
 +bb-loop_father-latch = dest;
 +
/* And kill the forwarder block.  */
delete_basic_block (bb);

 can you add a comment here?  I had

 @@ -497,7 +500,12 @@ remove_forwarder_block (basic_block bb)
set_immediate_dominator (CDI_DOMINATORS, dest, dom);
  }

 -  /* And kill the forwarder block.  */
 +  /* And kill the forwarder block, but first adjust its parent loop
 + latch info as otherwise the cfg hook has a hard time not to
 + kill the loop.  */
 +  if (current_loops
 +   bb-loop_father-latch == bb)
 +bb-loop_father-latch = dest;
delete_basic_block (bb);

return true;

 in my patch.

 Thanks,
 Richard.

 
  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.
 
  gcc/testsuite/ChangeLog
  2014-02-25  Bin Cheng  bin.ch...@arm.com
 
  PR target/60280
  * gnat.dg/renaming5.adb: Change to two expected gotos.
  * gcc.dg/tree-ssa/pr21559.c: Change back to three expected
  jump threads.
  * gcc.dg/tree-prof/update-loopch.c: Check two Invalid sum
  messages for removed basic block.
  * gcc.dg/tree-ssa/ivopt_1.c: Fix unreliable scanning string.
  * gcc.dg/tree-ssa/ivopt_2.c: Ditto.
  * gcc.dg/tree-ssa/ivopt_3.c: Ditto.
  * gcc.dg/tree-ssa/ivopt_4.c: Ditto.