| Message ID | 1625808781-21156-3-git-send-email-apinski@marvell.com |
|---|---|
| State | New |
| Headers | show |
| Series | Misc PHIOPT patches | expand |
On Fri, Jul 9, 2021 at 7:34 AM apinski--- via Gcc-patches <gcc-patches@gcc.gnu.org> wrote: > > From: Andrew Pinski <apinski@marvell.com> > > Instead of waiting for DCE to remove the unused statement, > and maybe optimize another conditional, it is better if > we don't move the statement and have the statement > removed. > > gcc/ChangeLog: > > * tree-ssa-phiopt.c (used_in_seq): New function. > (match_simplify_replacement): Don't move the statement > if not used in sequence. > > gcc/testsuite/ChangeLog: > > * gcc.dg/tree-ssa/pr96928-1.c: Update to similar as pr96928.c. > --- > gcc/testsuite/gcc.dg/tree-ssa/pr96928-1.c | 5 ++++- > gcc/tree-ssa-phiopt.c | 24 ++++++++++++++++++++++- > 2 files changed, 27 insertions(+), 2 deletions(-) > > diff --git a/gcc/testsuite/gcc.dg/tree-ssa/pr96928-1.c b/gcc/testsuite/gcc.dg/tree-ssa/pr96928-1.c > index 2e86620da11..9e505ac9900 100644 > --- a/gcc/testsuite/gcc.dg/tree-ssa/pr96928-1.c > +++ b/gcc/testsuite/gcc.dg/tree-ssa/pr96928-1.c > @@ -2,7 +2,10 @@ > /* { dg-do compile } */ > /* { dg-options "-O2 -fdump-tree-phiopt2 -fdump-tree-optimized" } */ > /* { dg-final { scan-tree-dump-times " = a_\[0-9]*\\\(D\\\) >> " 5 "phiopt2" } } */ > -/* { dg-final { scan-tree-dump-times " = ~c_\[0-9]*\\\(D\\\);" 1 "phiopt2" } } */ > +/* The following check is done at optimized because a ^ (~b) is rewritten as ~(a^b) > + and in the case of match.pd optimizing these ?:, the ~ is moved out already > + by the time we get to phiopt2. */ > +/* { dg-final { scan-tree-dump-times "c_\[0-9]*\\\(D\\\) \\\^" 1 "optimized" } } */ > /* { dg-final { scan-tree-dump-times " = ~" 1 "optimized" } } */ > /* { dg-final { scan-tree-dump-times " = \[abc_0-9\\\(\\\)D]* \\\^ " 5 "phiopt2" } } */ > /* { dg-final { scan-tree-dump-not "a < 0" "phiopt2" } } */ > diff --git a/gcc/tree-ssa-phiopt.c b/gcc/tree-ssa-phiopt.c > index 7a98b7afdf1..a237df02153 100644 > --- a/gcc/tree-ssa-phiopt.c > +++ b/gcc/tree-ssa-phiopt.c > @@ -934,6 +934,26 @@ gimple_simplify_phiopt (bool early_p, tree type, gimple *comp_stmt, > return NULL; > } > > +/* Return true if the lhs of STMT is used in the SEQ sequence > + of statements. */ > +static bool > +used_in_seq (gimple *stmt, gimple_seq seq) > +{ > + tree lhs = gimple_assign_lhs (stmt); > + for (auto gsi = gsi_start (seq); !gsi_end_p (gsi); gsi_next_nondebug (&gsi)) > + { > + use_operand_p use_p; > + ssa_op_iter iter; > + gimple *stmt1 = gsi_stmt (gsi); > + FOR_EACH_SSA_USE_OPERAND (use_p, stmt1, iter, SSA_OP_USE) > + { > + if (USE_FROM_PTR (use_p) == lhs) > + return true; > + } > + } > + return false; > +} > + > /* The function match_simplify_replacement does the main work of doing the > replacement using match and simplify. Return true if the replacement is done. > Otherwise return false. > @@ -1020,7 +1040,9 @@ match_simplify_replacement (basic_block cond_bb, basic_block middle_bb, > return false; > > gsi = gsi_last_bb (cond_bb); > - if (stmt_to_move) > + if (stmt_to_move > + && (gimple_assign_lhs (stmt_to_move) == result > + || used_in_seq (stmt_to_move, seq))) Err, why not insert 'seq' before moving the stmt (you'd have to fiddle with the iterator, using GSI_CONTINUE_LINKING I think) and then check has_zero_uses on the (hopefully) only def of the stmt to move? Richard. > { > if (dump_file && (dump_flags & TDF_DETAILS)) > { > -- > 2.27.0 >
On Thu, Jul 8, 2021 at 11:50 PM Richard Biener via Gcc-patches <gcc-patches@gcc.gnu.org> wrote: > > On Fri, Jul 9, 2021 at 7:34 AM apinski--- via Gcc-patches > <gcc-patches@gcc.gnu.org> wrote: > > > > From: Andrew Pinski <apinski@marvell.com> > > > > Instead of waiting for DCE to remove the unused statement, > > and maybe optimize another conditional, it is better if > > we don't move the statement and have the statement > > removed. > > > > gcc/ChangeLog: > > > > * tree-ssa-phiopt.c (used_in_seq): New function. > > (match_simplify_replacement): Don't move the statement > > if not used in sequence. > > > > gcc/testsuite/ChangeLog: > > > > * gcc.dg/tree-ssa/pr96928-1.c: Update to similar as pr96928.c. > > --- > > gcc/testsuite/gcc.dg/tree-ssa/pr96928-1.c | 5 ++++- > > gcc/tree-ssa-phiopt.c | 24 ++++++++++++++++++++++- > > 2 files changed, 27 insertions(+), 2 deletions(-) > > > > diff --git a/gcc/testsuite/gcc.dg/tree-ssa/pr96928-1.c b/gcc/testsuite/gcc.dg/tree-ssa/pr96928-1.c > > index 2e86620da11..9e505ac9900 100644 > > --- a/gcc/testsuite/gcc.dg/tree-ssa/pr96928-1.c > > +++ b/gcc/testsuite/gcc.dg/tree-ssa/pr96928-1.c > > @@ -2,7 +2,10 @@ > > /* { dg-do compile } */ > > /* { dg-options "-O2 -fdump-tree-phiopt2 -fdump-tree-optimized" } */ > > /* { dg-final { scan-tree-dump-times " = a_\[0-9]*\\\(D\\\) >> " 5 "phiopt2" } } */ > > -/* { dg-final { scan-tree-dump-times " = ~c_\[0-9]*\\\(D\\\);" 1 "phiopt2" } } */ > > +/* The following check is done at optimized because a ^ (~b) is rewritten as ~(a^b) > > + and in the case of match.pd optimizing these ?:, the ~ is moved out already > > + by the time we get to phiopt2. */ > > +/* { dg-final { scan-tree-dump-times "c_\[0-9]*\\\(D\\\) \\\^" 1 "optimized" } } */ > > /* { dg-final { scan-tree-dump-times " = ~" 1 "optimized" } } */ > > /* { dg-final { scan-tree-dump-times " = \[abc_0-9\\\(\\\)D]* \\\^ " 5 "phiopt2" } } */ > > /* { dg-final { scan-tree-dump-not "a < 0" "phiopt2" } } */ > > diff --git a/gcc/tree-ssa-phiopt.c b/gcc/tree-ssa-phiopt.c > > index 7a98b7afdf1..a237df02153 100644 > > --- a/gcc/tree-ssa-phiopt.c > > +++ b/gcc/tree-ssa-phiopt.c > > @@ -934,6 +934,26 @@ gimple_simplify_phiopt (bool early_p, tree type, gimple *comp_stmt, > > return NULL; > > } > > > > +/* Return true if the lhs of STMT is used in the SEQ sequence > > + of statements. */ > > +static bool > > +used_in_seq (gimple *stmt, gimple_seq seq) > > +{ > > + tree lhs = gimple_assign_lhs (stmt); > > + for (auto gsi = gsi_start (seq); !gsi_end_p (gsi); gsi_next_nondebug (&gsi)) > > + { > > + use_operand_p use_p; > > + ssa_op_iter iter; > > + gimple *stmt1 = gsi_stmt (gsi); > > + FOR_EACH_SSA_USE_OPERAND (use_p, stmt1, iter, SSA_OP_USE) > > + { > > + if (USE_FROM_PTR (use_p) == lhs) > > + return true; > > + } > > + } > > + return false; > > +} > > + > > /* The function match_simplify_replacement does the main work of doing the > > replacement using match and simplify. Return true if the replacement is done. > > Otherwise return false. > > @@ -1020,7 +1040,9 @@ match_simplify_replacement (basic_block cond_bb, basic_block middle_bb, > > return false; > > > > gsi = gsi_last_bb (cond_bb); > > - if (stmt_to_move) > > + if (stmt_to_move > > + && (gimple_assign_lhs (stmt_to_move) == result > > + || used_in_seq (stmt_to_move, seq))) > > Err, why not insert 'seq' before moving the stmt (you'd have to fiddle > with the iterator, > using GSI_CONTINUE_LINKING I think) and then check has_zero_uses on > the (hopefully) only > def of the stmt to move? Because stmt_to_move was used in the phi and if we move replace_phi_edge_with_variable before we move the statement, the statement has been removed permanently as the basic block holding it has been deleted. What about this order instead: remove stmt_to_move (not permanently) call replace_phi_edge_with_variable insert seq if !zero_uses insert stmt_to_move before seq else release defs for stmt_to_move Thanks, Andrew Pinski > > Richard. > > > { > > if (dump_file && (dump_flags & TDF_DETAILS)) > > { > > -- > > 2.27.0 > >
On Fri, Jul 9, 2021 at 9:16 AM Andrew Pinski <pinskia@gmail.com> wrote: > > On Thu, Jul 8, 2021 at 11:50 PM Richard Biener via Gcc-patches > <gcc-patches@gcc.gnu.org> wrote: > > > > On Fri, Jul 9, 2021 at 7:34 AM apinski--- via Gcc-patches > > <gcc-patches@gcc.gnu.org> wrote: > > > > > > From: Andrew Pinski <apinski@marvell.com> > > > > > > Instead of waiting for DCE to remove the unused statement, > > > and maybe optimize another conditional, it is better if > > > we don't move the statement and have the statement > > > removed. > > > > > > gcc/ChangeLog: > > > > > > * tree-ssa-phiopt.c (used_in_seq): New function. > > > (match_simplify_replacement): Don't move the statement > > > if not used in sequence. > > > > > > gcc/testsuite/ChangeLog: > > > > > > * gcc.dg/tree-ssa/pr96928-1.c: Update to similar as pr96928.c. > > > --- > > > gcc/testsuite/gcc.dg/tree-ssa/pr96928-1.c | 5 ++++- > > > gcc/tree-ssa-phiopt.c | 24 ++++++++++++++++++++++- > > > 2 files changed, 27 insertions(+), 2 deletions(-) > > > > > > diff --git a/gcc/testsuite/gcc.dg/tree-ssa/pr96928-1.c b/gcc/testsuite/gcc.dg/tree-ssa/pr96928-1.c > > > index 2e86620da11..9e505ac9900 100644 > > > --- a/gcc/testsuite/gcc.dg/tree-ssa/pr96928-1.c > > > +++ b/gcc/testsuite/gcc.dg/tree-ssa/pr96928-1.c > > > @@ -2,7 +2,10 @@ > > > /* { dg-do compile } */ > > > /* { dg-options "-O2 -fdump-tree-phiopt2 -fdump-tree-optimized" } */ > > > /* { dg-final { scan-tree-dump-times " = a_\[0-9]*\\\(D\\\) >> " 5 "phiopt2" } } */ > > > -/* { dg-final { scan-tree-dump-times " = ~c_\[0-9]*\\\(D\\\);" 1 "phiopt2" } } */ > > > +/* The following check is done at optimized because a ^ (~b) is rewritten as ~(a^b) > > > + and in the case of match.pd optimizing these ?:, the ~ is moved out already > > > + by the time we get to phiopt2. */ > > > +/* { dg-final { scan-tree-dump-times "c_\[0-9]*\\\(D\\\) \\\^" 1 "optimized" } } */ > > > /* { dg-final { scan-tree-dump-times " = ~" 1 "optimized" } } */ > > > /* { dg-final { scan-tree-dump-times " = \[abc_0-9\\\(\\\)D]* \\\^ " 5 "phiopt2" } } */ > > > /* { dg-final { scan-tree-dump-not "a < 0" "phiopt2" } } */ > > > diff --git a/gcc/tree-ssa-phiopt.c b/gcc/tree-ssa-phiopt.c > > > index 7a98b7afdf1..a237df02153 100644 > > > --- a/gcc/tree-ssa-phiopt.c > > > +++ b/gcc/tree-ssa-phiopt.c > > > @@ -934,6 +934,26 @@ gimple_simplify_phiopt (bool early_p, tree type, gimple *comp_stmt, > > > return NULL; > > > } > > > > > > +/* Return true if the lhs of STMT is used in the SEQ sequence > > > + of statements. */ > > > +static bool > > > +used_in_seq (gimple *stmt, gimple_seq seq) > > > +{ > > > + tree lhs = gimple_assign_lhs (stmt); > > > + for (auto gsi = gsi_start (seq); !gsi_end_p (gsi); gsi_next_nondebug (&gsi)) > > > + { > > > + use_operand_p use_p; > > > + ssa_op_iter iter; > > > + gimple *stmt1 = gsi_stmt (gsi); > > > + FOR_EACH_SSA_USE_OPERAND (use_p, stmt1, iter, SSA_OP_USE) > > > + { > > > + if (USE_FROM_PTR (use_p) == lhs) > > > + return true; > > > + } > > > + } > > > + return false; > > > +} > > > + > > > /* The function match_simplify_replacement does the main work of doing the > > > replacement using match and simplify. Return true if the replacement is done. > > > Otherwise return false. > > > @@ -1020,7 +1040,9 @@ match_simplify_replacement (basic_block cond_bb, basic_block middle_bb, > > > return false; > > > > > > gsi = gsi_last_bb (cond_bb); > > > - if (stmt_to_move) > > > + if (stmt_to_move > > > + && (gimple_assign_lhs (stmt_to_move) == result > > > + || used_in_seq (stmt_to_move, seq))) > > > > Err, why not insert 'seq' before moving the stmt (you'd have to fiddle > > with the iterator, > > using GSI_CONTINUE_LINKING I think) and then check has_zero_uses on > > the (hopefully) only > > def of the stmt to move? > > Because stmt_to_move was used in the phi and if we move > replace_phi_edge_with_variable before we move the statement, the > statement has been removed permanently as the basic block holding it > has been deleted. > > What about this order instead: > remove stmt_to_move (not permanently) > call replace_phi_edge_with_variable > insert seq > if !zero_uses > insert stmt_to_move before seq > else > release defs for stmt_to_move Hmm, so if the stmt was used by the PHI then why not check for has_single_use after inserting the sequence (but before removing the PHI)? Richard. > Thanks, > Andrew Pinski > > > > > Richard. > > > > > { > > > if (dump_file && (dump_flags & TDF_DETAILS)) > > > { > > > -- > > > 2.27.0 > > >
diff --git a/gcc/testsuite/gcc.dg/tree-ssa/pr96928-1.c b/gcc/testsuite/gcc.dg/tree-ssa/pr96928-1.c index 2e86620da11..9e505ac9900 100644 --- a/gcc/testsuite/gcc.dg/tree-ssa/pr96928-1.c +++ b/gcc/testsuite/gcc.dg/tree-ssa/pr96928-1.c @@ -2,7 +2,10 @@ /* { dg-do compile } */ /* { dg-options "-O2 -fdump-tree-phiopt2 -fdump-tree-optimized" } */ /* { dg-final { scan-tree-dump-times " = a_\[0-9]*\\\(D\\\) >> " 5 "phiopt2" } } */ -/* { dg-final { scan-tree-dump-times " = ~c_\[0-9]*\\\(D\\\);" 1 "phiopt2" } } */ +/* The following check is done at optimized because a ^ (~b) is rewritten as ~(a^b) + and in the case of match.pd optimizing these ?:, the ~ is moved out already + by the time we get to phiopt2. */ +/* { dg-final { scan-tree-dump-times "c_\[0-9]*\\\(D\\\) \\\^" 1 "optimized" } } */ /* { dg-final { scan-tree-dump-times " = ~" 1 "optimized" } } */ /* { dg-final { scan-tree-dump-times " = \[abc_0-9\\\(\\\)D]* \\\^ " 5 "phiopt2" } } */ /* { dg-final { scan-tree-dump-not "a < 0" "phiopt2" } } */ diff --git a/gcc/tree-ssa-phiopt.c b/gcc/tree-ssa-phiopt.c index 7a98b7afdf1..a237df02153 100644 --- a/gcc/tree-ssa-phiopt.c +++ b/gcc/tree-ssa-phiopt.c @@ -934,6 +934,26 @@ gimple_simplify_phiopt (bool early_p, tree type, gimple *comp_stmt, return NULL; } +/* Return true if the lhs of STMT is used in the SEQ sequence + of statements. */ +static bool +used_in_seq (gimple *stmt, gimple_seq seq) +{ + tree lhs = gimple_assign_lhs (stmt); + for (auto gsi = gsi_start (seq); !gsi_end_p (gsi); gsi_next_nondebug (&gsi)) + { + use_operand_p use_p; + ssa_op_iter iter; + gimple *stmt1 = gsi_stmt (gsi); + FOR_EACH_SSA_USE_OPERAND (use_p, stmt1, iter, SSA_OP_USE) + { + if (USE_FROM_PTR (use_p) == lhs) + return true; + } + } + return false; +} + /* The function match_simplify_replacement does the main work of doing the replacement using match and simplify. Return true if the replacement is done. Otherwise return false. @@ -1020,7 +1040,9 @@ match_simplify_replacement (basic_block cond_bb, basic_block middle_bb, return false; gsi = gsi_last_bb (cond_bb); - if (stmt_to_move) + if (stmt_to_move + && (gimple_assign_lhs (stmt_to_move) == result + || used_in_seq (stmt_to_move, seq))) { if (dump_file && (dump_flags & TDF_DETAILS)) {
From: Andrew Pinski <apinski@marvell.com> Instead of waiting for DCE to remove the unused statement, and maybe optimize another conditional, it is better if we don't move the statement and have the statement removed. gcc/ChangeLog: * tree-ssa-phiopt.c (used_in_seq): New function. (match_simplify_replacement): Don't move the statement if not used in sequence. gcc/testsuite/ChangeLog: * gcc.dg/tree-ssa/pr96928-1.c: Update to similar as pr96928.c. --- gcc/testsuite/gcc.dg/tree-ssa/pr96928-1.c | 5 ++++- gcc/tree-ssa-phiopt.c | 24 ++++++++++++++++++++++- 2 files changed, 27 insertions(+), 2 deletions(-)