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
