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
