Re: [PATCH] Add hints for slim dumping if fallthrough bb of jump isn't next bb

[Richard Sandiford <richard dot sandiford at arm dot com>]

"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);