Jakub Jelinek <ja...@redhat.com> writes:
> Hi!
>
> Andrew ran some extra ranger checking during bootstrap and found one more
> case (though much rarer than the GIMPLE_COND case).
>
> Seems on fold-const.cc (native_encode_expr) we end up with bb 2, ENTRY
> bb successor, having PHI nodes (usually there is some bb in between, even if
> empty, in the native_encode_expr it is tail recursion but haven't managed
> to construct a test with such case by hand).
> So, we have in optimized dump
> <bb 2> [local count: 1089340384]:
> # expr_12 = PHI <expr_199(D)(0), part_93(51)>
> # ptr_13 = PHI <ptr_86(D)(0), ptr_13(51)>
> # len_14 = PHI <len_103(D)(0), _198(51)>
> # off_10 = PHI <off_102(D)(0), _207(51)>
> # add_acc_99 = PHI <0(0), add_acc_101(51)>
> where there are mostly default defs from the 0->2 edge (and one zero)
> and some other values from the other edge.
> construct_init_block inserts a BB_RTL basic block with the function start
> instructions and similarly to the GIMPLE_COND case it wants to insert that
> bb on the edge from ENTRY to its single successor.
> Now, without this patch redirect_edge_succ redirects the 0->2 edge to 0->52,
> so the 51->2 edge gets moved first by unordered_remove, and
> make_single_succ_edge adds a new 52->2 edge. So we end up with
> # expr_12 = PHI <expr_199(D)(51), part_93(52)>
> # ptr_13 = PHI <ptr_86(D)(51), ptr_13(52)>
> # len_14 = PHI <len_103(D)(51), _198(52)>
> # off_10 = PHI <off_102(D)(51), _207(52)>
> # add_acc_99 = PHI <0(51), add_acc_101(52)>
> which is not correct, the default definitions and zero are now from the edge
> from end of function and the other values from the edge from the new BB_RTL
> successor of ENTRY. With this patch we get
> # expr_12 = PHI <expr_199(D)(52), part_93(51)>
> # ptr_13 = PHI <ptr_86(D)(52), ptr_13(51)>
> # len_14 = PHI <len_103(D)(52), _198(51)>
> # off_10 = PHI <off_102(D)(52), _207(51)>
> # add_acc_99 = PHI <0(52), add_acc_101(51)>
> instead.
>
> Starting bootstrap/regtest now, ok for trunk if it passes on x86_64-linux
> and i686-linux?
>
> 2025-06-12 Jakub Jelinek <ja...@redhat.com>
>
> * cfgexpand.cc (construct_init_block): If first_block isn't BB_RTL,
> has any PHI nodes and false_edge->dest_idx before redirection is
> different from make_single_succ_edge result's dest_idx, swap the
> latter with the former last pred edge and their dest_idx members.
OK, thanks. But now that there are two instances, I wonder if it would
be worth hiding this detail in a helper function?
It looks like the remaining direct use of redirect_edge_succ and
make_single_succ_edge is in construct_exit_block, but I assume that
wouldn't be a problem because the successor is always the exit block.
Richard
>
> --- gcc/cfgexpand.cc.jj 2025-06-12 15:51:37.584210697 +0200
> +++ gcc/cfgexpand.cc 2025-06-12 17:46:13.946266774 +0200
> @@ -6570,9 +6570,22 @@ construct_init_block (void)
> add_bb_to_loop (init_block, ENTRY_BLOCK_PTR_FOR_FN (cfun)->loop_father);
> if (e)
> {
> + unsigned int dest_idx = e->dest_idx;
> first_block = e->dest;
> redirect_edge_succ (e, init_block);
> - make_single_succ_edge (init_block, first_block, flags);
> + e = make_single_succ_edge (init_block, first_block, flags);
> + if ((first_block->flags & BB_RTL) == 0
> + && phi_nodes (first_block)
> + && e->dest_idx != dest_idx)
> + {
> + /* If there are any PHI nodes on dest, swap the new succ edge
> + with the one moved into false_edge's former position, so that
> + PHI arguments don't need adjustment. */
> + edge e2 = EDGE_PRED (first_block, dest_idx);
> + std::swap (e->dest_idx, e2->dest_idx);
> + std::swap (EDGE_PRED (first_block, e->dest_idx),
> + EDGE_PRED (first_block, e2->dest_idx));
> + }
> }
> else
> make_single_succ_edge (init_block, EXIT_BLOCK_PTR_FOR_FN (cfun),
>
>
> Jakub
