[PATCH] Improve DSE to handle redundant zero initializations.

[Jeff Law]

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