Patchwork Fix PR 54494, removal of volatile store in strlen optimization

login
register
mail settings
Submitter Andrew Pinski
Date Sept. 5, 2012, 9:10 p.m.
Message ID <CA+=Sn1kDzzsBvndz64W=VTputQivU5QmUW67r-=0p-g2310P6A@mail.gmail.com>
Download mbox | patch
Permalink /patch/181957/
State New
Headers show

Comments

Andrew Pinski - Sept. 5, 2012, 9:10 p.m.
Hi,
  The problem here is the strlen optimization tries to remove a null
character store as we already have done it but it does it for a
volatile store which is not a valid thing to do.
This patch fixes the problem by ignoring statements which have
volatile operands.

OK?  Bootstrapped and tested on x86_64-linux-gnu with no regressions.

Thanks,
Andrew Pinski

 ChangeLog:
* tree-ssa-strlen.c (strlen_optimize_stmt): Don't look at statements
which have volatile operands.

testsuite/ChangeLog:
* gcc.dg/tree-ssa/strlen-1.c: New testcase.
Jakub Jelinek - Sept. 5, 2012, 9:36 p.m.
On Wed, Sep 05, 2012 at 02:10:03PM -0700, Andrew Pinski wrote:
>   The problem here is the strlen optimization tries to remove a null
> character store as we already have done it but it does it for a
> volatile store which is not a valid thing to do.
> This patch fixes the problem by ignoring statements which have
> volatile operands.

That should be caught by the !TREE_SIDE_EFFECTS (lhs) check a few lines
later.  Isn't the bug instead that remap_gimple_op_r copies over
TREE_THIS_VOLATILE flag, but doesn't copy over TREE_SIDE_EFFECTS?

> * tree-ssa-strlen.c (strlen_optimize_stmt): Don't look at statements
> which have volatile operands.
> 
> testsuite/ChangeLog:
> * gcc.dg/tree-ssa/strlen-1.c: New testcase.

> Index: gcc/tree-ssa-strlen.c
> ===================================================================
> --- gcc/tree-ssa-strlen.c	(revision 190993)
> +++ gcc/tree-ssa-strlen.c	(working copy)
> @@ -1782,7 +1782,8 @@ strlen_optimize_stmt (gimple_stmt_iterat
>  	    break;
>  	  }
>      }
> -  else if (is_gimple_assign (stmt))
> +  else if (is_gimple_assign (stmt)
> +	   && !gimple_has_volatile_ops (stmt))
>      {
>        tree lhs = gimple_assign_lhs (stmt);
>  
> Index: gcc/testsuite/gcc.dg/tree-ssa/strlen-1.c
> ===================================================================
> --- gcc/testsuite/gcc.dg/tree-ssa/strlen-1.c	(revision 0)
> +++ gcc/testsuite/gcc.dg/tree-ssa/strlen-1.c	(revision 0)
> @@ -0,0 +1,17 @@
> +/* { dg-do compile } */
> +/* { dg-options "-O2 -fdump-tree-optimized" } */
> +extern const unsigned long base;
> +static inline void wreg(unsigned char val, unsigned long addr) __attribute__((always_inline));
> +static inline void wreg(unsigned char val, unsigned long addr)
> +{
> +   *((volatile unsigned char *) (__SIZE_TYPE__) (base + addr)) = val;
> +}
> +void wreg_twice(void)
> +{
> +   wreg(0, 42);
> +   wreg(0, 42);
> +}
> +
> +/* We should not remove the second null character store to (base+42) address. */
> +/* { dg-final { scan-tree-dump-times " ={v} 0;" 2 "optimized" } }  */
> +/* { dg-final { cleanup-tree-dump "optimized" } }  */


	Jakub
Andrew Pinski - Sept. 5, 2012, 10:34 p.m.
On Wed, Sep 5, 2012 at 2:36 PM, Jakub Jelinek <jakub@redhat.com> wrote:
> On Wed, Sep 05, 2012 at 02:10:03PM -0700, Andrew Pinski wrote:
>>   The problem here is the strlen optimization tries to remove a null
>> character store as we already have done it but it does it for a
>> volatile store which is not a valid thing to do.
>> This patch fixes the problem by ignoring statements which have
>> volatile operands.
>
> That should be caught by the !TREE_SIDE_EFFECTS (lhs) check a few lines
> later.  Isn't the bug instead that remap_gimple_op_r copies over
> TREE_THIS_VOLATILE flag, but doesn't copy over TREE_SIDE_EFFECTS?

Yes it should be doing the copy.  In fact I have touched this area before.
I will submit a new patch.

Thanks,
Andrew Pinski


>
>> * tree-ssa-strlen.c (strlen_optimize_stmt): Don't look at statements
>> which have volatile operands.
>>
>> testsuite/ChangeLog:
>> * gcc.dg/tree-ssa/strlen-1.c: New testcase.
>
>> Index: gcc/tree-ssa-strlen.c
>> ===================================================================
>> --- gcc/tree-ssa-strlen.c     (revision 190993)
>> +++ gcc/tree-ssa-strlen.c     (working copy)
>> @@ -1782,7 +1782,8 @@ strlen_optimize_stmt (gimple_stmt_iterat
>>           break;
>>         }
>>      }
>> -  else if (is_gimple_assign (stmt))
>> +  else if (is_gimple_assign (stmt)
>> +        && !gimple_has_volatile_ops (stmt))
>>      {
>>        tree lhs = gimple_assign_lhs (stmt);
>>
>> Index: gcc/testsuite/gcc.dg/tree-ssa/strlen-1.c
>> ===================================================================
>> --- gcc/testsuite/gcc.dg/tree-ssa/strlen-1.c  (revision 0)
>> +++ gcc/testsuite/gcc.dg/tree-ssa/strlen-1.c  (revision 0)
>> @@ -0,0 +1,17 @@
>> +/* { dg-do compile } */
>> +/* { dg-options "-O2 -fdump-tree-optimized" } */
>> +extern const unsigned long base;
>> +static inline void wreg(unsigned char val, unsigned long addr) __attribute__((always_inline));
>> +static inline void wreg(unsigned char val, unsigned long addr)
>> +{
>> +   *((volatile unsigned char *) (__SIZE_TYPE__) (base + addr)) = val;
>> +}
>> +void wreg_twice(void)
>> +{
>> +   wreg(0, 42);
>> +   wreg(0, 42);
>> +}
>> +
>> +/* We should not remove the second null character store to (base+42) address. */
>> +/* { dg-final { scan-tree-dump-times " ={v} 0;" 2 "optimized" } }  */
>> +/* { dg-final { cleanup-tree-dump "optimized" } }  */
>
>
>         Jakub

Patch

Index: gcc/tree-ssa-strlen.c
===================================================================
--- gcc/tree-ssa-strlen.c	(revision 190993)
+++ gcc/tree-ssa-strlen.c	(working copy)
@@ -1782,7 +1782,8 @@  strlen_optimize_stmt (gimple_stmt_iterat
 	    break;
 	  }
     }
-  else if (is_gimple_assign (stmt))
+  else if (is_gimple_assign (stmt)
+	   && !gimple_has_volatile_ops (stmt))
     {
       tree lhs = gimple_assign_lhs (stmt);
 
Index: gcc/testsuite/gcc.dg/tree-ssa/strlen-1.c
===================================================================
--- gcc/testsuite/gcc.dg/tree-ssa/strlen-1.c	(revision 0)
+++ gcc/testsuite/gcc.dg/tree-ssa/strlen-1.c	(revision 0)
@@ -0,0 +1,17 @@ 
+/* { dg-do compile } */
+/* { dg-options "-O2 -fdump-tree-optimized" } */
+extern const unsigned long base;
+static inline void wreg(unsigned char val, unsigned long addr) __attribute__((always_inline));
+static inline void wreg(unsigned char val, unsigned long addr)
+{
+   *((volatile unsigned char *) (__SIZE_TYPE__) (base + addr)) = val;
+}
+void wreg_twice(void)
+{
+   wreg(0, 42);
+   wreg(0, 42);
+}
+
+/* We should not remove the second null character store to (base+42) address. */
+/* { dg-final { scan-tree-dump-times " ={v} 0;" 2 "optimized" } }  */
+/* { dg-final { cleanup-tree-dump "optimized" } }  */