On Fri, 2019-01-18 at 12:32 -0500, David Malcolm wrote:
[CCing Abel]
> 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.
> ---
> gcc/sched-ebb.c | 4 ++++
> gcc/testsuite/gcc.c-torture/compile/pr88423.c | 5 +++++
> 2 files changed, 9 insertions(+)
> create mode 100644 gcc/testsuite/gcc.c-torture/compile/pr88423.c
>
> diff --git a/gcc/sched-ebb.c b/gcc/sched-ebb.c
> index d459e09..1fe0b76 100644
> --- a/gcc/sched-ebb.c
> +++ b/gcc/sched-ebb.c
> @@ -485,6 +485,10 @@ schedule_ebb (rtx_insn *head, rtx_insn *tail,
> bool modulo_scheduling)
> tail = PREV_INSN (tail);
> else if (LABEL_P (head))
> head = NEXT_INSN (head);
> + else if (tablejump_p (tail, NULL, NULL))
> + /* Don't move a jump_insn for a tablejump, to avoid having
> + to move the placeholder code_label and jump_table_data.
> */
> + tail = PREV_INSN (tail);
> else
> break;
> }
> 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"
