Jakub Jelinek <ja...@redhat.com> writes:
> Hi!
>
> My r16-1398 PR120434 ranger during expansion change broke profiled lto
> bootstrap on x86_64-linux, the following testcase is reduced from that.
>
> The problem is during expand_gimple_cond, if we are unlucky that neither
> of edge_true and edge_false point to the next basic block, the code
> effectively attempts to split the false_edge and make the new bb BB_RTL
> with some extra instructions which just arranges to jump.
> It does it by creating a new bb, redirecting the false_edge and then
> creating a new edge from the new bb to the dest.
> Note, we don't have GIMPLE cfg hooks installed anymore and even if we
> would, the 3 calls aren't the same as one split_edge with transformation
> of the new bb into BB_RTL and adding it some BB_HEAD/BB_END. If
> false_edge->dest is BB_RTL or doesn't have PHI nodes (which before my
> patch was always the case because), then this works fine, but with
> PHI nodes on false_edge->dest redirect_edge_succ will remove the false_edge
> from dest->preds (unordered remove which moves into its place the last edge
> in the vector) and the new make_edge will then add the new edge as last
> in the vector. So, unless false_edge is the last edge in the dest->preds
> vector this effectively swaps the last edge in the vector with
> false_edge/its new replacement.
> gimple_split_edge solves this by temporarily clearing phi_nodes on dest
> (not needed when we don't have GIMPLE hooks), then making the new edge
> first and redirecting the old edge (plus restoring phi_nodes on dest).
> That way the redirection replaces the old edge with the new one and
> PHI arguments don't need adjustment. At the cost of temporarily needing
> one more edge in the vector and so if unlucky reallocation.
> Doing it like that is one of the options (i.e. just move the
> make_single_succ_edge call). This patch instead keeps doing what it did
> and just swaps two edges again if needed to restore the PHI behavior
> - remember edge_false->dest_idx first if there are PHI nodes in
> edge_false->dest and afterwards if new edge's dest_idx is different from
> the remembered one, swap the new edge with EDGE_PRED (dest, old_dest_idx).
> That way PHI arguments are maintained properly as well. Without this
> we sometimes just swap PHI arguments.
>
> Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?
>
> 2025-06-12 Jakub Jelinek <ja...@redhat.com>
>
> PR middle-end/120629
> * cfgexpand.cc (expand_gimple_cond): If dest bb 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.
>
> * g++.dg/opt/pr120629.C: New test.
This isn't really my area, but Jakub helped me with some of the details
off-list. And given the description of the problem, the fix seems
natural enough, even if we later decide to do things differently.
So LGTM. Very minor, but:
>
> --- gcc/cfgexpand.cc.jj 2025-06-11 19:28:52.462056696 +0200
> +++ gcc/cfgexpand.cc 2025-06-12 00:05:27.524152553 +0200
> @@ -3013,6 +3013,9 @@ expand_gimple_cond (basic_block bb, gcon
>
> new_bb = create_basic_block (NEXT_INSN (last), get_last_insn (), bb);
> dest = false_edge->dest;
> + unsigned int dest_idx = 0;
> + if ((dest->flags & BB_RTL) == 0 && phi_nodes (dest))
> + dest_idx = false_edge->dest_idx;
There didn't seem to be any particular need to keep this conditional.
OK from my POV either way though.
Thanks,
Richard
> redirect_edge_succ (false_edge, new_bb);
> false_edge->flags |= EDGE_FALLTHRU;
> new_bb->count = false_edge->count ();
> @@ -3021,7 +3024,19 @@ expand_gimple_cond (basic_block bb, gcon
> if (loop->latch == bb
> && loop->header == dest)
> loop->latch = new_bb;
> - make_single_succ_edge (new_bb, dest, 0);
> + edge e = make_single_succ_edge (new_bb, dest, 0);
> + if ((dest->flags & BB_RTL) == 0
> + && phi_nodes (dest)
> + && 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 (dest, dest_idx);
> + std::swap (e->dest_idx, e2->dest_idx);
> + std::swap (EDGE_PRED (dest, e->dest_idx),
> + EDGE_PRED (dest, e2->dest_idx));
> + }
> if (BARRIER_P (BB_END (new_bb)))
> BB_END (new_bb) = PREV_INSN (BB_END (new_bb));
> update_bb_for_insn (new_bb);
> --- gcc/testsuite/g++.dg/opt/pr120629.C.jj 2025-06-12 00:13:02.928211946
> +0200
> +++ gcc/testsuite/g++.dg/opt/pr120629.C 2025-06-12 00:14:26.008117524
> +0200
> @@ -0,0 +1,53 @@
> +// PR middle-end/120629
> +// { dg-do run }
> +// { dg-options "-O2 -fprofile-generate -fno-exceptions -fno-rtti" }
> +// { dg-require-profiling "-fprofile-generate" }
> +
> +__attribute__((noipa, noreturn, cold)) void
> +foo (const char *, int, const char *)
> +{
> + __builtin_abort ();
> +}
> +
> +struct S
> +{
> + __attribute__((noipa)) void bar (void *);
> + static const int a = 8;
> + unsigned int b[a + 1];
> +};
> +
> +__attribute__((noipa)) unsigned long
> +baz (void *)
> +{
> + static int x = 8;
> + return --x;
> +}
> +
> +__attribute__((noipa)) void
> +S::bar (void *x)
> +{
> + unsigned int c;
> + int k = 0;
> +
> + do
> + {
> + ((void) (!(k <= a) ? foo ("foo", 42, __FUNCTION__), 0 : 0));
> + c = b[k++] = baz (x);
> + }
> + while (c);
> + while (k <= a)
> + b[k++] = 0;
> +}
> +
> +int
> +main ()
> +{
> + struct T { S a; unsigned int b; } s = {};
> + s.b = 0x1234;
> + s.a.bar (0);
> + for (int i = 0; i < 9; ++i)
> + if (s.a.b[i] != (i == 8 ? 0 : 7 - i))
> + __builtin_abort ();
> + if (s.b != 0x1234)
> + __builtin_abort ();
> +}
>
> Jakub
