Re: PING^1 [PATCH v3] sched: Change no_real_insns_p to no_real_nondebug_insns_p [PR108273]

["Kewen.Lin" <linkw@linux.ibm.com>]

[-- Attachment #1: Type: text/plain, Size: 2061 bytes --]

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 <amonakov@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


[-- Attachment #2: 0001-sched-cleanup-cfg-for-empty-blocks-first-in-haifa_sc.patch --]
[-- Type: text/plain, Size: 1432 bytes --]

From 7652655f278cfe0f6271c50aecb56e68e0877cc2 Mon Sep 17 00:00:00 2001
From: Kewen Lin <linkw@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


[-- Attachment #3: 0001-sched-Assert-we-don-t-have-any-chance-to-get-empty-b.patch --]
[-- Type: text/plain, Size: 1825 bytes --]

From efe1ed8fb5b151c9c4819ff1fa9af579151e259c Mon Sep 17 00:00:00 2001
From: Kewen Lin <linkw@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