| Message ID | be4a62d2-32eb-eb3b-56de-801d602e364d@linux.ibm.com |
|---|---|
| State | New |
| Headers | show |
| Series | sched: Remove debug counter sched_block | expand |
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
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. jeff
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 --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)