[PATCH] Copy over TREE_THIS_VOLATILE during forwprop (PR tree-optimization/50078)

2011-11-28 Thread Jakub Jelinek
Hi!

On the following testcase the volatile load/store are optimized away.
IMHO this is a bug in forwprop, which replaces
  tmp_Y = nonvolvar[arg_X];
  MEM[(volatile ...*)tmp_Y] ={v} ...;
with
  MEM[(volatile ...*)nonvolvar][tmp_Y] ={v} ...;
where the LHS is no longer TREE_THIS_VOLATILE like before,
TREE_THIS_VOLATILE is newly only the MEM_REF operand of the non-volatile
ARRAY_REF.  This patch copies the TREE_THIS_VOLATILE bit to the ARRAY_REF.

Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?

2011-11-28  Jakub Jelinek  ja...@redhat.com

PR tree-optimization/50078
* tree-ssa-forwprop.c (forward_propagate_addr_expr_1): Copy over
TREE_THIS_VOLATILE also from the old to new lhs resp. rhs.

* gcc.dg/pr50078.c: New test.

--- gcc/tree-ssa-forwprop.c.jj  2011-10-20 14:13:43.0 +0200
+++ gcc/tree-ssa-forwprop.c 2011-11-28 09:47:39.822697552 +0100
@@ -910,7 +910,7 @@ forward_propagate_addr_expr_1 (tree name
 TREE_TYPE (gimple_assign_rhs1 (use_stmt
{
  tree *def_rhs_basep = TREE_OPERAND (def_rhs, 0);
- tree new_offset, new_base, saved;
+ tree new_offset, new_base, saved, new_lhs;
  while (handled_component_p (*def_rhs_basep))
def_rhs_basep = TREE_OPERAND (*def_rhs_basep, 0);
  saved = *def_rhs_basep;
@@ -930,8 +930,9 @@ forward_propagate_addr_expr_1 (tree name
   new_base, new_offset);
  TREE_THIS_VOLATILE (*def_rhs_basep) = TREE_THIS_VOLATILE (lhs);
  TREE_THIS_NOTRAP (*def_rhs_basep) = TREE_THIS_NOTRAP (lhs);
- gimple_assign_set_lhs (use_stmt,
-unshare_expr (TREE_OPERAND (def_rhs, 0)));
+ new_lhs = unshare_expr (TREE_OPERAND (def_rhs, 0));
+ gimple_assign_set_lhs (use_stmt, new_lhs);
+ TREE_THIS_VOLATILE (new_lhs) = TREE_THIS_VOLATILE (lhs);
  *def_rhs_basep = saved;
  tidy_after_forward_propagate_addr (use_stmt);
  /* Continue propagating into the RHS if this was not the
@@ -991,7 +992,7 @@ forward_propagate_addr_expr_1 (tree name
 TREE_TYPE (TREE_OPERAND (def_rhs, 0
{
  tree *def_rhs_basep = TREE_OPERAND (def_rhs, 0);
- tree new_offset, new_base, saved;
+ tree new_offset, new_base, saved, new_rhs;
  while (handled_component_p (*def_rhs_basep))
def_rhs_basep = TREE_OPERAND (*def_rhs_basep, 0);
  saved = *def_rhs_basep;
@@ -1011,8 +1012,9 @@ forward_propagate_addr_expr_1 (tree name
   new_base, new_offset);
  TREE_THIS_VOLATILE (*def_rhs_basep) = TREE_THIS_VOLATILE (rhs);
  TREE_THIS_NOTRAP (*def_rhs_basep) = TREE_THIS_NOTRAP (rhs);
- gimple_assign_set_rhs1 (use_stmt,
- unshare_expr (TREE_OPERAND (def_rhs, 0)));
+ new_rhs = unshare_expr (TREE_OPERAND (def_rhs, 0));
+ gimple_assign_set_rhs1 (use_stmt, new_rhs);
+ TREE_THIS_VOLATILE (new_rhs) = TREE_THIS_VOLATILE (rhs);
  *def_rhs_basep = saved;
  fold_stmt_inplace (use_stmt_gsi);
  tidy_after_forward_propagate_addr (use_stmt);
--- gcc/testsuite/gcc.dg/pr50078.c.jj   2011-11-28 10:03:18.020091603 +0100
+++ gcc/testsuite/gcc.dg/pr50078.c  2011-11-28 10:05:01.533389187 +0100
@@ -0,0 +1,14 @@
+/* PR tree-optimization/50078 */
+/* { dg-do compile } */
+/* { dg-options -O2 } */
+
+unsigned nonvolvar[2];
+
+void
+test (int arg)
+{
+  unsigned v = *(volatile unsigned *) (nonvolvar[arg]);
+  *(volatile unsigned *) (nonvolvar[arg]) = v;
+}
+
+/* { dg-final { scan-assembler-times movl\[^\n\r\]*nonvolvar 2 { target { { 
i?86-*-* x86_64-*-* }  nonpic } } } } */

Jakub


Re: [PATCH] Copy over TREE_THIS_VOLATILE during forwprop (PR tree-optimization/50078)

2011-11-28 Thread Jeff Law
-BEGIN PGP SIGNED MESSAGE-
Hash: SHA1

On 11/28/11 11:36, Jakub Jelinek wrote:
 Hi!
 
 On the following testcase the volatile load/store are optimized
 away. IMHO this is a bug in forwprop, which replaces tmp_Y =
 nonvolvar[arg_X]; MEM[(volatile ...*)tmp_Y] ={v} ...; with 
 MEM[(volatile ...*)nonvolvar][tmp_Y] ={v} ...; where the LHS is no
 longer TREE_THIS_VOLATILE like before, TREE_THIS_VOLATILE is newly
 only the MEM_REF operand of the non-volatile ARRAY_REF.  This patch
 copies the TREE_THIS_VOLATILE bit to the ARRAY_REF.
 
 Bootstrapped/regtested on x86_64-linux and i686-linux, ok for
 trunk?
 
 2011-11-28  Jakub Jelinek  ja...@redhat.com
 
 PR tree-optimization/50078 * tree-ssa-forwprop.c
 (forward_propagate_addr_expr_1): Copy over TREE_THIS_VOLATILE also
 from the old to new lhs resp. rhs.
 
 * gcc.dg/pr50078.c: New test.
Do we need to copy NOTRAP or any of the other flags?

OK, assuming we don't need to copy NOTRAP, etc.  If those need to be
copied too, preapproved with those changes.

Jeff
-BEGIN PGP SIGNATURE-
Version: GnuPG v1.4.11 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iQEcBAEBAgAGBQJO094dAAoJEBRtltQi2kC7Z+UH+QHq/yj9zX6wZz3cfTgZ4ZyR
oWQypCXYUgoIBYrX+rWD91dzI/R+2Zy2F6u5zBOtlzrmNTLbQcQt3PL2fzhz76/L
0+lZ8c8kTwh0lvmTI7/LPQXf0o7Zli43xteOSADglFVVGcWIPus/honBmP3dqkV+
O1n6WZq95eEZVl8HciZOQKANz+yrcVyMi6u/8WioGdn/3Sl9YodpN2R0kuSr++5V
//4XYO1crzMNqqjkxLTJNRNZiV0XH/WwiJx9MOoCKDMooBo7MNbDPtIcfeSbddIl
GhqPEvwK+xBWyOatJMFsyRVlFvcWCfdKGBE//B7+m9ml7eS5pVe/kngphbvA6RI=
=31Lf
-END PGP SIGNATURE-


Re: [PATCH] Copy over TREE_THIS_VOLATILE during forwprop (PR tree-optimization/50078)

2011-11-28 Thread Jakub Jelinek
On Mon, Nov 28, 2011 at 12:16:45PM -0700, Jeff Law wrote:
  On the following testcase the volatile load/store are optimized
  away. IMHO this is a bug in forwprop, which replaces tmp_Y =
  nonvolvar[arg_X]; MEM[(volatile ...*)tmp_Y] ={v} ...; with 
  MEM[(volatile ...*)nonvolvar][tmp_Y] ={v} ...; where the LHS is no
  longer TREE_THIS_VOLATILE like before, TREE_THIS_VOLATILE is newly
  only the MEM_REF operand of the non-volatile ARRAY_REF.  This patch
  copies the TREE_THIS_VOLATILE bit to the ARRAY_REF.
  
  Bootstrapped/regtested on x86_64-linux and i686-linux, ok for
  trunk?
  
  2011-11-28  Jakub Jelinek  ja...@redhat.com
  
  PR tree-optimization/50078 * tree-ssa-forwprop.c
  (forward_propagate_addr_expr_1): Copy over TREE_THIS_VOLATILE also
  from the old to new lhs resp. rhs.
  
  * gcc.dg/pr50078.c: New test.
 Do we need to copy NOTRAP or any of the other flags?
 
 OK, assuming we don't need to copy NOTRAP, etc.  If those need to be
 copied too, preapproved with those changes.

I had there TREE_THIS_NOTRAP as well in the first iteration, but
it ICEd, as TREE_THIS_NOTRAP is not valid e.g. on COMPONENT_REF.

Jakub