Re: [PATCH 2/2] [PHIOPT/MATCH] Remove the statement to move if not used
On Fri, Jul 9, 2021 at 9:16 AM Andrew Pinski wrote:
>
> On Thu, Jul 8, 2021 at 11:50 PM Richard Biener via Gcc-patches
> wrote:
> >
> > On Fri, Jul 9, 2021 at 7:34 AM apinski--- via Gcc-patches
> > wrote:
> > >
> > > From: Andrew Pinski
> > >
> > > 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
> > >
Re: [PATCH 2/2] [PHIOPT/MATCH] Remove the statement to move if not used
On Thu, Jul 8, 2021 at 11:50 PM Richard Biener via Gcc-patches
wrote:
>
> On Fri, Jul 9, 2021 at 7:34 AM apinski--- via Gcc-patches
> wrote:
> >
> > From: Andrew Pinski
> >
> > 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
> >
Re: [PATCH 2/2] [PHIOPT/MATCH] Remove the statement to move if not used
On Fri, Jul 9, 2021 at 7:34 AM apinski--- via Gcc-patches
wrote:
>
> From: Andrew Pinski
>
> 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
>
[PATCH 2/2] [PHIOPT/MATCH] Remove the statement to move if not used
From: Andrew Pinski
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)))
{
if (dump_file && (dump_flags & TDF_DETAILS))
{
--
2.27.0
