sel-sched: run cleanup_cfg just before loop_optimizer_init (PR 84659)

Message ID alpine.LNX.2.20.13.1804101726300.24851@monopod.intra.ispras.ru
State New
Headers show
Series
  • sel-sched: run cleanup_cfg just before loop_optimizer_init (PR 84659)
Related show

Commit Message

Alexander Monakov April 10, 2018, 2:40 p.m.
Hi,

We have this code in sel-sched.c sel_region_init():

6918   /* Init correct liveness sets on each instruction of a single-block loop.
6919      This is the only situation when we can't update liveness when calling
6920      compute_live for the first insn of the loop.  */
6921   if (current_loop_nest)
6922     {
6923       int header =
6924         (sel_is_loop_preheader_p (BASIC_BLOCK_FOR_FN (cfun, BB_TO_BLOCK (0)))
6925          ? 1
6926          : 0);
6927
6928       if (current_nr_blocks == header + 1)
6929         update_liveness_on_insn
6930           (sel_bb_head (BASIC_BLOCK_FOR_FN (cfun, BB_TO_BLOCK (header))));
6931     }

It appears it does not account for presence of empty BBs between the preheader
and the "actual header" BB hosting the first insn of the loop. The testcase in
the PR provides an example where we don't run update_liveness_on_insn here, but
then remove the empty BB and recurse endlessly in compute_live_after_bb.

This patch solves this in a brute-but-straightforward manner by invoking
cleanup_cfg just before loop_optimizer_init.

Bootstrapped/regtested on x86_64 together with the other two patches and also
checked on a powerpc cross-compiler that the testcase is fixed. OK to apply?

Thanks.
Alexander

	PR rtl-optimization/85659
	* sel-sched-ir.c (sel_init_pipelining): Invoke cleanup_cfg.

	* gcc.dg/pr84659.c: New test.

Comments

Andrey Belevantsev April 10, 2018, 3:38 p.m. | #1
Hello,

On 10.04.2018 17:40, Alexander Monakov wrote:
> Hi,
> 
> We have this code in sel-sched.c sel_region_init():
> 
> 6918   /* Init correct liveness sets on each instruction of a single-block loop.
> 6919      This is the only situation when we can't update liveness when calling
> 6920      compute_live for the first insn of the loop.  */
> 6921   if (current_loop_nest)
> 6922     {
> 6923       int header =
> 6924         (sel_is_loop_preheader_p (BASIC_BLOCK_FOR_FN (cfun, BB_TO_BLOCK (0)))
> 6925          ? 1
> 6926          : 0);
> 6927
> 6928       if (current_nr_blocks == header + 1)
> 6929         update_liveness_on_insn
> 6930           (sel_bb_head (BASIC_BLOCK_FOR_FN (cfun, BB_TO_BLOCK (header))));
> 6931     }
> 
> It appears it does not account for presence of empty BBs between the preheader
> and the "actual header" BB hosting the first insn of the loop. The testcase in
> the PR provides an example where we don't run update_liveness_on_insn here, but
> then remove the empty BB and recurse endlessly in compute_live_after_bb.
> 
> This patch solves this in a brute-but-straightforward manner by invoking
> cleanup_cfg just before loop_optimizer_init.
> 
> Bootstrapped/regtested on x86_64 together with the other two patches and also
> checked on a powerpc cross-compiler that the testcase is fixed. OK to apply?

OK.  The comment before cleanup_cfg will not hurt but do as you wish.

Thank you,
Andrey

> 
> Thanks.
> Alexander
> 
> 	PR rtl-optimization/85659
> 	* sel-sched-ir.c (sel_init_pipelining): Invoke cleanup_cfg.
> 
> 	* gcc.dg/pr84659.c: New test.
> 
> 
> diff --git a/gcc/sel-sched-ir.c b/gcc/sel-sched-ir.c
> index ee970522890..aef380a7e80 100644
> --- a/gcc/sel-sched-ir.c
> +++ b/gcc/sel-sched-ir.c
> @@ -30,6 +30,7 @@ along with GCC; see the file COPYING3.  If not see
>  #include "cfgrtl.h"
>  #include "cfganal.h"
>  #include "cfgbuild.h"
> +#include "cfgcleanup.h"
>  #include "insn-config.h"
>  #include "insn-attr.h"
>  #include "recog.h"
> @@ -6121,6 +6122,7 @@ make_regions_from_loop_nest (struct loop *loop)
>  void
>  sel_init_pipelining (void)
>  {
> +  cleanup_cfg (0);
>    /* Collect loop information to be used in outer loops pipelining.  */
>    loop_optimizer_init (LOOPS_HAVE_PREHEADERS
>                         | LOOPS_HAVE_FALLTHRU_PREHEADERS
> diff --git a/gcc/testsuite/gcc.dg/pr84659.c b/gcc/testsuite/gcc.dg/pr84659.c
> index e69de29bb2d..94c885f3869 100644
> --- a/gcc/testsuite/gcc.dg/pr84659.c
> +++ b/gcc/testsuite/gcc.dg/pr84659.c
> @@ -0,0 +1,19 @@
> +/* PR rtl-optimization/84659 */
> +/* { dg-do compile { target powerpc*-*-* ia64-*-* i?86-*-* x86_64-*-* } } */
> +/* { dg-options "-O2 -fselective-scheduling -fsel-sched-pipelining -fno-split-wide-types -fno-strict-aliasing -fno-tree-dce" } */
> +
> +void
> +jk (int **lq, int *k4, long long int qv, int od)
> +{
> +  while (**lq < 1)
> +    {
> +      int uo;
> +
> +      uo = ((od == 0) ? qv : *k4) != 1;
> +      ++**lq;
> +    }
> +
> +  for (;;)
> +    {
> +    }
> +}
>
Alexander Monakov April 11, 2018, 9:55 p.m. | #2
As noted in PR 85354, we cannot simply invoke cfg_cleanup after dominators are
computed, because they may become invalid but neither freed nor recomputed, so
this trips checking in flow_loops_find.

We can move cleanup_cfg earlier (and run it for all sel-sched invocations, not
only when pipelining).

Bootstrapped/regtested on x86_64 and ppc64 (my previous testing missed this
issue: the testcase requires graphite, but libisl wasn't present).

	PR rtl-optimization/85354
	* sel-sched-ir.c (sel_init_pipelining): Move cfg_cleanup call...
	* sel-sched.c (sel_global_init): ... here.

diff --git a/gcc/sel-sched-ir.c b/gcc/sel-sched-ir.c
index 50a7daafba6..ee970522890 100644
--- a/gcc/sel-sched-ir.c
+++ b/gcc/sel-sched-ir.c
@@ -30,7 +30,6 @@ along with GCC; see the file COPYING3.  If not see
 #include "cfgrtl.h"
 #include "cfganal.h"
 #include "cfgbuild.h"
-#include "cfgcleanup.h"
 #include "insn-config.h"
 #include "insn-attr.h"
 #include "recog.h"
@@ -6122,9 +6121,6 @@ make_regions_from_loop_nest (struct loop *loop)
 void
 sel_init_pipelining (void)
 {
-  /* Remove empty blocks: their presence can break assumptions elsewhere,
-     e.g. the logic to invoke update_liveness_on_insn in sel_region_init.  */
-  cleanup_cfg (0);
   /* Collect loop information to be used in outer loops pipelining.  */
   loop_optimizer_init (LOOPS_HAVE_PREHEADERS
                        | LOOPS_HAVE_FALLTHRU_PREHEADERS
diff --git a/gcc/sel-sched.c b/gcc/sel-sched.c
index cd29df35666..59762964c6e 100644
--- a/gcc/sel-sched.c
+++ b/gcc/sel-sched.c
@@ -28,6 +28,7 @@ along with GCC; see the file COPYING3.  If not see
 #include "tm_p.h"
 #include "regs.h"
 #include "cfgbuild.h"
+#include "cfgcleanup.h"
 #include "insn-config.h"
 #include "insn-attr.h"
 #include "params.h"
@@ -7661,6 +7662,10 @@ sel_sched_region (int rgn)
 static void
 sel_global_init (void)
 {
+  /* Remove empty blocks: their presence can break assumptions elsewhere,
+     e.g. the logic to invoke update_liveness_on_insn in sel_region_init.  */
+  cleanup_cfg (0);
+
   calculate_dominance_info (CDI_DOMINATORS);
   alloc_sched_pools ();
Andrey Belevantsev April 12, 2018, 2:58 p.m. | #3
On 12.04.2018 0:55, Alexander Monakov wrote:
> As noted in PR 85354, we cannot simply invoke cfg_cleanup after dominators are
> computed, because they may become invalid but neither freed nor recomputed, so
> this trips checking in flow_loops_find.
> 
> We can move cleanup_cfg earlier (and run it for all sel-sched invocations, not
> only when pipelining).

OK.  Sorry, I should have noticed that before, and our ia64 tester also
misses libraries required for graphite.

Best,
Andrey

> 
> Bootstrapped/regtested on x86_64 and ppc64 (my previous testing missed this
> issue: the testcase requires graphite, but libisl wasn't present).
> 
> 	PR rtl-optimization/85354
> 	* sel-sched-ir.c (sel_init_pipelining): Move cfg_cleanup call...
> 	* sel-sched.c (sel_global_init): ... here.
> 
> diff --git a/gcc/sel-sched-ir.c b/gcc/sel-sched-ir.c
> index 50a7daafba6..ee970522890 100644
> --- a/gcc/sel-sched-ir.c
> +++ b/gcc/sel-sched-ir.c
> @@ -30,7 +30,6 @@ along with GCC; see the file COPYING3.  If not see
>  #include "cfgrtl.h"
>  #include "cfganal.h"
>  #include "cfgbuild.h"
> -#include "cfgcleanup.h"
>  #include "insn-config.h"
>  #include "insn-attr.h"
>  #include "recog.h"
> @@ -6122,9 +6121,6 @@ make_regions_from_loop_nest (struct loop *loop)
>  void
>  sel_init_pipelining (void)
>  {
> -  /* Remove empty blocks: their presence can break assumptions elsewhere,
> -     e.g. the logic to invoke update_liveness_on_insn in sel_region_init.  */
> -  cleanup_cfg (0);
>    /* Collect loop information to be used in outer loops pipelining.  */
>    loop_optimizer_init (LOOPS_HAVE_PREHEADERS
>                         | LOOPS_HAVE_FALLTHRU_PREHEADERS
> diff --git a/gcc/sel-sched.c b/gcc/sel-sched.c
> index cd29df35666..59762964c6e 100644
> --- a/gcc/sel-sched.c
> +++ b/gcc/sel-sched.c
> @@ -28,6 +28,7 @@ along with GCC; see the file COPYING3.  If not see
>  #include "tm_p.h"
>  #include "regs.h"
>  #include "cfgbuild.h"
> +#include "cfgcleanup.h"
>  #include "insn-config.h"
>  #include "insn-attr.h"
>  #include "params.h"
> @@ -7661,6 +7662,10 @@ sel_sched_region (int rgn)
>  static void
>  sel_global_init (void)
>  {
> +  /* Remove empty blocks: their presence can break assumptions elsewhere,
> +     e.g. the logic to invoke update_liveness_on_insn in sel_region_init.  */
> +  cleanup_cfg (0);
> +
>    calculate_dominance_info (CDI_DOMINATORS);
>    alloc_sched_pools ();
>

Patch

diff --git a/gcc/sel-sched-ir.c b/gcc/sel-sched-ir.c
index ee970522890..aef380a7e80 100644
--- a/gcc/sel-sched-ir.c
+++ b/gcc/sel-sched-ir.c
@@ -30,6 +30,7 @@  along with GCC; see the file COPYING3.  If not see
 #include "cfgrtl.h"
 #include "cfganal.h"
 #include "cfgbuild.h"
+#include "cfgcleanup.h"
 #include "insn-config.h"
 #include "insn-attr.h"
 #include "recog.h"
@@ -6121,6 +6122,7 @@  make_regions_from_loop_nest (struct loop *loop)
 void
 sel_init_pipelining (void)
 {
+  cleanup_cfg (0);
   /* Collect loop information to be used in outer loops pipelining.  */
   loop_optimizer_init (LOOPS_HAVE_PREHEADERS
                        | LOOPS_HAVE_FALLTHRU_PREHEADERS
diff --git a/gcc/testsuite/gcc.dg/pr84659.c b/gcc/testsuite/gcc.dg/pr84659.c
index e69de29bb2d..94c885f3869 100644
--- a/gcc/testsuite/gcc.dg/pr84659.c
+++ b/gcc/testsuite/gcc.dg/pr84659.c
@@ -0,0 +1,19 @@ 
+/* PR rtl-optimization/84659 */
+/* { dg-do compile { target powerpc*-*-* ia64-*-* i?86-*-* x86_64-*-* } } */
+/* { dg-options "-O2 -fselective-scheduling -fsel-sched-pipelining -fno-split-wide-types -fno-strict-aliasing -fno-tree-dce" } */
+
+void
+jk (int **lq, int *k4, long long int qv, int od)
+{
+  while (**lq < 1)
+    {
+      int uo;
+
+      uo = ((od == 0) ? qv : *k4) != 1;
+      ++**lq;
+    }
+
+  for (;;)
+    {
+    }
+}