On Tue, Apr 1, 2025 at 6:10 AM Andrew Pinski <quic_apin...@quicinc.com> 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 <quic_apin...@quicinc.com>
> ---
> 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;
> /* 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
>
