| 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) | expand |
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 (;;) > + { > + } > +} >
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 ();
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 (); >
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 (;;) + { + } +}
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.