Improve DSE to handle redundant zero initializations.

Message ID CAOrE4X0EB-sq3FeoARm1tCfsXuL6TwqkPhDRQrt9zQrr4Hnfhg@mail.gmail.com
State New
Headers show
Series
  • Improve DSE to handle redundant zero initializations.
Related show

Commit Message

Matthew Beliveau Aug. 12, 2019, 8:14 p.m.
This patch improves DSE to handle missing optimizations for zero
initializations.

Thank you,
Matthew Beliveau

Comments

Marek Polacek Aug. 12, 2019, 8:19 p.m. | #1
I have no comments on the patch itself, but...

On Mon, Aug 12, 2019 at 04:14:40PM -0400, Matthew Beliveau wrote:
> 2019-08-12  Matthew Beliveau  <mbelivea@redhat.com>

> 

> 	PR c++/DSE.patch


Please drop this.

> 	* tree-ssa-dse.c (dse_optimize_redundant_stores): Improved check to

> 	catch more redundant zero initalization cases.

> 	(dse_dom_walker::dse_optimize_stmt): Improved check to catch more

> 	redundant zero initialization cases.


The second entry should just say "Likewise."

Also, no need to CC Jason on non-C++ patches.

Marek
Jeff Law Aug. 12, 2019, 11:04 p.m. | #2
On 8/12/19 2:14 PM, Matthew Beliveau wrote:
> This patch improves DSE to handle missing optimizations for zero

> initializations.

> 

> Thank you,

> Matthew Beliveau

> 

> 

> DSE.patch

> 

> Bootstrapped/regtested on x86_64-linux, ok for trunk?

> 

> 2019-08-12  Matthew Beliveau  <mbelivea@redhat.com>

> 

> 	PR c++/DSE.patch

> 	* tree-ssa-dse.c (dse_optimize_redundant_stores): Improved check to

> 	catch more redundant zero initalization cases.

> 	(dse_dom_walker::dse_optimize_stmt): Improved check to catch more

> 	redundant zero initialization cases.

> 

> 	* gcc.dg/tree-ssa/redundant-assign-zero-1.c: New test.

> 	* gcc.dg/tree-ssa/redundant-assign-zero-2.c: New test.

> 

> @@ -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.

Perhaps...

/* 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 delete the subsequent store.  */


With the ChangeLog nits noted by Marek and the comment fix this is fine
for the trunk.

jeff
Richard Sandiford Aug. 13, 2019, 8:18 a.m. | #3
Thanks for doing this.

Matthew Beliveau <mbelivea@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).

Richard
Richard Biener Aug. 13, 2019, 9:14 a.m. | #4
On Tue, Aug 13, 2019 at 10:18 AM Richard Sandiford
<richard.sandiford@arm.com> wrote:
>

> Thanks for doing this.

>

> Matthew Beliveau <mbelivea@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
Matthew Beliveau Aug. 13, 2019, 3:09 p.m. | #5
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.guenther@gmail.com> wrote:
>

> On Tue, Aug 13, 2019 at 10:18 AM Richard Sandiford

> <richard.sandiford@arm.com> wrote:

> >

> > Thanks for doing this.

> >

> > Matthew Beliveau <mbelivea@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
Bootstrapped/regtested on x86_64-linux, ok for trunk?

2019-08-13  Matthew Beliveau  <mbelivea@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)))
 	  || (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);
 
       /* Self-assignments are zombies.  */
Jeff Law Aug. 16, 2019, 4:30 p.m. | #6
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.guenther@gmail.com> wrote:

>> On Tue, Aug 13, 2019 at 10:18 AM Richard Sandiford

>> <richard.sandiford@arm.com> wrote:

>>> Thanks for doing this.

>>>

>>> Matthew Beliveau <mbelivea@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  <mbelivea@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
Matthew Beliveau Aug. 19, 2019, 7:53 p.m. | #7
Hello,

This should have the changes you wanted!

Thank you,
Matthew Beliveau

On Fri, Aug 16, 2019 at 12:30 PM Jeff Law <law@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.guenther@gmail.com> wrote:

> >> On Tue, Aug 13, 2019 at 10:18 AM Richard Sandiford

> >> <richard.sandiford@arm.com> wrote:

> >>> Thanks for doing this.

> >>>

> >>> Matthew Beliveau <mbelivea@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  <mbelivea@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  <mbelivea@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.  */
Jeff Law Aug. 19, 2019, 10:54 p.m. | #8
On 8/19/19 1:53 PM, Matthew Beliveau wrote:

>> Jeff

> 

> DSE-3.patch

> 

> Bootstrapped/regtested on x86_64-linux, ok for trunk?

> 

> 2019-08-19  Matthew Beliveau  <mbelivea@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.

OK for the trunk.  THanks for your patience.

Jeff

Patch

Bootstrapped/regtested on x86_64-linux, ok for trunk?

2019-08-12  Matthew Beliveau  <mbelivea@redhat.com>

	PR c++/DSE.patch
	* tree-ssa-dse.c (dse_optimize_redundant_stores): Improved check to
	catch more redundant zero initalization cases.
	(dse_dom_walker::dse_optimize_stmt): Improved check to catch more
	redundant zero initialization cases.

	* 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..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);