Re: [PATCH 1/4] Canonicalize argument order for commutative functions
On Mon, Nov 29, 2021 at 4:40 PM Richard Sandiford wrote: > > Sorry for the slow response, was away last week. > > Richard Biener writes: > > On Wed, Nov 10, 2021 at 1:50 PM Richard Sandiford via Gcc-patches > > 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. > > Hmm, yeah, thanks for the heads up. Does this updated version look OK? > Tested as before. Yes - OK. Thanks, Richard. > Thanks, > 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. > * gimple-match-head.c (gimple_resimplify2, gimple_resimplify3) > (gimple_resimplify4, gimple_resimplify5): Extend commutativity > checks to functions. > > gcc/testsuite/ > * gcc.dg/fmax-fmin-1.c: New test. > --- > gcc/gimple-fold.c | 25 -- > gcc/gimple-match-head.c| 52 -- > gcc/testsuite/gcc.dg/fmax-fmin-1.c | 18 +++ > 3 files changed, 75 insertions(+), 20 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 44fba12e150..1d8fd74f72c 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. */ > @@ -6109,18 +6110,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 (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/gimple-match-head.c b/gcc/gimple-match-head.c > index c481a625581..2d9364ca5de 100644 > --- a/gcc/gimple-match-head.c > +++ b/gcc/gimple-match-head.c > @@ -294,18 +294,16 @@ gimple_resimplify2 (gimple_seq *seq, gimple_match_op > *res_op, > >/* Canonicalize operand order. */ >bool canonicalized = false; > - if (res_op->code.is_tree_code ()) > + bool is_comparison > += (res_op->code.is_tree_code () > + && TREE_CODE_CLASS (tree_code (res_op->code)) == tcc_comparison); > + if ((is_comparison || commutative_binary_op_p (res_op->code, res_op->type)) > +
Re: [PATCH 1/4] Canonicalize argument order for commutative functions
Sorry for the slow response, was away last week. Richard Biener writes: > On Wed, Nov 10, 2021 at 1:50 PM Richard Sandiford via Gcc-patches > 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. Hmm, yeah, thanks for the heads up. Does this updated version look OK? Tested as before. Thanks, 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. * gimple-match-head.c (gimple_resimplify2, gimple_resimplify3) (gimple_resimplify4, gimple_resimplify5): Extend commutativity checks to functions. gcc/testsuite/ * gcc.dg/fmax-fmin-1.c: New test. --- gcc/gimple-fold.c | 25 -- gcc/gimple-match-head.c| 52 -- gcc/testsuite/gcc.dg/fmax-fmin-1.c | 18 +++ 3 files changed, 75 insertions(+), 20 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 44fba12e150..1d8fd74f72c 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. */ @@ -6109,18 +6110,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 (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/gimple-match-head.c b/gcc/gimple-match-head.c index c481a625581..2d9364ca5de 100644 --- a/gcc/gimple-match-head.c +++ b/gcc/gimple-match-head.c @@ -294,18 +294,16 @@ gimple_resimplify2 (gimple_seq *seq, gimple_match_op *res_op, /* Canonicalize operand order. */ bool canonicalized = false; - if (res_op->code.is_tree_code ()) + bool is_comparison += (res_op->code.is_tree_code () + && TREE_CODE_CLASS (tree_code (res_op->code)) == tcc_comparison); + if ((is_comparison || commutative_binary_op_p (res_op->code, res_op->type)) + && tree_swap_operands_p (res_op->ops[0], res_op->ops[1])) { - auto code = tree_code (res_op->code); - if ((TREE_CODE_CLASS (code) == tcc_comparison - || commutative_tree_code (code)) - && tree_swap_operands_p (res_op->ops[0], res_op->ops[1])) - { - std::swap (res_op->ops[0],
Re: [PATCH 1/4] Canonicalize argument order for commutative functions
On Wed, Nov 10, 2021 at 1:50 PM Richard Sandiford via Gcc-patches 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 (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 000..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 >
[PATCH 1/4] Canonicalize argument order for commutative functions
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? 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 (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 000..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