On 1/18/19 10:32 AM, David Malcolm wrote: > PR rtl-optimization/88423 reports an ICE within sched-ebb.c's > begin_move_insn, failing the assertion at line 175, where there's > no fall-through edge: > > 171 rtx_insn *x = NEXT_INSN (insn); > 172 if (e) > 173 gcc_checking_assert (NOTE_P (x) || LABEL_P (x)); > 174 else > 175 gcc_checking_assert (BARRIER_P (x)); > > "insn" is a jump_insn for a table jump, and its NEXT_INSN is the > placeholder code_label, followed by the jump_table_data. > > It's not clear to me if such a jump_insn can be repositioned within > the insn stream, or if the scheduler is able to do so. I believe a > tablejump is always at the end of such a head/tail insn sub-stream. > Is it a requirement that the placeholder code_label for the jump_insn > is always its NEXT_INSN? > > The loop at the start of schedule_ebb adjusts the head and tail > of the insns to be scheduled so that it skips leading and trailing notes > and debug insns. > > This patch adjusts that loop to also skip trailing jump_insn instances > that are table jumps, so that we don't attempt to move such table jumps. > > This fixes the ICE, but I'm not very familiar with this part of the > compiler - so I have two questions: > > (1) What does GCC mean by "ebb" in this context? > > My understanding is that the normal definition of an "extended basic > block" (e.g. Muchnick's book pp175-177) is that it's a maximal grouping > of basic blocks where only one BB in each group has multiple in-edges > and all other BBs in the group have a single in-edge (and thus e.g. > there's a tree-like structure of BBs within each EBB). > > From what I can tell, schedule_ebbs is iterating over BBs, looking for > runs of BBs joined by next_bb that are connected by fallthrough edges > and don't have labels (and aren't flagged with BB_DISABLE_SCHEDULE). > It uses this run of BBs to generate a run of instructions within the > NEXT_INSN/PREV_INSN doubly-linked list, which it passes as "head" > and "tail" to schedule_ebb. > > This sounds like it will be a group of basic blocks with single in-edges > internally, but it isn't a *maximal* group of such BBs - but perhaps > it's "maximal" in the sense of what the NEXT_INSN/PREV_INSN > representation can cope with? > > There (presumably) can't be a fallthrough edge after a table jump, so > a table jump could only ever be at the end of such a chain, never in the > middle. > > (2) Is it OK to omit "tail" from consideration here, from a dataflow > and insn-dependency point-of-view? Presumably the scheduler is written > to ensure that data used by subsequent basic blocks will still be available > after the insns within an "EBB" are reordered, so presumably any data > uses *within* the jump_insn are still going to be available - but, as I > said, I'm not very familiar with this part of the code. (I think I'm also > assuming that the jump_insn can't clobber data, just the PC) > > Successfully bootstrapped & regrtested on x86_64-pc-linux-gnu. > > OK for trunk? > > gcc/ChangeLog: > PR rtl-optimization/88423 > * sched-ebb.c (schedule_ebb): Don't move the jump_insn for a table > jump. > > gcc/testsuite/ChangeLog: > PR rtl-optimization/88423 > * gcc.c-torture/compile/pr88423.c: New test. I wonder if this should be generalized for SCHED_GROUP_P which is hte standard mechanism we use to ensure two insns fire together.
Can you check if SCHED_GROUP_P is set on either the jump or the jump table (I forget if it belongs on the first or the last insn). It may make more sense to be checking for SCHED_GROUP_P in sched_ebb rather than special casing tablejump_p. I'm not likely to be able to dig further into this for a while. Vlad might be able to provide guidance. jeff
