Hi Alexander/Richard/Jeff, Thanks for the insightful comments!
on 2023/11/10 22:41, Alexander Monakov wrote: > > On Fri, 10 Nov 2023, Richard Biener wrote: > >> On Fri, Nov 10, 2023 at 3:18 PM Alexander Monakov <amona...@ispras.ru> wrote: >>> >>> >>> On Fri, 10 Nov 2023, Richard Biener wrote: >>> >>>>> I'm afraid ignoring debug-only BBs goes contrary to overall var-tracking >>>>> design: >>>>> DEBUG_INSNs participate in dependency graph so that schedulers can remove >>>>> or >>>>> mutate them as needed when moving real insns across them. >>>> >>>> Note that debug-only BBs do not exist - the BB would be there even without >>>> debug >>>> insns! >>> >>> Yep, sorry, I misspoke when I earlier said >>> >>>>> and cause divergence when passing through a debug-only BB which would not >>>>> be >>>>> present at all without -g. >>> >>> They are present in the region, but skipped via no_real_insns_p. >>> >>>> So instead you have to handle BBs with just debug insns the same you >>>> handle a completely empty BB. >>> >>> Yeah. There would be no problem if the scheduler never used no_real_insns_p >>> and handled empty and non-empty BBs the same way. >> >> And I suppose it would be OK to do that. Empty BBs are usually removed by >> CFG cleanup so the situation should only happen in rare corner cases where >> the fix would be to actually run CFG cleanup ... > > Yeah, sel-sched invokes 'cfg_cleanup (0)' up front, and I suppose that > may be a preferable compromise for sched-rgn as well. > Inspired by this discussion, I tested the attached patch 1 which is to run cleanup_cfg (0) first in haifa_sched_init, it's bootstrapped and regress-tested on x86_64-redhat-linux and powerpc64{,le}-linux-gnu. Then I assumed some of the current uses of no_real_insns_p won't encounter empty blocks any more, so made a patch 2 with some explicit assertions, but unfortunately I got ICEs during bootstrapping happens in function compute_priorities. I'm going to investigate it further and post more findings, but just heads-up to ensure if this is on the right track. BR, Kewen
From 7652655f278cfe0f6271c50aecb56e68e0877cc2 Mon Sep 17 00:00:00 2001 From: Kewen Lin <li...@linux.ibm.com> Date: Tue, 14 Nov 2023 15:16:47 +0800 Subject: [PATCH] sched: cleanup cfg for empty blocks first in haifa_sched_init [PR108273] PR108273 exposed the inconsistent states issue between non-debug mode and debug mode, as the discussion in [1], we can follow the current practice in sel_global_init, to run cleanup_cfg (0) first to remove empty blocks. This patch is to follow this direction and remove empty blocks by cleanup_cfg (0) in haifa_sched_init which affects sms, ebb and rgn schedulings. PR rtl-optimization/108273 gcc/ChangeLog: * haifa-sched.cc (haifa_sched_init): Call cleanup_cfg (0) to remove empty blocks. --- gcc/haifa-sched.cc | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/gcc/haifa-sched.cc b/gcc/haifa-sched.cc index 8e8add709b3..e348d1a2119 100644 --- a/gcc/haifa-sched.cc +++ b/gcc/haifa-sched.cc @@ -7375,6 +7375,12 @@ haifa_sched_init (void) sched_deps_info->generate_spec_deps = 1; } + /* Remove empty blocks to avoid some inconsistency like: we skip + empty block in scheduling but don't for empty block + only + debug_insn, it could result in different subsequent states + and unexpected insn sequence difference. */ + cleanup_cfg (0); + /* Initialize luids, dependency caches, target and h_i_d for the whole function. */ { -- 2.39.1
From efe1ed8fb5b151c9c4819ff1fa9af579151e259c Mon Sep 17 00:00:00 2001 From: Kewen Lin <li...@linux.ibm.com> Date: Tue, 14 Nov 2023 15:39:24 +0800 Subject: [PATCH] sched: Assert we don't have any chance to get empty blocks att. gcc/ChangeLog: * sched-ebb.cc (schedule_ebb): Assert no empty blocks. * sched-rgn.cc (compute_priorities): Likewise. (schedule_region): Likewise. --- gcc/sched-ebb.cc | 5 ++++- gcc/sched-rgn.cc | 7 ++++++- 2 files changed, 10 insertions(+), 2 deletions(-) diff --git a/gcc/sched-ebb.cc b/gcc/sched-ebb.cc index 110fcdbca4d..9fdfd72b6fc 100644 --- a/gcc/sched-ebb.cc +++ b/gcc/sched-ebb.cc @@ -492,7 +492,10 @@ schedule_ebb (rtx_insn *head, rtx_insn *tail, bool modulo_scheduling) last_bb = BLOCK_FOR_INSN (tail); if (no_real_insns_p (head, tail)) - return BLOCK_FOR_INSN (tail); + { + gcc_unreachable (); + return BLOCK_FOR_INSN (tail); + } gcc_assert (INSN_P (head) && INSN_P (tail)); diff --git a/gcc/sched-rgn.cc b/gcc/sched-rgn.cc index 1c8acf5068a..795c455872e 100644 --- a/gcc/sched-rgn.cc +++ b/gcc/sched-rgn.cc @@ -3025,7 +3025,10 @@ compute_priorities (void) get_ebb_head_tail (EBB_FIRST_BB (bb), EBB_LAST_BB (bb), &head, &tail); if (no_real_insns_p (head, tail)) - continue; + { + gcc_unreachable (); + continue; + } rgn_n_insns += set_priorities (head, tail); } @@ -3160,6 +3163,7 @@ schedule_region (int rgn) if (no_real_insns_p (head, tail)) { + gcc_unreachable (); gcc_assert (first_bb == last_bb); continue; } @@ -3180,6 +3184,7 @@ schedule_region (int rgn) if (no_real_insns_p (head, tail)) { + gcc_unreachable (); gcc_assert (first_bb == last_bb); save_state_for_fallthru_edge (last_bb, bb_state[first_bb->index]); continue; -- 2.39.1