Kewen.Lin <li...@linux.ibm.com> writes:
> Hi Richard,
>
> on 2019/7/11 上午3:30, Richard Sandiford wrote:
>> "Kewen.Lin" <li...@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 <li...@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);
> + }
> + }
> }
> }
