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.
--- 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;
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
