| Message ID | b453ef22-0a65-4b4e-3241-b18c074ef3f7@ispras.ru |
|---|---|
| State | New |
| Headers | show |
| Series | Fix PR 86979 | expand |
On Fri, Mar 15, 2019 at 01:25:57PM +0300, Andrey Belevantsev wrote: > As explained in the PR trail, we incorrectly update the availability sets > in the rare case of several successors and one of them having another > fence. Fixed as follows. Ok for trunk? > > Best, > Andrey > > 2019-03-15 Andrey Belevantsev <abel@ispras.ru> > > PR middle-end/89676 > * sel-sched.c (compute_av_set_at_bb_end): When we have an ineligible > successor, > use NULL as its av set. Just formatting nits, will defer actual review to some scheduler maintainer. Too long line in ChangeLog above, "successor," should be on the next line already together with the rest of the description. > diff --git a/gcc/sel-sched.c b/gcc/sel-sched.c > index 315f2c0c0ab..2053694b196 100644 > --- a/gcc/sel-sched.c > +++ b/gcc/sel-sched.c > @@ -2820,10 +2820,12 @@ compute_av_set_at_bb_end (insn_t insn, ilist_t p, int ws) > FOR_EACH_VEC_ELT (sinfo->succs_ok, is, succ) > { > basic_block succ_bb = BLOCK_FOR_INSN (succ); > + av_set_t av_succ = (is_ineligible_successor (succ, p) > + ? NULL > + : BB_AV_SET (succ_bb)); > > gcc_assert (BB_LV_SET_VALID_P (succ_bb)); > - mark_unavailable_targets (av1, BB_AV_SET (succ_bb), > - BB_LV_SET (succ_bb)); > + mark_unavailable_targets (av1, av_succ, BB_LV_SET (succ_bb)); The above line should use tab instead of 8 spaces. Jakub
On Fri, 15 Mar 2019, Jakub Jelinek wrote: > On Fri, Mar 15, 2019 at 01:25:57PM +0300, Andrey Belevantsev wrote: > > As explained in the PR trail, we incorrectly update the availability sets > > in the rare case of several successors and one of them having another > > fence. Fixed as follows. Ok for trunk? > > > > Best, > > Andrey > > > > 2019-03-15 Andrey Belevantsev <abel@ispras.ru> > > > > PR middle-end/89676 > > * sel-sched.c (compute_av_set_at_bb_end): When we have an ineligible > > successor, > > use NULL as its av set. > > Just formatting nits, will defer actual review to some scheduler maintainer. I can do this, it falls under sel-sched maintenance and Andrey showed to me in person the algorithmic corner-case here, so: OK for trunk with formatting fixed. Thanks! Alexander
On Fri, Mar 15, 2019 at 02:27:35PM +0300, Alexander Monakov wrote: > On Fri, 15 Mar 2019, Jakub Jelinek wrote: > > > On Fri, Mar 15, 2019 at 01:25:57PM +0300, Andrey Belevantsev wrote: > > > As explained in the PR trail, we incorrectly update the availability sets > > > in the rare case of several successors and one of them having another > > > fence. Fixed as follows. Ok for trunk? > > > > > > Best, > > > Andrey > > > > > > 2019-03-15 Andrey Belevantsev <abel@ispras.ru> > > > > > > PR middle-end/89676 > > > * sel-sched.c (compute_av_set_at_bb_end): When we have an ineligible > > > successor, > > > use NULL as its av set. > > > > Just formatting nits, will defer actual review to some scheduler maintainer. > > I can do this, it falls under sel-sched maintenance and Andrey showed to me > in person the algorithmic corner-case here, so: OK for trunk with formatting > fixed. Can you please update MAINTAINERS for you then (and Dmitry too)? In https://gcc.gnu.org/ml/gcc/2011-11/msg00264.html you are listed all 3, but only Abel has added himself to selective scheduling in MAINTAINERS in r181284. Jakub
On Fri, Mar 15, 2019 at 01:25:57PM +0300, Andrey Belevantsev wrote: > 2019-03-15 Andrey Belevantsev <abel@ispras.ru> > > PR middle-end/89676 There is a typo in the PR number that I've fixed and added a testcase for this, committed as obvious: 2019-03-18 Jakub Jelinek <jakub@redhat.com> PR middle-end/86979 * gcc.dg/pr86979.c: New test. --- gcc/testsuite/gcc.dg/pr86979.c.jj 2019-03-18 09:28:00.090554402 +0100 +++ gcc/testsuite/gcc.dg/pr86979.c 2019-03-18 09:27:49.313728712 +0100 @@ -0,0 +1,5 @@ +/* { dg-options "-Og -fPIC -fschedule-insns2 -fselective-scheduling2 -fno-tree-fre --param=max-sched-extend-regions-iters=2" } */ +/* { dg-require-effective-target scheduling } */ +/* { dg-require-effective-target fpic } */ + +#include "../gcc.c-torture/compile/pr69102.c" --- gcc/ChangeLog.jj 2019-03-18 09:17:43.669527380 +0100 +++ gcc/ChangeLog 2019-03-18 09:30:15.647361882 +0100 @@ -1,6 +1,6 @@ 2019-03-18 Andrey Belevantsev <abel@ispras.ru> - PR middle-end/89676 + PR middle-end/86979 * sel-sched.c (compute_av_set_at_bb_end): When we have an ineligible successor, use NULL as its av set. Jakub
diff --git a/gcc/sel-sched.c b/gcc/sel-sched.c index 315f2c0c0ab..2053694b196 100644 --- a/gcc/sel-sched.c +++ b/gcc/sel-sched.c @@ -2820,10 +2820,12 @@ compute_av_set_at_bb_end (insn_t insn, ilist_t p, int ws) FOR_EACH_VEC_ELT (sinfo->succs_ok, is, succ) { basic_block succ_bb = BLOCK_FOR_INSN (succ); + av_set_t av_succ = (is_ineligible_successor (succ, p) + ? NULL + : BB_AV_SET (succ_bb)); gcc_assert (BB_LV_SET_VALID_P (succ_bb)); - mark_unavailable_targets (av1, BB_AV_SET (succ_bb), - BB_LV_SET (succ_bb)); + mark_unavailable_targets (av1, av_succ, BB_LV_SET (succ_bb)); } /* Finally, check liveness restrictions on paths leaving the region. */