| Message ID | 1548256275-42315-1-git-send-email-dmalcolm@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | Update assertion in sched-ebb.c to cope with table jumps | expand |
On 1/23/19 8:11 AM, David Malcolm wrote: > On Wed, 2019-01-23 at 16:52 +0300, Alexander Monakov wrote: >> On Wed, 23 Jan 2019, Andrey Belevantsev wrote: >> >>> For that, I'm not sure. Your patch will leave the tablejump >>> unscheduled at >>> all, i.e. any fields like INSN_TICK would be unfilled and thus the >>> later >>> passes like bundling on ia64 will not work. Maybe we can just stop >>> tablejumps from moving within its block, Alexander? >> >> On the Bugzilla there's an alternative patch by Segher that adjusts >> the assert >> to accept this situation (there's still a barrier insn after the >> tablejump insn, >> it's just after a jump_table_data insn rather than immediately >> following the >> jump). That should be a better approach, and David said he was >> testing it. >> >> That said, I'm really concerned that on this testcase we should not >> be moving >> the tablejump *at all*: we are moving it up past a 'use ax' insn (the >> use is >> for the function's return value). So after the move the use is in an >> unreachable >> block, which makes no sense. >> >> So my concern is that just fixing the assert changes the issue from >> the ICE to a >> (latent) wrong-code issue. >> >> There should have been an anti-dependence between the use and the >> jump. I'll try >> to investigate. >> >> Alexander > > Thanks everyone for their clarifications (this is somewhat outside > my normal comfort zone of diagnostics/frontend stuff...) > > Here's Segher's patch to sched-ebb.c, along with a couple of compile-only > testcases I put together (which triggered ICEs on x86_64 and powerpc > without the sched-ebb.c fix). > > Successfully bootstrapped & regrtested on x86_64-pc-linux-gnu, and this > fixes the ICE in the the powerpc testcase. > > That said, I share your concern that this might be papering over a > latent wrong-code issue. > > Dave > > gcc/ChangeLog: > PR rtl-optimization/88347 > PR rtl-optimization/88423 > * sched-ebb.c (begin_move_insn): Update assertion to handle table > jumps. > > gcc/testsuite/ChangeLog: > PR rtl-optimization/88347 > PR rtl-optimization/88423 > * gcc.c-torture/compile/pr88347.c: New test. > * gcc.c-torture/compile/pr88423.c: New test. To touch on one of the issues I raised. AFAICT the schedulers don't use SCHED_GROUP_P for dealing with tablejumps. They're used for cc0-user/setter, fused insns and the like. That's a bit of a surprise. Given that the table isn't actually associated with the block with the tablejump, SCHED_GROUP_P might actually create a whole new set of problems. Why oh why is the jump table data not actually attached to the tablejump insn itself. Nearly 30 years of working on GCC and I can't answer that one. I slightly prefer Alexander's version since it appears to arrange for a scheduling barrier at the jump. The one obvious worry I have is it may not be safe in the presence of DEBUG_INSNs and/or additional note insns due to its use of NEXT_INSN. But given the rest of the compiler should be keeping these 3 insns together, I suspect if something broke them apart with an ill-placed DEBUG_INSN or NOTE_INSN_DELETED or whatever we'd be seeing other problems. Jeff
On Mon, Mar 25, 2019 at 05:12:05PM -0600, Jeff Law wrote: > To touch on one of the issues I raised. AFAICT the schedulers don't use > SCHED_GROUP_P for dealing with tablejumps. They're used for > cc0-user/setter, fused insns and the like. That's a bit of a surprise. > > Given that the table isn't actually associated with the block with the > tablejump, SCHED_GROUP_P might actually create a whole new set of problems. > > Why oh why is the jump table data not actually attached to the tablejump > insn itself. Nearly 30 years of working on GCC and I can't answer that one. It somewhat makes sense for targets where the jump table is emitted in the text section; we then need to know its size for jumps over it, etc. Segher
On 3/26/19 11:52 AM, Segher Boessenkool wrote: > On Mon, Mar 25, 2019 at 05:12:05PM -0600, Jeff Law wrote: >> To touch on one of the issues I raised. AFAICT the schedulers don't use >> SCHED_GROUP_P for dealing with tablejumps. They're used for >> cc0-user/setter, fused insns and the like. That's a bit of a surprise. >> >> Given that the table isn't actually associated with the block with the >> tablejump, SCHED_GROUP_P might actually create a whole new set of problems. >> >> Why oh why is the jump table data not actually attached to the tablejump >> insn itself. Nearly 30 years of working on GCC and I can't answer that one. > > It somewhat makes sense for targets where the jump table is emitted in the > text section; we then need to know its size for jumps over it, etc. > ISTM if you attach the table to the indirect jump, then you could include the size of hte table in the size of the indirect jump on targets where the table is emitted inline in the text section. I don't think there's anything we're doing now that couldn't be done with the table attached to the jump. I would even claim that some things become notably easier :-) But none of that is gcc-9 material. jeff
On Tue, Mar 26, 2019 at 12:15:26PM -0600, Jeff Law wrote: > On 3/26/19 11:52 AM, Segher Boessenkool wrote: > > On Mon, Mar 25, 2019 at 05:12:05PM -0600, Jeff Law wrote: > >> To touch on one of the issues I raised. AFAICT the schedulers don't use > >> SCHED_GROUP_P for dealing with tablejumps. They're used for > >> cc0-user/setter, fused insns and the like. That's a bit of a surprise. > >> > >> Given that the table isn't actually associated with the block with the > >> tablejump, SCHED_GROUP_P might actually create a whole new set of problems. > >> > >> Why oh why is the jump table data not actually attached to the tablejump > >> insn itself. Nearly 30 years of working on GCC and I can't answer that one. > > > > It somewhat makes sense for targets where the jump table is emitted in the > > text section; we then need to know its size for jumps over it, etc. > > > ISTM if you attach the table to the indirect jump, then you could > include the size of hte table in the size of the indirect jump on > targets where the table is emitted inline in the text section. > > I don't think there's anything we're doing now that couldn't be done > with the table attached to the jump. I would even claim that some > things become notably easier :-) I agree; I was musing why it grew this way historically. > But none of that is gcc-9 material. Right, but it would be lovely if someone picked it up for GCC 10. Hint hint. Segher
diff --git a/gcc/sched-ebb.c b/gcc/sched-ebb.c index d459e09..76a72b0 100644 --- a/gcc/sched-ebb.c +++ b/gcc/sched-ebb.c @@ -172,7 +172,11 @@ begin_move_insn (rtx_insn *insn, rtx_insn *last) if (e) gcc_checking_assert (NOTE_P (x) || LABEL_P (x)); else - gcc_checking_assert (BARRIER_P (x)); + { + if (LABEL_P (x) && JUMP_TABLE_DATA_P (NEXT_INSN (x))) + x = NEXT_INSN (NEXT_INSN (x)); + gcc_checking_assert (BARRIER_P (x)); + } } if (e) diff --git a/gcc/testsuite/gcc.c-torture/compile/pr88347.c b/gcc/testsuite/gcc.c-torture/compile/pr88347.c new file mode 100644 index 0000000..4e9b438 --- /dev/null +++ b/gcc/testsuite/gcc.c-torture/compile/pr88347.c @@ -0,0 +1,4 @@ +/* { dg-do compile { target { powerpc-*-* powerpcle-*-* } } } */ +/* { dg-options "-mcpu=603e -fsched-stalled-insns -fsched2-use-superblocks -fschedule-insns2 -fno-dce -fno-guess-branch-probability --param max-cse-insns=4" } */ + +#include "../../gcc.target/powerpc/ppc-switch-2.c" diff --git a/gcc/testsuite/gcc.c-torture/compile/pr88423.c b/gcc/testsuite/gcc.c-torture/compile/pr88423.c new file mode 100644 index 0000000..4948817 --- /dev/null +++ b/gcc/testsuite/gcc.c-torture/compile/pr88423.c @@ -0,0 +1,5 @@ +/* { dg-do compile { target { i?86-*-* x86_64-*-* } } } */ +/* { dg-options "-march=skylake -fPIC -fsched2-use-superblocks -fno-if-conversion" } */ +/* { dg-require-effective-target fpic } */ + +#include "../../gcc.dg/20030309-1.c"