Hi Jeff,
on 2024/8/26 06:13, Jeff Law wrote:
>
> So is this patch still relevant Kewen?
Yes, sorry that I forgot to follow up this after stage 1 opens.
>
> 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.
Thanks for your review comments, I'll check SCHED_GROUP_P and get back to you
soon.
BR,
Kewen
