diff mbox series

sched: Remove debug counter sched_block

Message ID be4a62d2-32eb-eb3b-56de-801d602e364d@linux.ibm.com
State Accepted
Headers
Series sched: Remove debug counter sched_block |

Checks

Context Check Description
snail/gcc-patch-check success Github commit url

Commit Message

Kewen.Lin Nov. 15, 2023, 9:01 a.m. UTC 
  Hi,

on 2023/11/10 01:40, Alexander Monakov wrote:

> I agree with the concern. I hoped that solving the problem by skipping the BB
> like the (bit-rotted) debug code needs to would be a minor surgery. As things
> look now, it may be better to remove the non-working sched_block debug counter
> entirely and implement a good solution for the problem at hand.
> 

According to this comment, I made and tested the below patch to remove the
problematic debug counter:

Subject: [PATCH] sched: Remove debug counter sched_block

Currently the debug counter sched_block doesn't work well
since we create dependencies for some insns and those
dependencies are expected to be resolved during scheduling
insns but they can get skipped once we are skipping some
block while respecting sched_block debug counter.

For example, for the below test case:
--
int a, b, c, e, f;
float d;

void
g ()
{
  float h, i[1];
  for (; f;)
    if (c)
      {
	d *e;
	if (b)
	  {
	    float *j = i;
	    j[0] = 0;
	  }
	h = d;
      }
  if (h)
    a = i[0];
}
--
ICE occurs with option "-O2 -fdbg-cnt=sched_block:1".

As the discussion in [1], it seems that we think this debug
counter is useless and can be removed.  It's also implied
that if it's useful and used often, the above issue should
have been cared about and resolved earlier.  So this patch
is to remove this debug counter.

Bootstrapped and regtested on x86_64-redhat-linux and
powerpc64{,le}-linux-gnu.

[1] https://gcc.gnu.org/pipermail/gcc-patches/2023-November/635852.html

Is it ok for trunk?

BR,
Kewen
-----

gcc/ChangeLog:

	* dbgcnt.def (sched_block): Remove.
	* sched-rgn.cc (schedule_region): Remove the support of debug count
	sched_block.
---
 gcc/dbgcnt.def   |  1 -
 gcc/sched-rgn.cc | 19 ++++++-------------
 2 files changed, 6 insertions(+), 14 deletions(-)

--
2.39.1

Comments

Kewen.Lin Dec. 12, 2023, 6:17 a.m. UTC | #1 
Hi,

Gentle ping this:

https://gcc.gnu.org/pipermail/gcc-patches/2023-November/636597.html

BR,
Kewen

on 2023/11/15 17:01, Kewen.Lin wrote:
> Hi,
> 
> on 2023/11/10 01:40, Alexander Monakov wrote:
> 
>> I agree with the concern. I hoped that solving the problem by skipping the BB
>> like the (bit-rotted) debug code needs to would be a minor surgery. As things
>> look now, it may be better to remove the non-working sched_block debug counter
>> entirely and implement a good solution for the problem at hand.
>>
> 
> According to this comment, I made and tested the below patch to remove the
> problematic debug counter:
> 
> Subject: [PATCH] sched: Remove debug counter sched_block
> 
> Currently the debug counter sched_block doesn't work well
> since we create dependencies for some insns and those
> dependencies are expected to be resolved during scheduling
> insns but they can get skipped once we are skipping some
> block while respecting sched_block debug counter.
> 
> For example, for the below test case:
> --
> int a, b, c, e, f;
> float d;
> 
> void
> g ()
> {
>   float h, i[1];
>   for (; f;)
>     if (c)
>       {
> 	d *e;
> 	if (b)
> 	  {
> 	    float *j = i;
> 	    j[0] = 0;
> 	  }
> 	h = d;
>       }
>   if (h)
>     a = i[0];
> }
> --
> ICE occurs with option "-O2 -fdbg-cnt=sched_block:1".
> 
> As the discussion in [1], it seems that we think this debug
> counter is useless and can be removed.  It's also implied
> that if it's useful and used often, the above issue should
> have been cared about and resolved earlier.  So this patch
> is to remove this debug counter.
> 
> Bootstrapped and regtested on x86_64-redhat-linux and
> powerpc64{,le}-linux-gnu.
> 
> [1] https://gcc.gnu.org/pipermail/gcc-patches/2023-November/635852.html
> 
> Is it ok for trunk?
> 
> BR,
> Kewen
> -----
> 
> gcc/ChangeLog:
> 
> 	* dbgcnt.def (sched_block): Remove.
> 	* sched-rgn.cc (schedule_region): Remove the support of debug count
> 	sched_block.
> ---
>  gcc/dbgcnt.def   |  1 -
>  gcc/sched-rgn.cc | 19 ++++++-------------
>  2 files changed, 6 insertions(+), 14 deletions(-)
> 
> diff --git a/gcc/dbgcnt.def b/gcc/dbgcnt.def
> index 871cbf75d93..a8c4e61e13d 100644
> --- a/gcc/dbgcnt.def
> +++ b/gcc/dbgcnt.def
> @@ -198,7 +198,6 @@ DEBUG_COUNTER (pre_insn)
>  DEBUG_COUNTER (prefetch)
>  DEBUG_COUNTER (registered_jump_thread)
>  DEBUG_COUNTER (sched2_func)
> -DEBUG_COUNTER (sched_block)
>  DEBUG_COUNTER (sched_breakdep)
>  DEBUG_COUNTER (sched_func)
>  DEBUG_COUNTER (sched_insn)
> diff --git a/gcc/sched-rgn.cc b/gcc/sched-rgn.cc
> index e5964f54ead..1c8acf5068a 100644
> --- a/gcc/sched-rgn.cc
> +++ b/gcc/sched-rgn.cc
> @@ -3198,20 +3198,13 @@ schedule_region (int rgn)
>        current_sched_info->queue_must_finish_empty = current_nr_blocks == 1;
> 
>        curr_bb = first_bb;
> -      if (dbg_cnt (sched_block))
> -        {
> -	  int saved_last_basic_block = last_basic_block_for_fn (cfun);
> +      int saved_last_basic_block = last_basic_block_for_fn (cfun);
> 
> -	  schedule_block (&curr_bb, bb_state[first_bb->index]);
> -	  gcc_assert (EBB_FIRST_BB (bb) == first_bb);
> -	  sched_rgn_n_insns += sched_n_insns;
> -	  realloc_bb_state_array (saved_last_basic_block);
> -	  save_state_for_fallthru_edge (last_bb, curr_state);
> -        }
> -      else
> -        {
> -          sched_rgn_n_insns += rgn_n_insns;
> -        }
> +      schedule_block (&curr_bb, bb_state[first_bb->index]);
> +      gcc_assert (EBB_FIRST_BB (bb) == first_bb);
> +      sched_rgn_n_insns += sched_n_insns;
> +      realloc_bb_state_array (saved_last_basic_block);
> +      save_state_for_fallthru_edge (last_bb, curr_state);
> 
>        /* Clean up.  */
>        if (current_nr_blocks > 1)
> --
> 2.39.1
Kewen.Lin Dec. 21, 2023, 5:46 a.m. UTC | #2 
Hi Jeff,

on 2023/12/21 04:43, Jeff Law wrote:
> 
> 
> On 12/11/23 23:17, Kewen.Lin wrote:
>> Hi,
>>
>> Gentle ping this:
>>
>> https://gcc.gnu.org/pipermail/gcc-patches/2023-November/636597.html
>>
>> BR,
>> Kewen
>>
>> on 2023/11/15 17:01, Kewen.Lin wrote:
>>> Hi,
>>>
>>> on 2023/11/10 01:40, Alexander Monakov wrote:
>>>
>>>> I agree with the concern. I hoped that solving the problem by skipping the BB
>>>> like the (bit-rotted) debug code needs to would be a minor surgery. As things
>>>> look now, it may be better to remove the non-working sched_block debug counter
>>>> entirely and implement a good solution for the problem at hand.
>>>>
>>>
>>> According to this comment, I made and tested the below patch to remove the
>>> problematic debug counter:
>>>
>>> Subject: [PATCH] sched: Remove debug counter sched_block
>>>
>>> Currently the debug counter sched_block doesn't work well
>>> since we create dependencies for some insns and those
>>> dependencies are expected to be resolved during scheduling
>>> insns but they can get skipped once we are skipping some
>>> block while respecting sched_block debug counter.
>>>
>>> For example, for the below test case:
>>> -- 
>>> int a, b, c, e, f;
>>> float d;
>>>
>>> void
>>> g ()
>>> {
>>>    float h, i[1];
>>>    for (; f;)
>>>      if (c)
>>>        {
>>>     d *e;
>>>     if (b)
>>>       {
>>>         float *j = i;
>>>         j[0] = 0;
>>>       }
>>>     h = d;
>>>        }
>>>    if (h)
>>>      a = i[0];
>>> }
>>> -- 
>>> ICE occurs with option "-O2 -fdbg-cnt=sched_block:1".
>>>
>>> As the discussion in [1], it seems that we think this debug
>>> counter is useless and can be removed.  It's also implied
>>> that if it's useful and used often, the above issue should
>>> have been cared about and resolved earlier.  So this patch
>>> is to remove this debug counter.
>>>
>>> Bootstrapped and regtested on x86_64-redhat-linux and
>>> powerpc64{,le}-linux-gnu.
>>>
>>> [1] https://gcc.gnu.org/pipermail/gcc-patches/2023-November/635852.html
>>>
>>> Is it ok for trunk?
>>>
>>> BR,
>>> Kewen
>>> -----
>>>
>>> gcc/ChangeLog:
>>>
>>>     * dbgcnt.def (sched_block): Remove.
>>>     * sched-rgn.cc (schedule_region): Remove the support of debug count
>>>     sched_block.
> OK.  SOrry about the delay.

Thanks for the review, pushed as r14-6766-gef259ebeb39501.

BR,
Kewen
diff mbox series

Patch

diff --git a/gcc/dbgcnt.def b/gcc/dbgcnt.def
index 871cbf75d93..a8c4e61e13d 100644
--- a/gcc/dbgcnt.def
+++ b/gcc/dbgcnt.def
@@ -198,7 +198,6 @@  DEBUG_COUNTER (pre_insn)
 DEBUG_COUNTER (prefetch)
 DEBUG_COUNTER (registered_jump_thread)
 DEBUG_COUNTER (sched2_func)
-DEBUG_COUNTER (sched_block)
 DEBUG_COUNTER (sched_breakdep)
 DEBUG_COUNTER (sched_func)
 DEBUG_COUNTER (sched_insn)
diff --git a/gcc/sched-rgn.cc b/gcc/sched-rgn.cc
index e5964f54ead..1c8acf5068a 100644
--- a/gcc/sched-rgn.cc
+++ b/gcc/sched-rgn.cc
@@ -3198,20 +3198,13 @@  schedule_region (int rgn)
       current_sched_info->queue_must_finish_empty = current_nr_blocks == 1;

       curr_bb = first_bb;
-      if (dbg_cnt (sched_block))
-        {
-	  int saved_last_basic_block = last_basic_block_for_fn (cfun);
+      int saved_last_basic_block = last_basic_block_for_fn (cfun);

-	  schedule_block (&curr_bb, bb_state[first_bb->index]);
-	  gcc_assert (EBB_FIRST_BB (bb) == first_bb);
-	  sched_rgn_n_insns += sched_n_insns;
-	  realloc_bb_state_array (saved_last_basic_block);
-	  save_state_for_fallthru_edge (last_bb, curr_state);
-        }
-      else
-        {
-          sched_rgn_n_insns += rgn_n_insns;
-        }
+      schedule_block (&curr_bb, bb_state[first_bb->index]);
+      gcc_assert (EBB_FIRST_BB (bb) == first_bb);
+      sched_rgn_n_insns += sched_n_insns;
+      realloc_bb_state_array (saved_last_basic_block);
+      save_state_for_fallthru_edge (last_bb, curr_state);

       /* Clean up.  */
       if (current_nr_blocks > 1)