| Message ID | b5200b1f-b51b-bff4-57c1-f7a03cf82a41@linux.ibm.com |
|---|---|
| State | New |
| Headers | show |
| Series | Add hints for slim dumping if fallthrough bb of jump isn't next bb | expand |
"Kewen.Lin" <linkw@linux.ibm.com> writes: > Hi all, > > 6: NOTE_INSN_BASIC_BLOCK 2 > .... > 12: r135:CC=cmp(r122:DI,0) > 13: pc={(r135:CC!=0)?L52:pc} > REG_DEAD r135:CC > REG_BR_PROB 1041558836 > 31: L31: > 17: NOTE_INSN_BASIC_BLOCK 3 > > The above RTL sequence is from pass doloop dumping with -fdump-rtl-all-slim, I > misunderstood that: the fall through BB of BB 2 is BB 3, since BB 3 is placed > just next to BB 2. Then I found the contradiction that BB 3 will have some > uninitialized regs if it's true. > > I can get the exact information with "-blocks" dumping and even detailed one > with "-details". But I'm thinking whether it's worth to giving some > information on "-slim" dump (or more exactly without "-blocks") to avoid some > confusion especially for new comers like me. Yeah, agree the output is overly confusing. > This patch is to add one line to hint what's the fallthrough BB if it's the > one closely sitting, for example: > > 6: NOTE_INSN_BASIC_BLOCK 2 > .... > 12: r135:CC=cmp(r122:DI,0) > 13: pc={(r135:CC!=0)?L52:pc} > REG_DEAD r135:CC > REG_BR_PROB 1041558836 > ;; pc falls through to BB 10 > 31: L31: > 17: NOTE_INSN_BASIC_BLOCK 3 > > Bootstrapped and regression test passed on powerpc64le-unknown-linux-gnu. > > Is it a reasonable patch? If yes, is it ok for trunk? It looks really useful, but IMO we should either emit the hint for all end-of-block insns with a surprising fallthrough edge, or -- if we really do want to keep it specific to conditional jumps -- we should replace rhs "pc" in the jump insn with something like "bb <N>". Personally the former seems better to me (and should also be simpler). > > > Thanks, > Kewen > > --------- > > gcc/ChangeLog > > 2019-07-08 Kewen Lin <linkw@gcc.gnu.org> > > * gcc/cfgrtl.c (print_rtl_with_bb): Check and call > hint_if_pc_fall_through_not_next for jump insn with two successors. > (hint_if_pc_fall_through_not_next): New function. > > diff --git a/gcc/cfgrtl.c b/gcc/cfgrtl.c > index a1ca5992c41..928b9b0f691 100644 > --- a/gcc/cfgrtl.c > +++ b/gcc/cfgrtl.c > @@ -2164,7 +2164,26 @@ rtl_dump_bb (FILE *outf, basic_block bb, int indent, dump_flags_t flags) > } > > } > - > + > +/* For dumping without specifying basic blocks option, when we see PC is one of > + jump targets, it's easy to misunderstand the next basic block is the > + fallthrough one, but it's not so true sometimes. This function is to dump > + hints for the case where basic block of next insn isn't the fall through > + target. */ > + > +static void > +hint_if_pc_fall_through_not_next (FILE *outf, basic_block bb) > +{ > + gcc_assert (outf); > + gcc_assert (EDGE_COUNT (bb->succs) >= 2); > + const rtx_insn *einsn = BB_END (bb); > + const rtx_insn *ninsn = NEXT_INSN (einsn); > + edge e = FALLTHRU_EDGE (bb); > + basic_block dest = e->dest; > + if (BB_HEAD (dest) != ninsn) > + fprintf (outf, ";; pc falls through to BB %d\n", dest->index); Very minor, but I think the output would be more readable if the comment were indented by the same amount as the register notes (like REG_DEAD above). In lisp tradition I guess the comment marker would then be ";" rather than ";;". Thanks, Richard > +} > + > /* Like dump_function_to_file, but for RTL. Print out dataflow information > for the start of each basic block. FLAGS are the TDF_* masks documented > in dumpfile.h. */ > @@ -2255,6 +2274,14 @@ print_rtl_with_bb (FILE *outf, const rtx_insn *rtx_first, dump_flags_t flags) > putc ('\n', outf); > } > } > + else if (GET_CODE (tmp_rtx) == JUMP_INSN > + && GET_CODE (PATTERN (tmp_rtx)) == SET) > + { > + bb = BLOCK_FOR_INSN (tmp_rtx); > + const_rtx src = SET_SRC (PATTERN (tmp_rtx)); > + if (bb != NULL && GET_CODE (src) == IF_THEN_ELSE) > + hint_if_pc_fall_through_not_next (outf, bb); > + } > } > > free (start);
Hi Richard, on 2019/7/11 上午3:30, Richard Sandiford wrote: > "Kewen.Lin" <linkw@linux.ibm.com> writes: >> Hi all, >> Is it a reasonable patch? If yes, is it ok for trunk? > > It looks really useful, but IMO we should either emit the hint for all > end-of-block insns with a surprising fallthrough edge, or -- if we > really do want to keep it specific to conditional jumps -- we should > replace rhs "pc" in the jump insn with something like "bb <N>". > Personally the former seems better to me (and should also be simpler). > Thanks a lot for the comments. It's a good idea not only to check conditional jump insn. I've updated the patch accordingly. I was intended to change the pc dumping by using the actual BB label similar to what we see for non fallthrough edge. But as you mentioned it's not simple, need to pass some information down, it looks like a hint is enough for this case. :) >> + basic_block dest = e->dest; >> + if (BB_HEAD (dest) != ninsn) >> + fprintf (outf, ";; pc falls through to BB %d\n", dest->index); > > Very minor, but I think the output would be more readable if the comment > were indented by the same amount as the register notes (like REG_DEAD above). > In lisp tradition I guess the comment marker would then be ";" rather > than ";;". > Good idea, also updated it accordingly, thanks! With the attached patch, the dumping is as below: 12: r135:CC=cmp(r122:DI,0) 13: pc={(r135:CC!=0)?L52:pc} REG_DEAD r135:CC REG_BR_PROB 1041558836 ; pc falls through to BB 10 31: L31: 17: NOTE_INSN_BASIC_BLOCK 3 Thanks, Kewen -------------- gcc/ChangeLog 2019-07-11 Kewen Lin <linkw@gcc.gnu.org> * gcc/cfgrtl.c (print_rtl_with_bb): Emit a hint if the fallthrough target of current basic block isn't the placed right next. diff --git a/gcc/cfgrtl.c b/gcc/cfgrtl.c index a1ca5992c41..5f2accf1f4f 100644 --- a/gcc/cfgrtl.c +++ b/gcc/cfgrtl.c @@ -2193,14 +2193,14 @@ print_rtl_with_bb (FILE *outf, const rtx_insn *rtx_first, dump_flags_t flags) if (df) df_dump_start (outf); - if (flags & TDF_BLOCKS) + FOR_EACH_BB_REVERSE_FN (bb, cfun) { - FOR_EACH_BB_REVERSE_FN (bb, cfun) - { - rtx_insn *x; + rtx_insn *x; - start[INSN_UID (BB_HEAD (bb))] = bb; - end[INSN_UID (BB_END (bb))] = bb; + start[INSN_UID (BB_HEAD (bb))] = bb; + end[INSN_UID (BB_END (bb))] = bb; + if (flags & TDF_BLOCKS) + { for (x = BB_HEAD (bb); x != NULL_RTX; x = NEXT_INSN (x)) { enum bb_state state = IN_MULTIPLE_BB; @@ -2244,16 +2244,31 @@ print_rtl_with_bb (FILE *outf, const rtx_insn *rtx_first, dump_flags_t flags) if (flags & TDF_DETAILS) df_dump_insn_bottom (tmp_rtx, outf); - if (flags & TDF_BLOCKS) + bb = end[INSN_UID (tmp_rtx)]; + if (bb != NULL) { - bb = end[INSN_UID (tmp_rtx)]; - if (bb != NULL) + if (flags & TDF_BLOCKS) { dump_bb_info (outf, bb, 0, dump_flags, false, true); if (df && (flags & TDF_DETAILS)) df_dump_bottom (bb, outf); putc ('\n', outf); } + /* Emit a hint if the fallthrough target of current basic block + isn't the one placed right next. */ + else if (EDGE_COUNT (bb->succs) > 0) + { + gcc_assert (BB_END (bb) == tmp_rtx); + const rtx_insn *ninsn = NEXT_INSN (tmp_rtx); + edge e = find_fallthru_edge (bb->succs); + if (e && ninsn) + { + basic_block dest = e->dest; + if (start[INSN_UID (ninsn)] != dest) + fprintf (outf, "%s ; pc falls through to BB %d\n", + print_rtx_head, dest->index); + } + } } }
Kewen.Lin <linkw@linux.ibm.com> writes: > Hi Richard, > > on 2019/7/11 上午3:30, Richard Sandiford wrote: >> "Kewen.Lin" <linkw@linux.ibm.com> writes: >>> Hi all, >>> Is it a reasonable patch? If yes, is it ok for trunk? >> >> It looks really useful, but IMO we should either emit the hint for all >> end-of-block insns with a surprising fallthrough edge, or -- if we >> really do want to keep it specific to conditional jumps -- we should >> replace rhs "pc" in the jump insn with something like "bb <N>". >> Personally the former seems better to me (and should also be simpler). >> > > Thanks a lot for the comments. It's a good idea not only to check conditional > jump insn. I've updated the patch accordingly. I was intended to change the > pc dumping by using the actual BB label similar to what we see for non > fallthrough edge. But as you mentioned it's not simple, need to pass some > information down, it looks like a hint is enough for this case. :) > >>> + basic_block dest = e->dest; >>> + if (BB_HEAD (dest) != ninsn) >>> + fprintf (outf, ";; pc falls through to BB %d\n", dest->index); >> >> Very minor, but I think the output would be more readable if the comment >> were indented by the same amount as the register notes (like REG_DEAD above). >> In lisp tradition I guess the comment marker would then be ";" rather >> than ";;". >> > > Good idea, also updated it accordingly, thanks! With the attached patch, the > dumping is as below: > > 12: r135:CC=cmp(r122:DI,0) > 13: pc={(r135:CC!=0)?L52:pc} > REG_DEAD r135:CC > REG_BR_PROB 1041558836 > ; pc falls through to BB 10 > 31: L31: > 17: NOTE_INSN_BASIC_BLOCK 3 > > > Thanks, > Kewen > > > -------------- > > gcc/ChangeLog > > 2019-07-11 Kewen Lin <linkw@gcc.gnu.org> > > * gcc/cfgrtl.c (print_rtl_with_bb): Emit a hint if the fallthrough > target of current basic block isn't the placed right next. > > > diff --git a/gcc/cfgrtl.c b/gcc/cfgrtl.c > index a1ca5992c41..5f2accf1f4f 100644 > --- a/gcc/cfgrtl.c > +++ b/gcc/cfgrtl.c > @@ -2193,14 +2193,14 @@ print_rtl_with_bb (FILE *outf, const rtx_insn *rtx_first, dump_flags_t flags) > if (df) > df_dump_start (outf); > > - if (flags & TDF_BLOCKS) I think we still need an if here, but with the condition instead being: cfun->curr_properties & PROP_cfg > + FOR_EACH_BB_REVERSE_FN (bb, cfun) > { > - FOR_EACH_BB_REVERSE_FN (bb, cfun) > - { > - rtx_insn *x; > + rtx_insn *x; > > - start[INSN_UID (BB_HEAD (bb))] = bb; > - end[INSN_UID (BB_END (bb))] = bb; > + start[INSN_UID (BB_HEAD (bb))] = bb; > + end[INSN_UID (BB_END (bb))] = bb; > + if (flags & TDF_BLOCKS) > + { > for (x = BB_HEAD (bb); x != NULL_RTX; x = NEXT_INSN (x)) > { > enum bb_state state = IN_MULTIPLE_BB; > @@ -2244,16 +2244,31 @@ print_rtl_with_bb (FILE *outf, const rtx_insn *rtx_first, dump_flags_t flags) > if (flags & TDF_DETAILS) > df_dump_insn_bottom (tmp_rtx, outf); > > - if (flags & TDF_BLOCKS) > + bb = end[INSN_UID (tmp_rtx)]; > + if (bb != NULL) > { > - bb = end[INSN_UID (tmp_rtx)]; > - if (bb != NULL) > + if (flags & TDF_BLOCKS) > { > dump_bb_info (outf, bb, 0, dump_flags, false, true); > if (df && (flags & TDF_DETAILS)) > df_dump_bottom (bb, outf); > putc ('\n', outf); > } > + /* Emit a hint if the fallthrough target of current basic block > + isn't the one placed right next. */ > + else if (EDGE_COUNT (bb->succs) > 0) > + { > + gcc_assert (BB_END (bb) == tmp_rtx); > + const rtx_insn *ninsn = NEXT_INSN (tmp_rtx); I think it'd be better to loop until we hit a real insn or a nonnull start[], e.g. to cope with intervening deleted-insn notes and debug insns. Something like: while (ninsn && !INSN_P (nisn) && !start[INSN_UID (ninsn)]) ninsn = NEXT_INSN (insn); Looks good otherwise, thanks. Richard > + edge e = find_fallthru_edge (bb->succs); > + if (e && ninsn) > + { > + basic_block dest = e->dest; > + if (start[INSN_UID (ninsn)] != dest) > + fprintf (outf, "%s ; pc falls through to BB %d\n", > + print_rtx_head, dest->index); > + } > + } > } > }
Hi Richard, on 2019/7/11 下午4:51, Richard Sandiford wrote: > Kewen.Lin <linkw@linux.ibm.com> writes: >> >> - if (flags & TDF_BLOCKS) > > I think we still need an if here, but with the condition instead being: > > cfun->curr_properties & PROP_cfg > >> + else if (EDGE_COUNT (bb->succs) > 0) >> + { >> + gcc_assert (BB_END (bb) == tmp_rtx); >> + const rtx_insn *ninsn = NEXT_INSN (tmp_rtx); > > I think it'd be better to loop until we hit a real insn or a nonnull > start[], e.g. to cope with intervening deleted-insn notes and debug insns. > Something like: > > while (ninsn && !INSN_P (nisn) && !start[INSN_UID (ninsn)]) > ninsn = NEXT_INSN (insn); > Thanks a lot for the comments, I've merged your suggested codes as below. Regression testing is ongoing on powerpc64le-unknown-linux-gnu. If regtest and bootstrap is ok, is it ok for trunk? Thanks, Kewen --------- gcc/ChangeLog 2019-07-11 Kewen Lin <linkw@gcc.gnu.org> * gcc/cfgrtl.c (print_rtl_with_bb): Emit a hint if the fallthrough target of current basic block isn't the placed right next. diff --git a/gcc/cfgrtl.c b/gcc/cfgrtl.c index a1ca5992c41..19b118a8ece 100644 --- a/gcc/cfgrtl.c +++ b/gcc/cfgrtl.c @@ -2193,7 +2193,7 @@ print_rtl_with_bb (FILE *outf, const rtx_insn *rtx_first, dump_flags_t flags) if (df) df_dump_start (outf); - if (flags & TDF_BLOCKS) + if (cfun->curr_properties & PROP_cfg) { FOR_EACH_BB_REVERSE_FN (bb, cfun) { @@ -2201,16 +2201,19 @@ print_rtl_with_bb (FILE *outf, const rtx_insn *rtx_first, dump_flags_t flags) start[INSN_UID (BB_HEAD (bb))] = bb; end[INSN_UID (BB_END (bb))] = bb; - for (x = BB_HEAD (bb); x != NULL_RTX; x = NEXT_INSN (x)) + if (flags & TDF_BLOCKS) { - enum bb_state state = IN_MULTIPLE_BB; + for (x = BB_HEAD (bb); x != NULL_RTX; x = NEXT_INSN (x)) + { + enum bb_state state = IN_MULTIPLE_BB; - if (in_bb_p[INSN_UID (x)] == NOT_IN_BB) - state = IN_ONE_BB; - in_bb_p[INSN_UID (x)] = state; + if (in_bb_p[INSN_UID (x)] == NOT_IN_BB) + state = IN_ONE_BB; + in_bb_p[INSN_UID (x)] = state; - if (x == BB_END (bb)) - break; + if (x == BB_END (bb)) + break; + } } } } @@ -2244,16 +2247,35 @@ print_rtl_with_bb (FILE *outf, const rtx_insn *rtx_first, dump_flags_t flags) if (flags & TDF_DETAILS) df_dump_insn_bottom (tmp_rtx, outf); - if (flags & TDF_BLOCKS) + bb = end[INSN_UID (tmp_rtx)]; + if (bb != NULL) { - bb = end[INSN_UID (tmp_rtx)]; - if (bb != NULL) + if (flags & TDF_BLOCKS) { dump_bb_info (outf, bb, 0, dump_flags, false, true); if (df && (flags & TDF_DETAILS)) df_dump_bottom (bb, outf); putc ('\n', outf); } + /* Emit a hint if the fallthrough target of current basic block + isn't the one placed right next. */ + else if (EDGE_COUNT (bb->succs) > 0) + { + gcc_assert (BB_END (bb) == tmp_rtx); + const rtx_insn *ninsn = NEXT_INSN (tmp_rtx); + /* Bypass intervening deleted-insn notes and debug insns. */ + while (ninsn && !NONDEBUG_INSN_P (ninsn) + && !start[INSN_UID (ninsn)]) + ninsn = NEXT_INSN (ninsn); + edge e = find_fallthru_edge (bb->succs); + if (e && ninsn) + { + basic_block dest = e->dest; + if (start[INSN_UID (ninsn)] != dest) + fprintf (outf, "%s ; pc falls through to BB %d\n", + print_rtx_head, dest->index); + } + } } }
Kewen.Lin <linkw@linux.ibm.com> writes: > + /* Emit a hint if the fallthrough target of current basic block > + isn't the one placed right next. */ > + else if (EDGE_COUNT (bb->succs) > 0) > + { > + gcc_assert (BB_END (bb) == tmp_rtx); > + const rtx_insn *ninsn = NEXT_INSN (tmp_rtx); > + /* Bypass intervening deleted-insn notes and debug insns. */ > + while (ninsn && !NONDEBUG_INSN_P (ninsn) > + && !start[INSN_UID (ninsn)]) Just a cosmetic thing, but when the full expression needs to be split over several lines, there should be one condition per line: while (ninsn && !NONDEBUG_INSN_P (ninsn) && !start[INSN_UID (ninsn)]) OK with that change, thanks. Richard > + ninsn = NEXT_INSN (ninsn); > + edge e = find_fallthru_edge (bb->succs); > + if (e && ninsn) > + { > + basic_block dest = e->dest; > + if (start[INSN_UID (ninsn)] != dest) > + fprintf (outf, "%s ; pc falls through to BB %d\n", > + print_rtx_head, dest->index); > + } > + } > } > }
on 2019/7/11 下午9:55, Richard Sandiford wrote: > Kewen.Lin <linkw@linux.ibm.com> writes: >> + /* Emit a hint if the fallthrough target of current basic block >> + isn't the one placed right next. */ >> + else if (EDGE_COUNT (bb->succs) > 0) >> + { >> + gcc_assert (BB_END (bb) == tmp_rtx); >> + const rtx_insn *ninsn = NEXT_INSN (tmp_rtx); >> + /* Bypass intervening deleted-insn notes and debug insns. */ >> + while (ninsn && !NONDEBUG_INSN_P (ninsn) >> + && !start[INSN_UID (ninsn)]) > > Just a cosmetic thing, but when the full expression needs to be split > over several lines, there should be one condition per line: > > while (ninsn > && !NONDEBUG_INSN_P (ninsn) > && !start[INSN_UID (ninsn)]) > > OK with that change, thanks. > Thanks Richard! Regression tested and bootstrapped on powerpc64le-unknown-linux-gnu. Committed as r273430 with above change. Kewen
diff --git a/gcc/cfgrtl.c b/gcc/cfgrtl.c index a1ca5992c41..928b9b0f691 100644 --- a/gcc/cfgrtl.c +++ b/gcc/cfgrtl.c @@ -2164,7 +2164,26 @@ rtl_dump_bb (FILE *outf, basic_block bb, int indent, dump_flags_t flags) } } - + +/* For dumping without specifying basic blocks option, when we see PC is one of + jump targets, it's easy to misunderstand the next basic block is the + fallthrough one, but it's not so true sometimes. This function is to dump + hints for the case where basic block of next insn isn't the fall through + target. */ + +static void +hint_if_pc_fall_through_not_next (FILE *outf, basic_block bb) +{ + gcc_assert (outf); + gcc_assert (EDGE_COUNT (bb->succs) >= 2); + const rtx_insn *einsn = BB_END (bb); + const rtx_insn *ninsn = NEXT_INSN (einsn); + edge e = FALLTHRU_EDGE (bb); + basic_block dest = e->dest; + if (BB_HEAD (dest) != ninsn) + fprintf (outf, ";; pc falls through to BB %d\n", dest->index); +} + /* Like dump_function_to_file, but for RTL. Print out dataflow information for the start of each basic block. FLAGS are the TDF_* masks documented in dumpfile.h. */ @@ -2255,6 +2274,14 @@ print_rtl_with_bb (FILE *outf, const rtx_insn *rtx_first, dump_flags_t flags) putc ('\n', outf); } } + else if (GET_CODE (tmp_rtx) == JUMP_INSN + && GET_CODE (PATTERN (tmp_rtx)) == SET) + { + bb = BLOCK_FOR_INSN (tmp_rtx); + const_rtx src = SET_SRC (PATTERN (tmp_rtx)); + if (bb != NULL && GET_CODE (src) == IF_THEN_ELSE) + hint_if_pc_fall_through_not_next (outf, bb); + } } free (start);
Hi all, 6: NOTE_INSN_BASIC_BLOCK 2 .... 12: r135:CC=cmp(r122:DI,0) 13: pc={(r135:CC!=0)?L52:pc} REG_DEAD r135:CC REG_BR_PROB 1041558836 31: L31: 17: NOTE_INSN_BASIC_BLOCK 3 The above RTL sequence is from pass doloop dumping with -fdump-rtl-all-slim, I misunderstood that: the fall through BB of BB 2 is BB 3, since BB 3 is placed just next to BB 2. Then I found the contradiction that BB 3 will have some uninitialized regs if it's true. I can get the exact information with "-blocks" dumping and even detailed one with "-details". But I'm thinking whether it's worth to giving some information on "-slim" dump (or more exactly without "-blocks") to avoid some confusion especially for new comers like me. This patch is to add one line to hint what's the fallthrough BB if it's the one closely sitting, for example: 6: NOTE_INSN_BASIC_BLOCK 2 .... 12: r135:CC=cmp(r122:DI,0) 13: pc={(r135:CC!=0)?L52:pc} REG_DEAD r135:CC REG_BR_PROB 1041558836 ;; pc falls through to BB 10 31: L31: 17: NOTE_INSN_BASIC_BLOCK 3 Bootstrapped and regression test passed on powerpc64le-unknown-linux-gnu. Is it a reasonable patch? If yes, is it ok for trunk? Thanks, Kewen --------- gcc/ChangeLog 2019-07-08 Kewen Lin <linkw@gcc.gnu.org> * gcc/cfgrtl.c (print_rtl_with_bb): Check and call hint_if_pc_fall_through_not_next for jump insn with two successors. (hint_if_pc_fall_through_not_next): New function.