| Message ID | 20240817021300.1590935-1-quic_apinski@quicinc.com |
|---|---|
| State | New |
| Headers | show |
| Series | PHIOPT: move factor_out_conditional_operation over to use gimple_match_op | expand |
On 8/16/24 8:13 PM, Andrew Pinski wrote: > To start working on more with expressions with more than one operand, converting > over to use gimple_match_op is needed. > The added side-effect here is factor_out_conditional_operation can now support > builtins/internal calls that has one operand without any extra code added. > > Note on the changed testcases: > * pr87007-5.c: the test was testing testing for avoiding partial register stalls > for the sqrt and making sure there is only one zero of the register before the > branch, the phiopt would now merge the sqrt's so disable phiopt. > > Bootstrapped and tested on x86_64-linux-gnu with no regressions. > > gcc/ChangeLog: > > * gimple-match-exports.cc (gimple_match_op::operands_occurs_in_abnormal_phi): > New function. > * gimple-match.h (gimple_match_op): Add operands_occurs_in_abnormal_phi. > * tree-ssa-phiopt.cc (factor_out_conditional_operation): Use gimple_match_op > instead of manually extracting from/creating the gimple. > > gcc/testsuite/ChangeLog: > > * gcc.target/i386/pr87007-5.c: Disable phi-opt. > > diff --git a/gcc/testsuite/gcc.target/i386/pr87007-5.c b/gcc/testsuite/gcc.target/i386/pr87007-5.c > index 8f2dc947f6c..1a240adef63 100644 > --- a/gcc/testsuite/gcc.target/i386/pr87007-5.c > +++ b/gcc/testsuite/gcc.target/i386/pr87007-5.c > @@ -1,8 +1,11 @@ > /* { dg-do compile } */ > -/* { dg-options "-Ofast -march=skylake-avx512 -mfpmath=sse -fno-tree-vectorize -fdump-tree-cddce3-details -fdump-tree-lsplit-optimized" } */ > +/* { dg-options "-Ofast -march=skylake-avx512 -mfpmath=sse -fno-tree-vectorize -fdump-tree-cddce3-details -fdump-tree-lsplit-optimized -fno-ssa-phiopt" } */ > /* Load of d2/d3 is hoisted out, the loop is split, store of d1 and sqrt > are sunk out of the loop and the loop is elided. One vsqrtsd with > memory operand needs a xor to avoid partial dependence. */ > +/* Phi-OPT needs to ne disabled otherwise, sqrt calls are merged which is better > + but we are testing to make sure the partial register stall for SSE is still avoided > + for sqrts. */ Nit. s/to ne/to be/g OK with the nit fixed. Note this is getting closer to doing generalized sinking a common op through PHI nodes which is something we've wanted for a long time. Jeff
On Sun, Aug 18, 2024 at 11:06 AM Jeff Law <jeffreyalaw@gmail.com> wrote: > > > > On 8/16/24 8:13 PM, Andrew Pinski wrote: > > To start working on more with expressions with more than one operand, converting > > over to use gimple_match_op is needed. > > The added side-effect here is factor_out_conditional_operation can now support > > builtins/internal calls that has one operand without any extra code added. > > > > Note on the changed testcases: > > * pr87007-5.c: the test was testing testing for avoiding partial register stalls > > for the sqrt and making sure there is only one zero of the register before the > > branch, the phiopt would now merge the sqrt's so disable phiopt. > > > > Bootstrapped and tested on x86_64-linux-gnu with no regressions. > > > > gcc/ChangeLog: > > > > * gimple-match-exports.cc (gimple_match_op::operands_occurs_in_abnormal_phi): > > New function. > > * gimple-match.h (gimple_match_op): Add operands_occurs_in_abnormal_phi. > > * tree-ssa-phiopt.cc (factor_out_conditional_operation): Use gimple_match_op > > instead of manually extracting from/creating the gimple. > > > > gcc/testsuite/ChangeLog: > > > > * gcc.target/i386/pr87007-5.c: Disable phi-opt. > > > > > diff --git a/gcc/testsuite/gcc.target/i386/pr87007-5.c b/gcc/testsuite/gcc.target/i386/pr87007-5.c > > index 8f2dc947f6c..1a240adef63 100644 > > --- a/gcc/testsuite/gcc.target/i386/pr87007-5.c > > +++ b/gcc/testsuite/gcc.target/i386/pr87007-5.c > > @@ -1,8 +1,11 @@ > > /* { dg-do compile } */ > > -/* { dg-options "-Ofast -march=skylake-avx512 -mfpmath=sse -fno-tree-vectorize -fdump-tree-cddce3-details -fdump-tree-lsplit-optimized" } */ > > +/* { dg-options "-Ofast -march=skylake-avx512 -mfpmath=sse -fno-tree-vectorize -fdump-tree-cddce3-details -fdump-tree-lsplit-optimized -fno-ssa-phiopt" } */ > > /* Load of d2/d3 is hoisted out, the loop is split, store of d1 and sqrt > > are sunk out of the loop and the loop is elided. One vsqrtsd with > > memory operand needs a xor to avoid partial dependence. */ > > +/* Phi-OPT needs to ne disabled otherwise, sqrt calls are merged which is better > > + but we are testing to make sure the partial register stall for SSE is still avoided > > + for sqrts. */ > Nit. s/to ne/to be/g > > OK with the nit fixed. > > Note this is getting closer to doing generalized sinking a common op > through PHI nodes which is something we've wanted for a long time. Yes that is the plan; I just want to do it in steps as I have a few other projects in progress; and I don't know how much of each I will be able to get done in time for GCC 15. I originally had this done differently but I thought it would be better to reuse infrastructure that was already there instead of creating new ones. I had implemented this patch back in April and I didn't know if I could get to the rest due to other projects going on so I submitted it finally. Thanks, Andrew > > Jeff >
diff --git a/gcc/gimple-match-exports.cc b/gcc/gimple-match-exports.cc index aacf3ff0414..15d54b7d843 100644 --- a/gcc/gimple-match-exports.cc +++ b/gcc/gimple-match-exports.cc @@ -126,6 +126,20 @@ gimple_match_op::resimplify (gimple_seq *seq, tree (*valueize)(tree)) } } +/* Returns true if any of the operands of THIS occurs + in abnormal phis. */ +bool +gimple_match_op::operands_occurs_in_abnormal_phi() const +{ + for (unsigned int i = 0; i < num_ops; i++) + { + if (TREE_CODE (ops[i]) == SSA_NAME + && SSA_NAME_OCCURS_IN_ABNORMAL_PHI (ops[i])) + return true; + } + return false; +} + /* Return whether T is a constant that we'll dispatch to fold to evaluate fully constant expressions. */ diff --git a/gcc/gimple-match.h b/gcc/gimple-match.h index d710fcbace2..8edff578ba9 100644 --- a/gcc/gimple-match.h +++ b/gcc/gimple-match.h @@ -136,6 +136,8 @@ public: /* The operands to CODE. Only the first NUM_OPS entries are meaningful. */ tree ops[MAX_NUM_OPS]; + + bool operands_occurs_in_abnormal_phi() const; }; inline diff --git a/gcc/testsuite/gcc.target/i386/pr87007-5.c b/gcc/testsuite/gcc.target/i386/pr87007-5.c index 8f2dc947f6c..1a240adef63 100644 --- a/gcc/testsuite/gcc.target/i386/pr87007-5.c +++ b/gcc/testsuite/gcc.target/i386/pr87007-5.c @@ -1,8 +1,11 @@ /* { dg-do compile } */ -/* { dg-options "-Ofast -march=skylake-avx512 -mfpmath=sse -fno-tree-vectorize -fdump-tree-cddce3-details -fdump-tree-lsplit-optimized" } */ +/* { dg-options "-Ofast -march=skylake-avx512 -mfpmath=sse -fno-tree-vectorize -fdump-tree-cddce3-details -fdump-tree-lsplit-optimized -fno-ssa-phiopt" } */ /* Load of d2/d3 is hoisted out, the loop is split, store of d1 and sqrt are sunk out of the loop and the loop is elided. One vsqrtsd with memory operand needs a xor to avoid partial dependence. */ +/* Phi-OPT needs to ne disabled otherwise, sqrt calls are merged which is better + but we are testing to make sure the partial register stall for SSE is still avoided + for sqrts. */ #include<math.h> diff --git a/gcc/tree-ssa-phiopt.cc b/gcc/tree-ssa-phiopt.cc index aacccc414f6..2d4aba5b087 100644 --- a/gcc/tree-ssa-phiopt.cc +++ b/gcc/tree-ssa-phiopt.cc @@ -220,13 +220,12 @@ static gphi * factor_out_conditional_operation (edge e0, edge e1, gphi *phi, tree arg0, tree arg1, gimple *cond_stmt) { - gimple *arg0_def_stmt = NULL, *arg1_def_stmt = NULL, *new_stmt; - tree new_arg0 = NULL_TREE, new_arg1 = NULL_TREE; + gimple *arg0_def_stmt = NULL, *arg1_def_stmt = NULL; tree temp, result; gphi *newphi; gimple_stmt_iterator gsi, gsi_for_def; location_t locus = gimple_location (phi); - enum tree_code op_code; + gimple_match_op arg0_op, arg1_op; /* Handle only PHI statements with two arguments. TODO: If all other arguments to PHI are INTEGER_CST or if their defining @@ -250,31 +249,31 @@ factor_out_conditional_operation (edge e0, edge e1, gphi *phi, /* Check if arg0 is an SSA_NAME and the stmt which defines arg0 is an unary operation. */ arg0_def_stmt = SSA_NAME_DEF_STMT (arg0); - if (!is_gimple_assign (arg0_def_stmt) - || (gimple_assign_rhs_class (arg0_def_stmt) != GIMPLE_UNARY_RHS - && gimple_assign_rhs_code (arg0_def_stmt) != VIEW_CONVERT_EXPR)) + if (!gimple_extract_op (arg0_def_stmt, &arg0_op)) return NULL; - /* Use the RHS as new_arg0. */ - op_code = gimple_assign_rhs_code (arg0_def_stmt); - new_arg0 = gimple_assign_rhs1 (arg0_def_stmt); - if (op_code == VIEW_CONVERT_EXPR) - { - new_arg0 = TREE_OPERAND (new_arg0, 0); - if (!is_gimple_reg_type (TREE_TYPE (new_arg0))) - return NULL; - } - if (TREE_CODE (new_arg0) == SSA_NAME - && SSA_NAME_OCCURS_IN_ABNORMAL_PHI (new_arg0)) + /* Check to make sure none of the operands are in abnormal phis. */ + if (arg0_op.operands_occurs_in_abnormal_phi ()) + return NULL; + + /* Currently just support one operand expressions. */ + if (arg0_op.num_ops != 1) return NULL; + tree new_arg0 = arg0_op.ops[0]; + tree new_arg1; + if (TREE_CODE (arg1) == SSA_NAME) { - /* Check if arg1 is an SSA_NAME and the stmt which defines arg1 - is an unary operation. */ + /* Check if arg1 is an SSA_NAME. */ arg1_def_stmt = SSA_NAME_DEF_STMT (arg1); - if (!is_gimple_assign (arg1_def_stmt) - || gimple_assign_rhs_code (arg1_def_stmt) != op_code) + if (!gimple_extract_op (arg1_def_stmt, &arg1_op)) + return NULL; + if (arg1_op.code != arg0_op.code) + return NULL; + if (arg1_op.num_ops != arg0_op.num_ops) + return NULL; + if (arg1_op.operands_occurs_in_abnormal_phi ()) return NULL; /* Either arg1_def_stmt or arg0_def_stmt should be conditional. */ @@ -282,14 +281,7 @@ factor_out_conditional_operation (edge e0, edge e1, gphi *phi, && dominated_by_p (CDI_DOMINATORS, gimple_bb (phi), gimple_bb (arg1_def_stmt))) return NULL; - - /* Use the RHS as new_arg1. */ - new_arg1 = gimple_assign_rhs1 (arg1_def_stmt); - if (op_code == VIEW_CONVERT_EXPR) - new_arg1 = TREE_OPERAND (new_arg1, 0); - if (TREE_CODE (new_arg1) == SSA_NAME - && SSA_NAME_OCCURS_IN_ABNORMAL_PHI (new_arg1)) - return NULL; + new_arg1 = arg1_op.ops[0]; } else { @@ -300,6 +292,7 @@ factor_out_conditional_operation (edge e0, edge e1, gphi *phi, /* arg0_def_stmt should be conditional. */ if (dominated_by_p (CDI_DOMINATORS, gimple_bb (phi), gimple_bb (arg0_def_stmt))) return NULL; + /* If arg1 is an INTEGER_CST, fold it to new type. */ if (INTEGRAL_TYPE_P (TREE_TYPE (new_arg0)) && (int_fits_type_p (arg1, TREE_TYPE (new_arg0)) @@ -405,16 +398,15 @@ factor_out_conditional_operation (edge e0, edge e1, gphi *phi, add_phi_arg (newphi, new_arg0, e0, locus); add_phi_arg (newphi, new_arg1, e1, locus); + gimple_match_op new_op = arg0_op; + /* Create the operation stmt and insert it. */ - if (op_code == VIEW_CONVERT_EXPR) - { - temp = fold_build1 (VIEW_CONVERT_EXPR, TREE_TYPE (result), temp); - new_stmt = gimple_build_assign (result, temp); - } - else - new_stmt = gimple_build_assign (result, op_code, temp); + new_op.ops[0] = temp; + gimple_seq seq = NULL; + result = maybe_push_res_to_seq (&new_op, &seq, result); + gcc_assert (result); gsi = gsi_after_labels (gimple_bb (phi)); - gsi_insert_before (&gsi, new_stmt, GSI_SAME_STMT); + gsi_insert_seq_before (&gsi, seq, GSI_CONTINUE_LINKING); /* Remove the original PHI stmt. */ gsi = gsi_for_stmt (phi);
To start working on more with expressions with more than one operand, converting over to use gimple_match_op is needed. The added side-effect here is factor_out_conditional_operation can now support builtins/internal calls that has one operand without any extra code added. Note on the changed testcases: * pr87007-5.c: the test was testing testing for avoiding partial register stalls for the sqrt and making sure there is only one zero of the register before the branch, the phiopt would now merge the sqrt's so disable phiopt. Bootstrapped and tested on x86_64-linux-gnu with no regressions. gcc/ChangeLog: * gimple-match-exports.cc (gimple_match_op::operands_occurs_in_abnormal_phi): New function. * gimple-match.h (gimple_match_op): Add operands_occurs_in_abnormal_phi. * tree-ssa-phiopt.cc (factor_out_conditional_operation): Use gimple_match_op instead of manually extracting from/creating the gimple. gcc/testsuite/ChangeLog: * gcc.target/i386/pr87007-5.c: Disable phi-opt. Signed-off-by: Andrew Pinski <quic_apinski@quicinc.com> --- gcc/gimple-match-exports.cc | 14 +++++ gcc/gimple-match.h | 2 + gcc/testsuite/gcc.target/i386/pr87007-5.c | 5 +- gcc/tree-ssa-phiopt.cc | 66 ++++++++++------------- 4 files changed, 49 insertions(+), 38 deletions(-)