Re: [PATCH 1/4] Canonicalize argument order for commutative functions

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

On Wed, Nov 10, 2021 at 1:50 PM Richard Sandiford via Gcc-patches
<gcc-patches@gcc.gnu.org> wrote:
>
> This patch uses information about internal functions to canonicalize
> the argument order of calls.
>
> Tested on aarch64-linux-gnu and x86_64-linux-gnu.  OK to install?

OK.  Note the gimple_resimplifyN functions also canonicalize operand
order, currently for is_tree_code only:

  /* Canonicalize operand order.  */
  bool canonicalized = false;
  if (res_op->code.is_tree_code ()
      && (TREE_CODE_CLASS ((enum tree_code) res_op->code) == tcc_comparison
          || commutative_tree_code (res_op->code))
      && tree_swap_operands_p (res_op->ops[0], res_op->ops[1]))
    {
      std::swap (res_op->ops[0], res_op->ops[1]);
      if (TREE_CODE_CLASS ((enum tree_code) res_op->code) == tcc_comparison)
        res_op->code = swap_tree_comparison (res_op->code);
      canonicalized = true;
    }

that's maybe not the best place.  The function assumes the operands
are already valueized,
so it maybe should be valueization that does the canonicalization -
but I think doing it
elsewhere made operand order unreliable (we do end up with
non-canonical order in
the IL sometimes).

So maybe you should amend the code in resimplifyN as well.

Richard.


> Richard
>
>
> gcc/
>         * gimple-fold.c: Include internal-fn.h.
>         (fold_stmt_1): If a function maps to an internal one, use
>         first_commutative_argument to canonicalize the order of
>         commutative arguments.
>
> gcc/testsuite/
>         * gcc.dg/fmax-fmin-1.c: New test.
> ---
>  gcc/gimple-fold.c                  | 25 ++++++++++++++++++++++---
>  gcc/testsuite/gcc.dg/fmax-fmin-1.c | 18 ++++++++++++++++++
>  2 files changed, 40 insertions(+), 3 deletions(-)
>  create mode 100644 gcc/testsuite/gcc.dg/fmax-fmin-1.c
>
> diff --git a/gcc/gimple-fold.c b/gcc/gimple-fold.c
> index a937f130815..6a7d4507c89 100644
> --- a/gcc/gimple-fold.c
> +++ b/gcc/gimple-fold.c
> @@ -69,6 +69,7 @@ along with GCC; see the file COPYING3.  If not see
>  #include "varasm.h"
>  #include "memmodel.h"
>  #include "optabs.h"
> +#include "internal-fn.h"
>
>  enum strlen_range_kind {
>    /* Compute the exact constant string length.  */
> @@ -6140,18 +6141,36 @@ fold_stmt_1 (gimple_stmt_iterator *gsi, bool inplace, tree (*valueize) (tree))
>        break;
>      case GIMPLE_CALL:
>        {
> -       for (i = 0; i < gimple_call_num_args (stmt); ++i)
> +       gcall *call = as_a<gcall *> (stmt);
> +       for (i = 0; i < gimple_call_num_args (call); ++i)
>           {
> -           tree *arg = gimple_call_arg_ptr (stmt, i);
> +           tree *arg = gimple_call_arg_ptr (call, i);
>             if (REFERENCE_CLASS_P (*arg)
>                 && maybe_canonicalize_mem_ref_addr (arg))
>               changed = true;
>           }
> -       tree *lhs = gimple_call_lhs_ptr (stmt);
> +       tree *lhs = gimple_call_lhs_ptr (call);
>         if (*lhs
>             && REFERENCE_CLASS_P (*lhs)
>             && maybe_canonicalize_mem_ref_addr (lhs))
>           changed = true;
> +       if (*lhs)
> +         {
> +           combined_fn cfn = gimple_call_combined_fn (call);
> +           internal_fn ifn = associated_internal_fn (cfn, TREE_TYPE (*lhs));
> +           int opno = first_commutative_argument (ifn);
> +           if (opno >= 0)
> +             {
> +               tree arg1 = gimple_call_arg (call, opno);
> +               tree arg2 = gimple_call_arg (call, opno + 1);
> +               if (tree_swap_operands_p (arg1, arg2))
> +                 {
> +                   gimple_call_set_arg (call, opno, arg2);
> +                   gimple_call_set_arg (call, opno + 1, arg1);
> +                   changed = true;
> +                 }
> +             }
> +         }
>         break;
>        }
>      case GIMPLE_ASM:
> diff --git a/gcc/testsuite/gcc.dg/fmax-fmin-1.c b/gcc/testsuite/gcc.dg/fmax-fmin-1.c
> new file mode 100644
> index 00000000000..e7e0518d8bb
> --- /dev/null
> +++ b/gcc/testsuite/gcc.dg/fmax-fmin-1.c
> @@ -0,0 +1,18 @@
> +/* { dg-options "-O -fdump-tree-optimized" } */
> +
> +void
> +f1 (double *res, double x, double y)
> +{
> +  res[0] = __builtin_fmax (x, y);
> +  res[1] = __builtin_fmax (y, x);
> +}
> +
> +void
> +f2 (double *res, double x, double y)
> +{
> +  res[0] = __builtin_fmin (x, y);
> +  res[1] = __builtin_fmin (y, x);
> +}
> +
> +/* { dg-final { scan-tree-dump-times {__builtin_fmax} 1 "optimized" } } */
> +/* { dg-final { scan-tree-dump-times {__builtin_fmin} 1 "optimized" } } */
> --
> 2.25.1
>