Hi David,
On 18.01.2019 19:58, David Malcolm wrote:
> 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.
This code was added at the time of our work for the speculation support in
ia64. I would guess it was just never designed to support tablejumps,
rather the jumps to recovery blocks or some such. But I might be wrong, it
was back in 2006. If you look at fix_jump_move, which was added about that
time, it doesn't have any traces of tablejumps as well.
>>
>> 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?
You are right, it's not a tree structure in the sense of classical EBBs but
rather a trace. There was a code to also perform tail duplication in order
to have better traces for scheduling, but the corresponding option got
deprecated back in 2010.
>>
>> 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)
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?
Andrey
>>
>> 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"
