On Thu, Nov 20, 2025 at 8:00 PM Andrew Pinski <[email protected]> wrote: > > On Tue, Apr 1, 2025 at 12:54 AM Richard Biener > <[email protected]> wrote: > > > > On Tue, Apr 1, 2025 at 6:10 AM Andrew Pinski <[email protected]> > > wrote: > > > > > > phiprop can sometimes prop loads back into loops > > > and in some cases cause wrong code when the load > > > was from a weak symbol as now it becomes an unconditional > > > load before the loop. > > > > > > Bootstrapped and tested on x86_64-linux-gnu with no regressions. > > > > > > PR tree-optimization/116835 > > > > > > gcc/ChangeLog: > > > > > > * tree-ssa-phiprop.cc (propagate_with_phi): Check > > > the use is at the same or deeper loop depth than > > > the phi node. > > > (pass_phiprop::execute): Initialize loops. > > > > > > gcc/testsuite/ChangeLog: > > > > > > * gcc.dg/torture/pr116835.c: New test. > > > > > > Signed-off-by: Andrew Pinski <[email protected]> > > > --- > > > gcc/testsuite/gcc.dg/torture/pr116835.c | 33 +++++++++++++++++++++++++ > > > gcc/tree-ssa-phiprop.cc | 14 ++++++++++- > > > 2 files changed, 46 insertions(+), 1 deletion(-) > > > create mode 100644 gcc/testsuite/gcc.dg/torture/pr116835.c > > > > > > diff --git a/gcc/testsuite/gcc.dg/torture/pr116835.c > > > b/gcc/testsuite/gcc.dg/torture/pr116835.c > > > new file mode 100644 > > > index 00000000000..31d3b59d945 > > > --- /dev/null > > > +++ b/gcc/testsuite/gcc.dg/torture/pr116835.c > > > @@ -0,0 +1,33 @@ > > > +/* { dg-do run { target { weak_undefined } } } */ > > > +/* { dg-add-options weak_undefined } */ > > > + > > > +/* PR tree-optimization/116835 */ > > > +/* phiprop would prop into the loop the load of b > > > + and also prop the load of a before the loop. > > > + Which is incorrect as a is a weak symbol. */ > > > + > > > +struct s1 > > > +{ > > > + int t; > > > + int t1; > > > +}; > > > +typedef struct s1 type; > > > +extern type a __attribute__((weak)); > > > +int t; > > > +type b; > > > +type bar (int c) __attribute__((noipa, noinline)); > > > +type > > > +bar (int c) > > > +{ > > > + t = 1; > > > + type *p = &a; > > > + for (int j = 0; j < c; ++j) > > > + p = &b; > > > + return *p; > > > +} > > > + > > > +int main(void) > > > +{ > > > + if (bar(&a == nullptr).t) > > > + __builtin_abort(); > > > +} > > > diff --git a/gcc/tree-ssa-phiprop.cc b/gcc/tree-ssa-phiprop.cc > > > index a2e1fb16a30..c5fe231d3e9 100644 > > > --- a/gcc/tree-ssa-phiprop.cc > > > +++ b/gcc/tree-ssa-phiprop.cc > > > @@ -35,6 +35,7 @@ along with GCC; see the file COPYING3. If not see > > > #include "tree-ssa-loop.h" > > > #include "tree-cfg.h" > > > #include "tree-ssa-dce.h" > > > +#include "cfgloop.h" > > > > > > /* This pass propagates indirect loads through the PHI node for its > > > address to make the load source possibly non-addressable and to > > > @@ -348,11 +349,17 @@ propagate_with_phi (basic_block bb, gphi *phi, > > > struct phiprop_d *phivn, > > > calculate_dominance_info (CDI_POST_DOMINATORS); > > > > > > /* Only replace loads in blocks that post-dominate the PHI node. > > > That > > > - makes sure we don't end up speculating loads. */ > > > + makes sure we don't end up speculating loads. */ > > > if (!dominated_by_p (CDI_POST_DOMINATORS, > > > bb, gimple_bb (use_stmt))) > > > continue; > > > > I think the testcase shows the above test is not sufficient to avoid doing > > invalid speculation. We need to verify the assumption that the > > PHI is dereferenced each time it is "executed". > > With cycles SSA form and post-dominance are not helpful here. > > > > I think we have to amend the post-dominance check to deal with > > SSA cycles, but your check does not look fully correct (and its > > comment is misleading). > > We need to constrain the load to be in the same or a subloop (use > > flow_loop_nested_p, > > not loop depth) or in the same BB when either the load or the PHI is > > in an irreducible region. > > > > > > > > + /* Only replace loads in blocks are in the same loop > > > + are inside an deeper loop. This is to make sure not > > > + to prop back into the loop. */ > > > + if (bb_loop_depth (gimple_bb (use_stmt)) < bb_loop_depth (bb)) > > > + continue; > > > + > > > > So something like > > > > /* Amend the post-dominance check for SSA cycles, we need to > > make sure each PHI result value is dereferenced. */ > > if (!(gimple_bb (use_stmt) == bb > > || (!(bb->flags & BB_IRREDUCIBLE_LOOP) > > && !(gimple_bb (use_stmt)->flags & BB_IRREDUCIBLE_LOOP) > > && (bb->loop_father == gimple_bb (use_stmt)->loop_father > > || flow_loop_nested_p (bb->loop_father, gimple_bb > > (use_stmt)->loop_father))))) > > continue; > > That works except now phiprop only does the following partly: > ``` > int a; > > int b; > int bar (int c) __attribute__((noipa, noinline)); > int > bar (int c) > { > int *p = &a; > for (int j = 0; j < *p; ++j) > { > p = &b; > } > return *p; > } > ``` > So maybe the analysis part needs to be separated from the insert part ... > Let me try that.
I don't need a full disconnect. Just cause add a simple delay list for these and if we do an insert of the phi then reuse it in that case. I will send out a full patch tomorrow. Thanks, Andrew > > Thanks, > Andrew > > > > > > /* Check whether this is a load of *ptr. */ > > > if (!(is_gimple_assign (use_stmt) > > > && gimple_assign_rhs_code (use_stmt) == MEM_REF > > > @@ -520,6 +527,10 @@ pass_phiprop::execute (function *fun) > > > size_t n; > > > auto_bitmap dce_ssa_names; > > > > > > + /* Find the loops, so that we can prevent moving loads in > > > + them. */ > > > + loop_optimizer_init (AVOID_CFG_MODIFICATIONS); > > > + > > > calculate_dominance_info (CDI_DOMINATORS); > > > > > > n = num_ssa_names; > > > @@ -548,6 +559,7 @@ pass_phiprop::execute (function *fun) > > > free (phivn); > > > > > > free_dominance_info (CDI_POST_DOMINATORS); > > > + loop_optimizer_finalize (); > > > > > > return did_something ? TODO_update_ssa_only_virtuals : 0; > > > } > > > -- > > > 2.43.0 > > >
