"Kewen.Lin" <li...@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 <li...@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);
