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

login
register
mail settings
Submitter Andrew Pinski
Date Sept. 6, 2012, 4:10 a.m.
Message ID <CA+=Sn1=Tw68CwdNrNSwUo57hme7M6wEVf9fZq0B-4Egunmu-Ww@mail.gmail.com>
Download mbox | patch
Permalink /patch/182002/
State New
Headers show

Comments

Andrew Pinski - Sept. 6, 2012, 4:10 a.m.
Hi,
  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.
Jakub Jelinek - Sept. 6, 2012, 7:07 a.m.
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.

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

Patch

Index: testsuite/gcc.c-torture/compile/pr49474.c
===================================================================
--- testsuite/gcc.c-torture/compile/pr49474.c	(revision 0)
+++ testsuite/gcc.c-torture/compile/pr49474.c	(revision 0)
@@ -0,0 +1,16 @@ 
+typedef struct gfc_formal_arglist
+{
+  int next;
+}
+gfc_actual_arglist;
+update_arglist_pass (gfc_actual_arglist* lst, int po, unsigned argpos,
+       const char *name)
+{
+  ((void)(__builtin_expect(!(argpos > 0), 0) ? __builtin_unreachable(), 0 : 0));
+  if (argpos == 1)
+      return 0;
+  if (lst)
+    lst->next = update_arglist_pass (lst->next, po, argpos - 1, name);
+  else
+    lst = update_arglist_pass (((void *)0), po, argpos - 1, name);
+}
Index: cprop.c
===================================================================
--- cprop.c	(revision 176187)
+++ cprop.c	(working copy)
@@ -1332,7 +1332,7 @@  find_implicit_sets (void)
   FOR_EACH_BB (bb)
     {
       /* Check for more than one successor.  */
-      if (! EDGE_COUNT (bb->succs) > 1)
+      if (EDGE_COUNT (bb->succs) <= 1)
 	continue;
 
       cond = fis_get_condition (BB_END (bb));