On Thu, Sep 6, 2012 at 12:07 AM, Jakub Jelinek <ja...@redhat.com> wrote: > On Wed, Sep 05, 2012 at 09:10:53PM -0700, Andrew Pinski wrote: >> The inlininer likes to recreate some MEM_REF, it copies most of the >> bits (TREE_THIS_NOTRAP, TREE_THIS_VOLATILE, etc.) but forgets about >> TREE_SIDE_EFFECTS. This causes the strlen optimization to think the >> memory store does not have a side effects. >> >> OK? Bootstrapped and tested on x86_64-linux-gnu with no regressions. >> >> Thanks, >> Andrew Pinski >> >> ChangeLog: >> >> * tree-inline.c (remap_gimple_op_r): Copy TREE_SIDE_EFFECTS also. >> >> testsuite/ChangeLog: >> * gcc.dg/tree-ssa/strlen-1.c: New testcase. > > Patch preapproved, but you've attached a different patch.
Sorry about that. Here is the correct one. Also is this ok for the 4.7 branch? Thanks, Andrew Pinski > > I'd say copy_tree_body_r's MEM_REF handling should also copy > TREE_SIDE_EFFECTS/TREE_THIS_NOTRAP (what about TREE_READONLY?), > maybe copy_decl_to_var/copy_result_decl_to_var should also > copy TREE_SIDE_EFFECTS, perhaps omp-low.c copy_var_decl, install_var_field, > tree-nested.c lookup_field_for_decl, tree-sra.c sra_ipa_reset_debug_stmts > (grep TREE_THIS_VOLATILE.*TREE_THIS_VOLATILE, looking for missing > TREE_SIDE_EFFECTS copy nearby). That said, perhaps the tree-ssa-strlen.c > change is desirable too, unless we add some checking that TREE_THIS_VOLATILE > references/decls have TREE_SIDE_EFFECTS bit set in the IL. > >> --- testsuite/gcc.c-torture/compile/pr49474.c (revision 0) >> +++ testsuite/gcc.c-torture/compile/pr49474.c (revision 0) >> --- cprop.c (revision 176187) >> +++ cprop.c (working copy) > > Jakub
Index: testsuite/gcc.dg/tree-ssa/strlen-1.c =================================================================== --- testsuite/gcc.dg/tree-ssa/strlen-1.c (revision 0) +++ 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" } } */ Index: tree-inline.c =================================================================== --- tree-inline.c (revision 191004) +++ tree-inline.c (working copy) @@ -848,6 +848,7 @@ remap_gimple_op_r (tree *tp, int *walk_s ptr, TREE_OPERAND (*tp, 1)); TREE_THIS_NOTRAP (*tp) = TREE_THIS_NOTRAP (old); TREE_THIS_VOLATILE (*tp) = TREE_THIS_VOLATILE (old); + TREE_SIDE_EFFECTS (*tp) = TREE_SIDE_EFFECTS (old); TREE_NO_WARNING (*tp) = TREE_NO_WARNING (old); *walk_subtrees = 0; return NULL;