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