So is this patch still relevant Kewen?
On 12/20/23 2:25 AM, Kewen.Lin wrote:
Hi,
This patch follows Richi's suggestion "scheduling shouldn't
special case empty blocks as they usually do not appear" in
[1], it removes function no_real_insns_p and its uses
completely.
There is some case that one block previously has only one
INSN_P, but while scheduling some other blocks this only
INSN_P gets moved there and the block becomes empty so
that the only NOTE_P insn was counted then, but since this
block isn't empty initially and any NOTE_P gets skipped in
a normal block, the count to-be-scheduled doesn't count it
in, it can cause the below assertion to fail:
/* Sanity check: verify that all region insns were scheduled. */
gcc_assert (sched_rgn_n_insns == rgn_n_insns);
A bitmap rgn_init_empty_bb is proposed to detect such case
by recording one one block is empty initially or not before
actual scheduling. The other changes are mainly to handle
NOTE which wasn't expected before but now we have to face
with.
Bootstrapped and regress-tested on:
- powerpc64{,le}-linux-gnu
- x86_64-redhat-linux
- aarch64-linux-gnu
Also tested this with superblock scheduling (sched2) turned
on by default, bootstrapped and regress-tested again on the
above triples. I tried to test with seletive-scheduling
1/2 enabled by default, it's bootstrapped & regress-tested
on x86_64-redhat-linux, but both failed to build on
powerpc64{,le}-linux-gnu and aarch64-linux-gnu even without
this patch (so it's unrelated, I've filed two PRs for
observed failures on Power separately).
[1] https://inbox.sourceware.org/gcc-patches/CAFiYyc2hMvbU_+
a47ytnbxf0yrcybwrhru2jdcw5a0px3+n...@mail.gmail.com/
Is it ok for trunk or next stage 1?
BR,
Kewen
-----
PR rtl-optimization/108273
gcc/ChangeLog:
* config/aarch64/aarch64.cc (aarch64_sched_adjust_priority): Early
return for NOTE_P.
* haifa-sched.cc (recompute_todo_spec): Likewise.
(setup_insn_reg_pressure_info): Likewise.
(schedule_insn): Handle NOTE_P specially as we don't skip empty block
any more and adopt NONDEBUG_INSN_P somewhere appropriate.
(commit_schedule): Likewise.
(prune_ready_list): Likewise.
(schedule_block): Likewise.
(set_priorities): Likewise.
(fix_tick_ready): Likewise.
(no_real_insns_p): Remove.
* rtl.h (SCHED_GROUP_P): Add NOTE consideration.
* sched-ebb.cc (schedule_ebb): Skip leading labels like note to ensure
that we don't have the chance to have single label block, remove the
call to no_real_insns_p.
* sched-int.h (no_real_insns_p): Remove declaration.
* sched-rgn.cc (free_block_dependencies): Remove the call to
no_real_insns_p.
(compute_priorities): Likewise.
(schedule_region): Remove the call to no_real_insns_p, check
rgn_init_empty_bb and update rgn_n_insns if need.
(sched_rgn_local_init): Init rgn_init_empty_bb.
(sched_rgn_local_free): Free rgn_init_empty_bb.
(rgn_init_empty_bb): New static bitmap.
* sel-sched.cc (sel_region_target_finish): Remove the call to
no_real_insns_p.
This largely looks sensible. One change caught my eye though.
SCHED_GROUP_P IIRC only applies to INSNs. That bit means something
different for NOTEs. I think the change to rtl.h should be backed out,
which may mean you need further changes into the scheduler infrastructure.
Definitely will need a rebase and retest given the age of the patch.
jeff
