On Thu, Nov 13, 2025 at 6:10 AM Andrew Pinski <[email protected]> wrote: > > This adds support for merging forwarder blocks with phis in cleanupcfg. > This patch might seem small but that is because the previous patches were > done to build up to make it easier to add this support. > > There is still one more patch to merge remove_forwarder_block > and remove_forwarder_block_with_phi since remove_forwarder_block_with_phi > supports splitting an edge which is not supported as an option in > remove_forwarder_block. > The splitting edge option should not be enabled for cfgcleanup but only for > mergephi. > > Note r8-338-ge7d70c6c3bccb2 added always creating a preheader for loops so we > should > protect them if we have a phi node as it goes back and forth here. And both > the gimple > and RTL loop code likes to have this preheader in the case of having the same > constant > value being starting of the loop. > > explaination on testcase changes > gcc.target/i386/pr121062-1.c needed a small change because there is a basic > block > which is not duplicated so only one `movq reg, -1` is there instead of 2. > > uninit-pred-7_a.c is xfailed and filed as PR122660, some analysis in the PR > already of > the difference now. > > uninit-pred-5.C was actually a false positive because when > m_best_candidate is non-NULL, m_best_candidate_len is always initialized. > The log message on the testcase is wrong if you manually fall the path > you can notice that. With an extra jump threading after the merging of > some bbs, the false positive is now no longer happening. So change the > dg-warning to dg-bogus. > > ssa-dom-thread-7.c now jump threads 12 times in thread2 instead of 8 > > Bootstrapped and tested on x86_64-linux-gnu.
OK to this and the previous in the series. Richard. > PR tree-optimization/122493 > gcc/ChangeLog: > > * tree-cfgcleanup.cc (tree_forwarder_block_p): Change bool argument > to a must have phi and allow phis if it is false. > (remove_forwarder_block): Add support for merging of forwarder blocks > with phis. > > gcc/testsuite/ChangeLog: > > * gcc.target/i386/pr121062-1.c: Update count. > * gcc.dg/uninit-pred-7_a.c: xfail line 23. > * g++.dg/uninit-pred-5.C: Change dg-warning to dg-bogus. > * gcc.dg/tree-ssa/ssa-dom-thread-7.c: Update count of jump thread. > > Signed-off-by: Andrew Pinski <[email protected]> > --- > gcc/testsuite/g++.dg/uninit-pred-5.C | 2 +- > .../gcc.dg/tree-ssa/ssa-dom-thread-7.c | 4 +- > gcc/testsuite/gcc.dg/uninit-pred-7_a.c | 2 +- > gcc/testsuite/gcc.target/i386/pr121062-1.c | 2 +- > gcc/tree-cfgcleanup.cc | 43 +++++++++++++++---- > 5 files changed, 39 insertions(+), 14 deletions(-) > > diff --git a/gcc/testsuite/g++.dg/uninit-pred-5.C > b/gcc/testsuite/g++.dg/uninit-pred-5.C > index 8dfd9874f65..cffac6b0c1e 100644 > --- a/gcc/testsuite/g++.dg/uninit-pred-5.C > +++ b/gcc/testsuite/g++.dg/uninit-pred-5.C > @@ -41,7 +41,7 @@ public: > edit_distance_t __trans_tmp_1; > if (m_best_candidate) { > size_t candidate_len = m_best_candidate_len; > - __trans_tmp_1 = get_edit_distance_cutoff(candidate_len); // { > dg-warning "may be used uninitialized" } > + __trans_tmp_1 = get_edit_distance_cutoff(candidate_len); // { dg-bogus > "may be used uninitialized" } > } > edit_distance_t cutoff = __trans_tmp_1; > if (cutoff) > diff --git a/gcc/testsuite/gcc.dg/tree-ssa/ssa-dom-thread-7.c > b/gcc/testsuite/gcc.dg/tree-ssa/ssa-dom-thread-7.c > index 1c2cfa4fa8d..81bb7fc9d1e 100644 > --- a/gcc/testsuite/gcc.dg/tree-ssa/ssa-dom-thread-7.c > +++ b/gcc/testsuite/gcc.dg/tree-ssa/ssa-dom-thread-7.c > @@ -11,8 +11,8 @@ > to change decisions in switch expansion which in turn can expose new > jump threading opportunities. Skip the later tests on aarch64. */ > /* { dg-final { scan-tree-dump-not "Jumps threaded" "dom3" { target { ! > aarch64*-*-* } } } } */ > -/* { dg-final { scan-tree-dump "Jumps threaded: 8" "thread2" { target { ! > aarch64*-*-* } } } } */ > -/* { dg-final { scan-tree-dump "Jumps threaded: 8" "thread2" { target { > aarch64*-*-* } } } } */ > +/* { dg-final { scan-tree-dump "Jumps threaded: 12" "thread2" { target { ! > aarch64*-*-* } } } } */ > +/* { dg-final { scan-tree-dump "Jumps threaded: 12" "thread2" { target { > aarch64*-*-* } } } } */ > > enum STATE { > S0=0, > diff --git a/gcc/testsuite/gcc.dg/uninit-pred-7_a.c > b/gcc/testsuite/gcc.dg/uninit-pred-7_a.c > index c2ba2a4248d..7aaadf7def7 100644 > --- a/gcc/testsuite/gcc.dg/uninit-pred-7_a.c > +++ b/gcc/testsuite/gcc.dg/uninit-pred-7_a.c > @@ -20,7 +20,7 @@ int foo (int n, int l, int m, int r) > blah(v); /* { dg-bogus "uninitialized" "bogus warning" } */ > > if ( n ) > - blah(v); /* { dg-bogus "uninitialized" "bogus warning" } */ > + blah(v); /* { dg-bogus "uninitialized" "bogus warning" { xfail *-*-* } > } */ > > if ( l ) > blah(v); /* { dg-bogus "uninitialized" "bogus warning" } */ > diff --git a/gcc/testsuite/gcc.target/i386/pr121062-1.c > b/gcc/testsuite/gcc.target/i386/pr121062-1.c > index 799f8562c9f..c334df64cdc 100644 > --- a/gcc/testsuite/gcc.target/i386/pr121062-1.c > +++ b/gcc/testsuite/gcc.target/i386/pr121062-1.c > @@ -31,4 +31,4 @@ render_result_from_bake_h(int tx) > } > } > > -/* { dg-final { scan-assembler-times "movq\[ \\t\]+\\\$-1, %r\[a-z0-9\]+" 2 > { target { ! ia32 } } } } */ > +/* { dg-final { scan-assembler-times "movq\[ \\t\]+\\\$-1, %r\[a-z0-9\]+" 1 > { target { ! ia32 } } } } */ > diff --git a/gcc/tree-cfgcleanup.cc b/gcc/tree-cfgcleanup.cc > index 803d8599b57..636a546a3dc 100644 > --- a/gcc/tree-cfgcleanup.cc > +++ b/gcc/tree-cfgcleanup.cc > @@ -386,16 +386,13 @@ cleanup_control_flow_bb (basic_block bb) > the entry block. */ > > static bool > -tree_forwarder_block_p (basic_block bb, bool phi_wanted) > +tree_forwarder_block_p (basic_block bb, bool must_have_phis) > { > gimple_stmt_iterator gsi; > location_t locus; > > /* BB must have a single outgoing edge. */ > if (!single_succ_p (bb) > - /* If PHI_WANTED is false, BB must not have any PHI nodes. > - Otherwise, BB must have PHI nodes. */ > - || gimple_seq_empty_p (phi_nodes (bb)) == phi_wanted > /* BB may not be a predecessor of the exit block. */ > || single_succ (bb) == EXIT_BLOCK_PTR_FOR_FN (cfun) > /* Nor should this be an infinite loop. */ > @@ -404,6 +401,10 @@ tree_forwarder_block_p (basic_block bb, bool phi_wanted) > || (single_succ_edge (bb)->flags & EDGE_ABNORMAL)) > return false; > > + /* If MUST_HAVE_PHIS is true and we don't have any phis, return false. */ > + if (must_have_phis && gimple_seq_empty_p (phi_nodes (bb))) > + return false; > + > gcc_checking_assert (bb != ENTRY_BLOCK_PTR_FOR_FN (cfun)); > > locus = single_succ_edge (bb)->goto_locus; > @@ -520,8 +521,10 @@ tree_forwarder_block_p (basic_block bb, bool phi_wanted) > || loops_state_satisfies_p > (LOOPS_MAY_HAVE_MULTIPLE_LATCHES)); > } > + /* cleanup_tree_cfg_noloop just created the loop preheader, don't > + remove it if it has phis. */ > else if (bb->loop_father == loop_outer (dest->loop_father)) > - return !loops_state_satisfies_p (LOOPS_HAVE_PREHEADERS); > + return gimple_seq_empty_p (phi_nodes (bb)); > /* Always preserve other edges into loop headers that are > not simple latches or preheaders. */ > return false; > @@ -625,6 +628,7 @@ remove_forwarder_block (basic_block bb) > basic_block dest = succ->dest; > gimple *stmt; > gimple_stmt_iterator gsi, gsi_to; > + bool has_phi = !gimple_seq_empty_p (phi_nodes (bb)); > > /* If there is an abnormal edge to basic block BB, but not into > dest, problems might occur during removal of the phi node at out > @@ -636,15 +640,24 @@ remove_forwarder_block (basic_block bb) > does not like it. > > So if there is an abnormal edge to BB, proceed only if there is > - no abnormal edge to DEST and there are no phi nodes in DEST. */ > + no abnormal edge to DEST and there are no phi nodes in DEST. > + If the BB has phi, we don't want to deal with abnormal edges either. */ > if (bb_has_abnormal_pred (bb) > && (bb_has_abnormal_pred (dest) > - || !gimple_seq_empty_p (phi_nodes (dest)))) > + || !gimple_seq_empty_p (phi_nodes (dest)) > + || has_phi)) > + return false; > + > + /* When we have a phi, we have to feed into another > + basic block with PHI nodes. */ > + if (has_phi && gimple_seq_empty_p (phi_nodes (dest))) > return false; > > /* If there are phi nodes in DEST, and some of the blocks that are > predecessors of BB are also predecessors of DEST, check that the > - phi node arguments match. */ > + phi node arguments match. > + Otherwise we have to split the edge and that becomes > + a "forwarder" again. */ > if (!gimple_seq_empty_p (phi_nodes (dest))) > { > edge_iterator ei; > @@ -680,9 +693,21 @@ remove_forwarder_block (basic_block bb) > > if (s == e) > { > + /* If we merge the forwarder with phis into a loop header > + verify if we are creating another loop latch edge. > + If so, reset number of iteration information of the loop. */ > + if (has_phi > + && dest->loop_father > + && dest->loop_father->header == dest > + && dominated_by_p (CDI_DOMINATORS, e->src, dest)) > + { > + dest->loop_father->any_upper_bound = false; > + dest->loop_father->any_likely_upper_bound = false; > + free_numbers_of_iterations_estimates (dest->loop_father); > + } > /* Copy arguments for the phi nodes, since the edge was not > here before. */ > - copy_phi_arg_into_existing_phi (succ, s); > + copy_phi_arg_into_existing_phi (succ, s, has_phi); > } > } > > -- > 2.43.0 >
