Re: [PATCH 2/3] PHIOPT: Loop over calling factor_out_conditional_conversion

[Richard Biener <richard.guenther@gmail.com>]

On Sun, May 7, 2023 at 6:45 AM Andrew Pinski via Gcc-patches
<gcc-patches@gcc.gnu.org> wrote:
>
> After adding diamond shaped bb support to factor_out_conditional_conversion,
> we can get a case where we have two conversions that needs factored out
> and then would have another phiopt happen.
> An example is:
> ```
> static inline unsigned long long g(int t)
> {
>   unsigned t1 = t;
>   return t1;
> }
> unsigned long long f(int c, int d, int e)
> {
>   unsigned long long t;
>   if (c > d)
>     t = g(c);
>   else
>     t = g(d);
>   return t;
> }
> ```
> In this case we should get a MAX_EXPR in phiopt1 with two casts.
> Before this patch, we would just factor out the outer cast and then
> wait till phiopt2 to factor out the inner cast.
>
> OK? Bootstrapped and tested on x86_64-linux-gnu with no regressions.

OK.

Thanks,
Richard.

> gcc/ChangeLog:
>
>         * tree-ssa-phiopt.cc (pass_phiopt::execute): Loop
>         over factor_out_conditional_conversion.
>
> gcc/testsuite/ChangeLog:
>
>         * gcc.dg/tree-ssa/minmax-17.c: New test.
> ---
>  gcc/testsuite/gcc.dg/tree-ssa/minmax-17.c | 21 ++++++++++++++++++
>  gcc/tree-ssa-phiopt.cc                    | 27 +++++++++++++----------
>  2 files changed, 36 insertions(+), 12 deletions(-)
>  create mode 100644 gcc/testsuite/gcc.dg/tree-ssa/minmax-17.c
>
> diff --git a/gcc/testsuite/gcc.dg/tree-ssa/minmax-17.c b/gcc/testsuite/gcc.dg/tree-ssa/minmax-17.c
> new file mode 100644
> index 00000000000..bd737e6b4cb
> --- /dev/null
> +++ b/gcc/testsuite/gcc.dg/tree-ssa/minmax-17.c
> @@ -0,0 +1,21 @@
> +/* { dg-do compile } */
> +/* { dg-options "-O -fdump-tree-phiopt1-details" } */
> +
> +static inline unsigned long long g(int t)
> +{
> +  unsigned t1 = t;
> +  return t1;
> +}
> +unsigned long long test_max(int c, int d, int e)
> +{
> +  unsigned long long t;
> +  if (c > d)
> +    t = g(c);
> +  else
> +    t = g(d);
> +  return t;
> +}
> +
> +/* We should figure out that test_max has an MAX_EXPR in it. */
> +/* { dg-final { scan-tree-dump " = MAX_EXPR" "phiopt1"} } */
> +/* { dg-final { scan-tree-dump-times "changed to factor conversion out from" 2 "phiopt1"} } */
> diff --git a/gcc/tree-ssa-phiopt.cc b/gcc/tree-ssa-phiopt.cc
> index 41fea78dc8d..7fe088b13ff 100644
> --- a/gcc/tree-ssa-phiopt.cc
> +++ b/gcc/tree-ssa-phiopt.cc
> @@ -4085,20 +4085,23 @@ pass_phiopt::execute (function *)
>           node.  */
>        gcc_assert (arg0 != NULL_TREE && arg1 != NULL_TREE);
>
> -      gphi *newphi;
>        if (single_pred_p (bb1)
> -         && EDGE_COUNT (merge->preds) == 2
> -         && (newphi = factor_out_conditional_conversion (e1, e2, phi,
> -                                                         arg0, arg1,
> -                                                         cond_stmt)))
> +         && EDGE_COUNT (merge->preds) == 2)
>         {
> -         phi = newphi;
> -         /* factor_out_conditional_conversion may create a new PHI in
> -            BB2 and eliminate an existing PHI in BB2.  Recompute values
> -            that may be affected by that change.  */
> -         arg0 = gimple_phi_arg_def (phi, e1->dest_idx);
> -         arg1 = gimple_phi_arg_def (phi, e2->dest_idx);
> -         gcc_assert (arg0 != NULL_TREE && arg1 != NULL_TREE);
> +         gphi *newphi = phi;
> +         while (newphi)
> +           {
> +             phi = newphi;
> +             /* factor_out_conditional_conversion may create a new PHI in
> +                BB2 and eliminate an existing PHI in BB2.  Recompute values
> +                that may be affected by that change.  */
> +             arg0 = gimple_phi_arg_def (phi, e1->dest_idx);
> +             arg1 = gimple_phi_arg_def (phi, e2->dest_idx);
> +             gcc_assert (arg0 != NULL_TREE && arg1 != NULL_TREE);
> +             newphi = factor_out_conditional_conversion (e1, e2, phi,
> +                                                         arg0, arg1,
> +                                                         cond_stmt);
> +           }
>         }
>
>        /* Do the replacement of conditional if it can be done.  */
> --
> 2.31.1
>