Segher Boessenkool <seg...@kernel.crashing.org> writes:
> When the compgotos pass copies the tail of blocks ending in an indirect
> jump, there is a micro-optimization to not copy the last one, since the
> original block will then just be deleted. This does not work properly
> if cleanup_cfg does not merge all pairs of blocks we expect it to.
>
>
> v2: This also deletes the other use of single_pred_p, which has the same
> problem in principle, I just never have triggered it so far.
Could you go into more details? I thought the reason you wanted to
remove the test in the loop was that each successful iteration of the
loop removes one incoming edge, meaning that if all previous iterations
succeed, the final iteration is bound to see a single predecessor.
And there's no guarantee in that case that the jump in the final
block will be removed. I.e. the loop is unnecessarily leaving
cfgcleanup to finish off the last step for it. So I agree that the
loop part looks good. (I assume that was v1, but I missed it, sorry.)
But it looks like the point of the initial test is to avoid “duplicating”
the block if there would still only be one copy of the code. That test
still seems reasonable since the case it rejects should be handled by
other cfg cleanups. If for some reason it isn't, there doesn't seem
to be any reason to apply the max_size limit for a single predecessor,
since no code duplication will take place. So ISTM that removing the
first condition is really a separate thing that should be its own patch.
Thanks,
Richard
>
> Tested on powerpc64-linux {-m32,-m64} like before. Is this okay for
> trunk?
>
>
> Segher
>
>
> 2020-08-07 Segher Boessenkool <seg...@kernel.crashing.org>
>
> PR rtl-optimization/96475
> * bb-reorder.c (maybe_duplicate_computed_goto): Remove single_pred_p
> micro-optimization.
> ---
> gcc/bb-reorder.c | 10 +++-------
> 1 file changed, 3 insertions(+), 7 deletions(-)
>
> diff --git a/gcc/bb-reorder.c b/gcc/bb-reorder.c
> index c635010..76e56b5 100644
> --- a/gcc/bb-reorder.c
> +++ b/gcc/bb-reorder.c
> @@ -2680,9 +2680,6 @@ make_pass_reorder_blocks (gcc::context *ctxt)
> static bool
> maybe_duplicate_computed_goto (basic_block bb, int max_size)
> {
> - if (single_pred_p (bb))
> - return false;
> -
> /* Make sure that the block is small enough. */
> rtx_insn *insn;
> FOR_BB_INSNS (bb, insn)
> @@ -2700,10 +2697,9 @@ maybe_duplicate_computed_goto (basic_block bb, int
> max_size)
> {
> basic_block pred = e->src;
>
> - /* Do not duplicate BB into PRED if that is the last predecessor, or if
> - we cannot merge a copy of BB with PRED. */
> - if (single_pred_p (bb)
> - || !single_succ_p (pred)
> + /* Do not duplicate BB into PRED if we cannot merge a copy of BB
> + with PRED. */
> + if (!single_succ_p (pred)
> || e->flags & EDGE_COMPLEX
> || pred->index < NUM_FIXED_BLOCKS
> || (JUMP_P (BB_END (pred)) && !simplejump_p (BB_END (pred)))
