Message ID | CA+=Sn1kDzzsBvndz64W=VTputQivU5QmUW67r-=0p-g2310P6A@mail.gmail.com |
---|---|
State | New |
Headers | show |
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
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
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" } } */