Hello,
This should have the changes you wanted!
Thank you,
Matthew Beliveau
On Fri, Aug 16, 2019 at 12:30 PM Jeff Law <l...@redhat.com> wrote:
>
> On 8/13/19 9:09 AM, Matthew Beliveau wrote:
> > Hello,
> >
> > This should have the changes you all asked for! Let me know if I
> > missed anything!
> >
> > Thank you,
> > Matthew Beliveau
> >
> > On Tue, Aug 13, 2019 at 5:15 AM Richard Biener
> > <richard.guent...@gmail.com> wrote:
> >> On Tue, Aug 13, 2019 at 10:18 AM Richard Sandiford
> >> <richard.sandif...@arm.com> wrote:
> >>> Thanks for doing this.
> >>>
> >>> Matthew Beliveau <mbeli...@redhat.com> writes:
> >>>> diff --git gcc/tree-ssa-dse.c gcc/tree-ssa-dse.c
> >>>> index 5b7c4fc6d1a..dcaeb8edbfe 100644
> >>>> --- gcc/tree-ssa-dse.c
> >>>> +++ gcc/tree-ssa-dse.c
> >>>> @@ -628,11 +628,8 @@ dse_optimize_redundant_stores (gimple *stmt)
> >>>> tree fndecl;
> >>>> if ((is_gimple_assign (use_stmt)
> >>>> && gimple_vdef (use_stmt)
> >>>> - && ((gimple_assign_rhs_code (use_stmt) == CONSTRUCTOR
> >>>> - && CONSTRUCTOR_NELTS (gimple_assign_rhs1 (use_stmt)) == 0
> >>>> - && !gimple_clobber_p (stmt))
> >>>> - || (gimple_assign_rhs_code (use_stmt) == INTEGER_CST
> >>>> - && integer_zerop (gimple_assign_rhs1 (use_stmt)))))
> >>>> + && initializer_zerop (gimple_op (use_stmt, 1), NULL)
> >>>> + && !gimple_clobber_p (stmt))
> >>>> || (gimple_call_builtin_p (use_stmt, BUILT_IN_NORMAL)
> >>>> && (fndecl = gimple_call_fndecl (use_stmt)) != NULL
> >>>> && (DECL_FUNCTION_CODE (fndecl) == BUILT_IN_MEMSET
> >>>> @@ -1027,15 +1024,13 @@ dse_dom_walker::dse_optimize_stmt
> >>>> (gimple_stmt_iterator *gsi)
> >>>> {
> >>>> bool by_clobber_p = false;
> >>>>
> >>>> - /* First see if this store is a CONSTRUCTOR and if there
> >>>> - are subsequent CONSTRUCTOR stores which are totally
> >>>> - subsumed by this statement. If so remove the subsequent
> >>>> - CONSTRUCTOR store.
> >>>> + /* Check if this store initalizes zero, or some aggregate of
> >>>> zeros,
> >>>> + and check if there are subsequent stores which are subsumed by
> >>>> this
> >>>> + statement. If so, remove the subsequent store.
> >>>>
> >>>> This will tend to make fewer calls into memset with longer
> >>>> arguments. */
> >>>> - if (gimple_assign_rhs_code (stmt) == CONSTRUCTOR
> >>>> - && CONSTRUCTOR_NELTS (gimple_assign_rhs1 (stmt)) == 0
> >>>> + if (initializer_zerop (gimple_op (stmt, 1), NULL)
> >>>> && !gimple_clobber_p (stmt))
> >>>> dse_optimize_redundant_stores (stmt);
> >>>>
> >>> In addition to Jeff's comment, the original choice of gimple_assign_rhs1
> >>> is the preferred way to write this (applies to both hunks).
> >> And the !gimple_clobber_p test is now redundant.
> >>
> >>> Richard
> >
> > DSE-2.patch
> >
> > Bootstrapped/regtested on x86_64-linux, ok for trunk?
> >
> > 2019-08-13 Matthew Beliveau <mbeli...@redhat.com>
> >
> > * tree-ssa-dse.c (dse_optimize_redundant_stores): Improved check to
> > catch more redundant zero initialization cases.
> > (dse_dom_walker::dse_optimize_stmt): Likewise.
> >
> > * gcc.dg/tree-ssa/redundant-assign-zero-1.c: New test.
> > * gcc.dg/tree-ssa/redundant-assign-zero-2.c: New test.
> >
> > diff --git gcc/testsuite/gcc.dg/tree-ssa/redundant-assign-zero-1.c
> > gcc/testsuite/gcc.dg/tree-ssa/redundant-assign-zero-1.c
> > new file mode 100644
> > index 00000000000..b8d01d1644b
> > --- /dev/null
> > +++ gcc/testsuite/gcc.dg/tree-ssa/redundant-assign-zero-1.c
> > @@ -0,0 +1,13 @@
> > +/* { dg-do compile } */
> > +/* { dg-options "-O2 -fdump-tree-dse-details" } */
> > +
> > +void blah (char *);
> > +
> > +void bar ()
> > +{
> > + char a[256] = "";
> > + a[3] = 0;
> > + blah (a);
> > +}
> > +
> > +/* { dg-final { scan-tree-dump-times "Deleted redundant store" 1 "dse1"} }
> > */
> > diff --git gcc/testsuite/gcc.dg/tree-ssa/redundant-assign-zero-2.c
> > gcc/testsuite/gcc.dg/tree-ssa/redundant-assign-zero-2.c
> > new file mode 100644
> > index 00000000000..8cefa6f0cb7
> > --- /dev/null
> > +++ gcc/testsuite/gcc.dg/tree-ssa/redundant-assign-zero-2.c
> > @@ -0,0 +1,18 @@
> > +/* { dg-do compile } */
> > +/* { dg-options "-O2 -fdump-tree-dse-details" } */
> > +
> > +#include <string.h>
> > +
> > +void blahd (double *);
> > +
> > +void fubar ()
> > +{
> > + double d;
> > + double *x = &d;
> > +
> > + memset (&d, 0 , sizeof d);
> > + *x = 0.0;
> > + blahd (x);
> > +}
> > +
> > +/* { dg-final { scan-tree-dump-times "Deleted redundant store" 1 "dse1"} }
> > */
> > diff --git gcc/tree-ssa-dse.c gcc/tree-ssa-dse.c
> > index 5b7c4fc6d1a..14b66228e1e 100644
> > --- gcc/tree-ssa-dse.c
> > +++ gcc/tree-ssa-dse.c
> > @@ -628,11 +628,7 @@ dse_optimize_redundant_stores (gimple *stmt)
> > tree fndecl;
> > if ((is_gimple_assign (use_stmt)
> > && gimple_vdef (use_stmt)
> > - && ((gimple_assign_rhs_code (use_stmt) == CONSTRUCTOR
> > - && CONSTRUCTOR_NELTS (gimple_assign_rhs1 (use_stmt)) == 0
> > - && !gimple_clobber_p (stmt))
> > - || (gimple_assign_rhs_code (use_stmt) == INTEGER_CST
> > - && integer_zerop (gimple_assign_rhs1 (use_stmt)))))
> > + && initializer_zerop (gimple_assign_rhs1 (use_stmt)))
> So I think we over-simplified here. All we know is that USE_STMT is a
> gimple assignment with virtual operands. We don't know what the form of
> the right hand side of the assignment is. You extract the first operand
> of the right hand side and check if that is zero. But you're losing the
> operator.
>
> This may work in practice because of the limitations imposed by having a
> virtual definition, but it seems fragile in the long run. I should have
> caught this the first time I looked at the patch, sorry.
>
> I think you can guard the call to initializer_zerop with something like
>
> (gimple_assign_single_p (use_stmt)
> && initializer_zero_p (gimple_assign_rhs1 (use_stmt)))
>
> That will restrict the form of the right hand side a bit, but will still
> allow us to capture anything handled by initializer_zero_p.
>
> > || (gimple_call_builtin_p (use_stmt, BUILT_IN_NORMAL)
> > && (fndecl = gimple_call_fndecl (use_stmt)) != NULL
> > && (DECL_FUNCTION_CODE (fndecl) == BUILT_IN_MEMSET
> > @@ -1027,16 +1023,10 @@ dse_dom_walker::dse_optimize_stmt
> > (gimple_stmt_iterator *gsi)
> > {
> > bool by_clobber_p = false;
> >
> > - /* First see if this store is a CONSTRUCTOR and if there
> > - are subsequent CONSTRUCTOR stores which are totally
> > - subsumed by this statement. If so remove the subsequent
> > - CONSTRUCTOR store.
> > -
> > - This will tend to make fewer calls into memset with longer
> > - arguments. */
> > - if (gimple_assign_rhs_code (stmt) == CONSTRUCTOR
> > - && CONSTRUCTOR_NELTS (gimple_assign_rhs1 (stmt)) == 0
> > - && !gimple_clobber_p (stmt))
> > + /* Check if this statement stores zero to a memory location,
> > + and if there is a subsequent store of zero to the same
> > + memory location. If so, remove the subsequent store. */
> > + if (initializer_zerop (gimple_assign_rhs1 (stmt)))
> > dse_optimize_redundant_stores (stmt);
> Same here.
>
> Jeff
Bootstrapped/regtested on x86_64-linux, ok for trunk?
2019-08-19 Matthew Beliveau <mbeli...@redhat.com>
* tree-ssa-dse.c (dse_optimize_redundant_stores): Improved check to
catch more redundant zero initialization cases.
(dse_dom_walker::dse_optimize_stmt): Likewise.
* gcc.dg/tree-ssa/redundant-assign-zero-1.c: New test.
* gcc.dg/tree-ssa/redundant-assign-zero-2.c: New test.
diff --git gcc/testsuite/gcc.dg/tree-ssa/redundant-assign-zero-1.c gcc/testsuite/gcc.dg/tree-ssa/redundant-assign-zero-1.c
new file mode 100644
index 00000000000..b8d01d1644b
--- /dev/null
+++ gcc/testsuite/gcc.dg/tree-ssa/redundant-assign-zero-1.c
@@ -0,0 +1,13 @@
+/* { dg-do compile } */
+/* { dg-options "-O2 -fdump-tree-dse-details" } */
+
+void blah (char *);
+
+void bar ()
+{
+ char a[256] = "";
+ a[3] = 0;
+ blah (a);
+}
+
+/* { dg-final { scan-tree-dump-times "Deleted redundant store" 1 "dse1"} } */
diff --git gcc/testsuite/gcc.dg/tree-ssa/redundant-assign-zero-2.c gcc/testsuite/gcc.dg/tree-ssa/redundant-assign-zero-2.c
new file mode 100644
index 00000000000..8cefa6f0cb7
--- /dev/null
+++ gcc/testsuite/gcc.dg/tree-ssa/redundant-assign-zero-2.c
@@ -0,0 +1,18 @@
+/* { dg-do compile } */
+/* { dg-options "-O2 -fdump-tree-dse-details" } */
+
+#include <string.h>
+
+void blahd (double *);
+
+void fubar ()
+{
+ double d;
+ double *x = &d;
+
+ memset (&d, 0 , sizeof d);
+ *x = 0.0;
+ blahd (x);
+}
+
+/* { dg-final { scan-tree-dump-times "Deleted redundant store" 1 "dse1"} } */
diff --git gcc/tree-ssa-dse.c gcc/tree-ssa-dse.c
index 5b7c4fc6d1a..ba67884a825 100644
--- gcc/tree-ssa-dse.c
+++ gcc/tree-ssa-dse.c
@@ -628,11 +628,8 @@ dse_optimize_redundant_stores (gimple *stmt)
tree fndecl;
if ((is_gimple_assign (use_stmt)
&& gimple_vdef (use_stmt)
- && ((gimple_assign_rhs_code (use_stmt) == CONSTRUCTOR
- && CONSTRUCTOR_NELTS (gimple_assign_rhs1 (use_stmt)) == 0
- && !gimple_clobber_p (stmt))
- || (gimple_assign_rhs_code (use_stmt) == INTEGER_CST
- && integer_zerop (gimple_assign_rhs1 (use_stmt)))))
+ && (gimple_assign_single_p (use_stmt)
+ && initializer_zerop (gimple_assign_rhs1 (use_stmt))))
|| (gimple_call_builtin_p (use_stmt, BUILT_IN_NORMAL)
&& (fndecl = gimple_call_fndecl (use_stmt)) != NULL
&& (DECL_FUNCTION_CODE (fndecl) == BUILT_IN_MEMSET
@@ -1027,16 +1024,11 @@ dse_dom_walker::dse_optimize_stmt (gimple_stmt_iterator *gsi)
{
bool by_clobber_p = false;
- /* First see if this store is a CONSTRUCTOR and if there
- are subsequent CONSTRUCTOR stores which are totally
- subsumed by this statement. If so remove the subsequent
- CONSTRUCTOR store.
-
- This will tend to make fewer calls into memset with longer
- arguments. */
- if (gimple_assign_rhs_code (stmt) == CONSTRUCTOR
- && CONSTRUCTOR_NELTS (gimple_assign_rhs1 (stmt)) == 0
- && !gimple_clobber_p (stmt))
+ /* Check if this statement stores zero to a memory location,
+ and if there is a subsequent store of zero to the same
+ memory location. If so, remove the subsequent store. */
+ if (gimple_assign_single_p (stmt)
+ && initializer_zerop (gimple_assign_rhs1 (stmt)))
dse_optimize_redundant_stores (stmt);
/* Self-assignments are zombies. */
