Re: [PATCH] Clean up loop-closed PHIs at loopdone pass

[Jiufu Guo <guojiufu@linux.ibm.com>]

On 2020-11-05 21:43, Richard Biener wrote:

Hi Richard,

Thanks for your comments and suggestions!

> On Thu, Nov 5, 2020 at 2:19 PM guojiufu via Gcc-patches
> <gcc-patches@gcc.gnu.org> wrote:
>> 
>> In PR87473, there are discussions about loop-closed PHIs which
>> are generated for loop optimization passes.  It would be helpful
>> to clean them up after loop optimization is done, then this may
>> simplify some jobs of following passes.
>> This patch introduces a cheaper way to propagate them out in
>> pass_tree_loop_done.
>> 
>> This patch passes bootstrap and regtest on ppc64le.  Is this ok for 
>> trunk?
> 
> Huh, I think this is somewhat useless work, the PHIs won't survive for 
> long
> and you certainly cannot expect degenerate PHIs to not occur anyway.

After `loopdone` pass, those loop-closed-PHIs will still live ~10 passes
(veclower, switchlower, slsr...) till the next `copyprop` pass.
It would be helpful to those passes if we can eliminate those 
degenerated PHIs
in a cheaper way.  As you mentioned in
https://gcc.gnu.org/legacy-ml/gcc-patches/2018-10/msg00834.html

We know vrp/dom may generate some degenerated PHIS, and then we have 
`copyprop`
was added after each vrp/dom pair to propagate out those PHIs.  Likely, 
I
think for loop-closed PHIs, we may also eliminate them once they are not 
needed.


> You probably can replace propagate_rhs_into_lhs by the
> existing replace_uses_by function.  You're walking loop exits

Yes, replace_uses_by + remove_phi_node would be a good implementation
propagate_rhs_into_lhs.


Thanks!

> after loop_optimizer_finalize () - that's wasting work.  If you want to
> avoid inconsistent state and we really want to go with this I suggest
> to instead add a flag to loop_optimizer_finalize () as to whether to
> propagate out LC PHI nodes or not and do this from within there.

Thank you for the suggestion!
You mean adding a flag and in loop_optimizer_finalize, and add code 
like:
```
if (flag_propagate_loop_closed_phi_when_loop_done)
{
   loops_state_clear (fn, LOOP_CLOSED_SSA)
   clean_up_loop_closed_phis(fn);
}
```

Is this align with your suggestions?
One concern: function loop_optimizer_finalize is called a lot of places,
while we just need to clean up loop-closed PHIs at GIMPLE loopdone pass.

Thanks again,

Jiufu Guo.

> 
> Thanks,
> Richard.
> 
>> gcc/ChangeLog
>> 2020-10-05  Jiufu Guo   <guojiufu@linux.ibm.com>
>> 
>>         * tree-ssa-loop.h (clean_up_loop_closed_phi): New declaration.
>>         * tree-ssa-loop.c (tree_ssa_loop_done): Call 
>> clean_up_loop_closed_phi.
>>         * tree-ssa-propagate.c (propagate_rhs_into_lhs): New function.
>> 
>> gcc/testsuite/ChangeLog
>> 2020-10-05  Jiufu Guo   <guojiufu@linux.ibm.com>
>> 
>>         * gcc.dg/tree-ssa/loopclosedphi.c: New test.
>> ---
>>  gcc/testsuite/gcc.dg/tree-ssa/loopclosedphi.c |  21 +++
>>  gcc/tree-ssa-loop.c                           |   1 +
>>  gcc/tree-ssa-loop.h                           |   1 +
>>  gcc/tree-ssa-propagate.c                      | 120 
>> ++++++++++++++++++
>>  4 files changed, 143 insertions(+)
>>  create mode 100644 gcc/testsuite/gcc.dg/tree-ssa/loopclosedphi.c
>> 
>> diff --git a/gcc/testsuite/gcc.dg/tree-ssa/loopclosedphi.c 
>> b/gcc/testsuite/gcc.dg/tree-ssa/loopclosedphi.c
>> new file mode 100644
>> index 00000000000..d71b757fbca
>> --- /dev/null
>> +++ b/gcc/testsuite/gcc.dg/tree-ssa/loopclosedphi.c
>> @@ -0,0 +1,21 @@
>> +/* { dg-do compile } */
>> +/* { dg-options "-O3 -fno-tree-ch -w -fdump-tree-loopdone-details" } 
>> */
>> +
>> +void
>> +t6 (int qz, int wh)
>> +{
>> +  int jl = wh;
>> +
>> +  while (1.0 * qz / wh < 1)
>> +    {
>> +      qz = wh * (wh + 2);
>> +
>> +      while (wh < 1)
>> +        jl = 0;
>> +    }
>> +
>> +  while (qz < 1)
>> +    qz = jl * wh;
>> +}
>> +
>> +/* { dg-final { scan-tree-dump-times "Replacing" 2 "loopdone"} } */
>> diff --git a/gcc/tree-ssa-loop.c b/gcc/tree-ssa-loop.c
>> index 5e8365d4e83..7d680b2f5d2 100644
>> --- a/gcc/tree-ssa-loop.c
>> +++ b/gcc/tree-ssa-loop.c
>> @@ -530,6 +530,7 @@ tree_ssa_loop_done (void)
>>    free_numbers_of_iterations_estimates (cfun);
>>    scev_finalize ();
>>    loop_optimizer_finalize ();
>> +  clean_up_loop_closed_phi (cfun);
>>    return 0;
>>  }
>> 
>> diff --git a/gcc/tree-ssa-loop.h b/gcc/tree-ssa-loop.h
>> index 9e35125e6e8..baa940b9d1e 100644
>> --- a/gcc/tree-ssa-loop.h
>> +++ b/gcc/tree-ssa-loop.h
>> @@ -67,6 +67,7 @@ public:
>>  extern bool for_each_index (tree *, bool (*) (tree, tree *, void *), 
>> void *);
>>  extern char *get_lsm_tmp_name (tree ref, unsigned n, const char 
>> *suffix = NULL);
>>  extern unsigned tree_num_loop_insns (class loop *, struct eni_weights 
>> *);
>> +extern unsigned clean_up_loop_closed_phi (function *);
>> 
>>  /* Returns the loop of the statement STMT.  */
>> 
>> diff --git a/gcc/tree-ssa-propagate.c b/gcc/tree-ssa-propagate.c
>> index 87dbf55fab9..813143852b9 100644
>> --- a/gcc/tree-ssa-propagate.c
>> +++ b/gcc/tree-ssa-propagate.c
>> @@ -1549,4 +1549,123 @@ propagate_tree_value_into_stmt 
>> (gimple_stmt_iterator *gsi, tree val)
>>    else
>>      gcc_unreachable ();
>>  }
>> +
>> +/* Propagate RHS into all uses of LHS (when possible).
>> +
>> +   RHS and LHS are derived from STMT, which is passed in solely so
>> +   that we can remove it if propagation is successful.  */
>> +
>> +static bool
>> +propagate_rhs_into_lhs (gphi *stmt, tree lhs, tree rhs)
>> +{
>> +  use_operand_p use_p;
>> +  imm_use_iterator iter;
>> +  gimple_stmt_iterator gsi;
>> +  gimple *use_stmt;
>> +  bool changed = false;
>> +  bool all = true;
>> +
>> +  /* Dump details.  */
>> +  if (dump_file && (dump_flags & TDF_DETAILS))
>> +    {
>> +      fprintf (dump_file, "  Replacing '");
>> +      print_generic_expr (dump_file, lhs, dump_flags);
>> +      fprintf (dump_file, "' with '");
>> +      print_generic_expr (dump_file, rhs, dump_flags);
>> +      fprintf (dump_file, "'\n");
>> +    }
>> +
>> +  /* Walk over every use of LHS and try to replace the use with RHS. 
>> */
>> +  FOR_EACH_IMM_USE_STMT (use_stmt, iter, lhs)
>> +    {
>> +      /* It is not safe to propagate into below stmts.  */
>> +      if (gimple_debug_bind_p (use_stmt)
>> +         || (gimple_code (use_stmt) == GIMPLE_ASM
>> +             && !may_propagate_copy_into_asm (lhs))
>> +         || (TREE_CODE (rhs) == SSA_NAME
>> +             && SSA_NAME_DEF_STMT (rhs) == use_stmt))
>> +       {
>> +         all = false;
>> +         continue;
>> +       }
>> +
>> +      /* Dump details.  */
>> +      if (dump_file && (dump_flags & TDF_DETAILS))
>> +       {
>> +         fprintf (dump_file, "    Original statement:");
>> +         print_gimple_stmt (dump_file, use_stmt, 0, dump_flags);
>> +       }
>> +
>> +      /* Propagate the RHS into this use of the LHS.  */
>> +      FOR_EACH_IMM_USE_ON_STMT (use_p, iter)
>> +       propagate_value (use_p, rhs);
>> +
>> +      /* Propagation may expose new operands to the renamer.  */
>> +      update_stmt (use_stmt);
>> +
>> +      /* If variable index is replaced with a constant, then
>> +        update the invariant flag for ADDR_EXPRs.  */
>> +      if (gimple_assign_single_p (use_stmt)
>> +         && TREE_CODE (gimple_assign_rhs1 (use_stmt)) == ADDR_EXPR)
>> +       recompute_tree_invariant_for_addr_expr (gimple_assign_rhs1 
>> (use_stmt));
>> +
>> +      /* Dump details.  */
>> +      if (dump_file && (dump_flags & TDF_DETAILS))
>> +       {
>> +         fprintf (dump_file, "    Updated statement:");
>> +         print_gimple_stmt (dump_file, use_stmt, 0, dump_flags);
>> +       }
>> +
>> +      changed = true;
>> +    }
>> +
>> +  /* Remove the degenerate PHI node.  */
>> +  if (all)
>> +    {
>> +      gsi = gsi_for_stmt (stmt);
>> +      remove_phi_node (&gsi, true);
>> +    }
>> +
>> +  return changed;
>> +}
>> +
>> +/* Check exits of each loop in FUN, walk over loop closed PHIs in
>> +   each exit basic block and propagate degenerate PHIs.  */
>> +
>> +unsigned
>> +clean_up_loop_closed_phi (function *fun)
>> +{
>> +  unsigned i;
>> +  edge e;
>> +  gphi *phi;
>> +  tree rhs;
>> +  tree lhs;
>> +  gphi_iterator gsi;
>> +  struct loop *loop;
>> +  bool cfg_altered = false;
>> +
>> +  /* Walk over loop in function.  */
>> +  FOR_EACH_LOOP_FN (fun, loop, 0)
>> +    {
>> +      /* Check each exit edge of loop.  */
>> +      auto_vec<edge> exits = get_loop_exit_edges (loop);
>> +      FOR_EACH_VEC_ELT (exits, i, e)
>> +       if (single_pred_p (e->dest))
>> +         /* Walk over loop-closed PHIs.  */
>> +         for (gsi = gsi_start_phis (e->dest); !gsi_end_p (gsi);)
>> +           {
>> +             phi = gsi.phi ();
>> +             rhs = degenerate_phi_result (phi);
>> +             lhs = gimple_phi_result (phi);
>> +
>> +             /* Advance the iterator before stmt is removed.  */
>> +             gsi_next (&gsi);
>> +
>> +             if (rhs && !virtual_operand_p (lhs)
>> +                 && may_propagate_copy (lhs, rhs))
>> +               cfg_altered |= propagate_rhs_into_lhs (phi, lhs, rhs);
>> +           }
>> +    }
>> +
>> +  return cfg_altered;
>> +}
>> --
>> 2.25.1
>>