diff mbox

Fix PR 54494, removal of volatile store in strlen optimization because of the inlininer

Message ID CA+=Sn1m6=h+OrmXhUe7CocOtW4n9CJTUNi3xu+XBjSOJfAmMVg@mail.gmail.com
State New
Headers show

Commit Message

Andrew Pinski Sept. 6, 2012, 7:50 a.m. UTC 
On Thu, Sep 6, 2012 at 12:07 AM, Jakub Jelinek <jakub@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

Comments

Jakub Jelinek Sept. 6, 2012, 8:12 a.m. UTC | #1 
On Thu, Sep 06, 2012 at 12:50:44AM -0700, Andrew Pinski wrote:
> > 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?

Yes, thanks.

> --- 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;


	Jakub
diff mbox

Patch

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;