tree-optimization/93354 FRE redundant store removal validity fix
diff mbox series

Message ID nycvar.YFH.7.76.2001221451211.5566@zhemvz.fhfr.qr
State New
Headers show
Series
  • tree-optimization/93354 FRE redundant store removal validity fix
Related show

Commit Message

Richard Biener Jan. 22, 2020, 1:52 p.m. UTC
This fixes the validity check for redundant store removal by looking
at the actual stmt that represents the last (relevant) change to
the TBAA state rather than the imperfectly tracked alias set in the
expression hash table which then becomes unused (see a followup change
to get rid of that).

On the way this allows re-enabling of VN_WALKREWRITE, fixing PR91123.

This also exposes an out-of-bound array access in 
real.c:clear_significand_below fixed as well.

Bootstrapped on x86_64-unknown-linux-gnu, testing in progress.

Richard.

2020-01-22  Richard Biener  <rguenther@suse.de>

	PR tree-optimization/91123
	PR tree-optimization/93381
	* tree-ssa-sccvn.c (store_affects_tbaa_memory_state): New function.
	(visit_reference_op_store): Use it to check validity of eliding
	redundant stores.
	(eliminate_dom_walker::eliminate_stmt): Likewise, allow lookup
	to look through aggregate copies.
	(vn_reference_lookup): Consistently set *last_use_ptr.
	* real.c (clear_significand_below): Fix out-of-bound access.

	* gcc.dg/torture/pr93354.c: New testcase.
	* gcc.dg/tree-ssa/ssa-fre-85.c: Likewise.

Comments

Richard Biener Jan. 23, 2020, 11:42 a.m. UTC | #1
On Wed, 22 Jan 2020, Richard Biener wrote:

> 
> This fixes the validity check for redundant store removal by looking
> at the actual stmt that represents the last (relevant) change to
> the TBAA state rather than the imperfectly tracked alias set in the
> expression hash table which then becomes unused (see a followup change
> to get rid of that).
> 
> On the way this allows re-enabling of VN_WALKREWRITE, fixing PR91123.
> 
> This also exposes an out-of-bound array access in 
> real.c:clear_significand_below fixed as well.
> 
> Bootstrapped on x86_64-unknown-linux-gnu, testing in progress.

So this didn't work out.  The following addresses this in a different
way, postponing a fix for PR91123 which needs another adjustment.

Bootstrapped on x86_64-unknown-linux-gnu, pushed.


This fixes tracking of the alias-set of partial defs for use by
redundant store removal.

2020-01-23  Richard Biener  <rguenther@suse.de>

	PR tree-optimization/93381
	* tree-ssa-sccvn.c (vn_walk_cb_data::push_partial_def): Take
	alias-set of the def as argument and record the first one.
	(vn_walk_cb_data::first_set): New member.
	(vn_reference_lookup_3): Pass the alias-set of the current def
	to push_partial_def.  Fix alias-set used in the aggregate copy
	case.
	(vn_reference_lookup): Consistently set *last_vuse_ptr.
	* real.c (clear_significand_below): Fix out-of-bound access.

	* gcc.dg/torture/pr93354.c: New testcase.

diff --git a/gcc/real.c b/gcc/real.c
index a57e5df113f..46d8126efe4 100644
--- a/gcc/real.c
+++ b/gcc/real.c
@@ -420,7 +420,10 @@ clear_significand_below (REAL_VALUE_TYPE *r, unsigned int n)
   for (i = 0; i < w; ++i)
     r->sig[i] = 0;
 
-  r->sig[w] &= ~(((unsigned long)1 << (n % HOST_BITS_PER_LONG)) - 1);
+  /* We are actually passing N == SIGNIFICAND_BITS which would result
+     in an out-of-bound access below.  */
+  if (n % HOST_BITS_PER_LONG != 0)
+    r->sig[w] &= ~(((unsigned long)1 << (n % HOST_BITS_PER_LONG)) - 1);
 }
 
 /* Divide the significands of A and B, placing the result in R.  Return
diff --git a/gcc/testsuite/gcc.dg/torture/pr93354.c b/gcc/testsuite/gcc.dg/torture/pr93354.c
new file mode 100644
index 00000000000..8ccf1e6d2c0
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/torture/pr93354.c
@@ -0,0 +1,22 @@
+/* { dg-do run } */
+
+typedef __INT32_TYPE__ int32_t;
+typedef __INT64_TYPE__ int64_t;
+struct X { int32_t i; int32_t j; };
+void foo (int64_t *z)
+{
+  ((struct X *)z)->i = 0x05060708;
+  ((struct X *)z)->j = 0x01020304;
+  *z = 0x0102030405060708;
+}
+
+int main()
+{
+  int64_t l = 0;
+  int64_t *p;
+  asm ("" : "=r" (p) : "0" (&l));
+  foo (p);
+  if (l != 0x0102030405060708)
+    __builtin_abort ();
+  return 0;
+}
diff --git a/gcc/tree-ssa-sccvn.c b/gcc/tree-ssa-sccvn.c
index 0b8ee586139..6e0b2202157 100644
--- a/gcc/tree-ssa-sccvn.c
+++ b/gcc/tree-ssa-sccvn.c
@@ -1693,7 +1693,8 @@ struct vn_walk_cb_data
      ao_ref_init (&orig_ref, orig_ref_);
    }
   ~vn_walk_cb_data ();
-  void *push_partial_def (const pd_data& pd, tree, HOST_WIDE_INT);
+  void *push_partial_def (const pd_data& pd, tree,
+			  alias_set_type, HOST_WIDE_INT);
 
   vn_reference_t vr;
   ao_ref orig_ref;
@@ -1706,6 +1707,7 @@ struct vn_walk_cb_data
   /* The first defs range to avoid splay tree setup in most cases.  */
   pd_range first_range;
   tree first_vuse;
+  alias_set_type first_set;
   splay_tree known_ranges;
   obstack ranges_obstack;
 };
@@ -1746,13 +1748,13 @@ pd_tree_dealloc (void *, void *)
 }
 
 /* Push PD to the vector of partial definitions returning a
-   value when we are ready to combine things with VUSE and MAXSIZEI,
+   value when we are ready to combine things with VUSE, SET and MAXSIZEI,
    NULL when we want to continue looking for partial defs or -1
    on failure.  */
 
 void *
 vn_walk_cb_data::push_partial_def (const pd_data &pd, tree vuse,
-				   HOST_WIDE_INT maxsizei)
+				   alias_set_type set, HOST_WIDE_INT maxsizei)
 {
   const HOST_WIDE_INT bufsize = 64;
   /* We're using a fixed buffer for encoding so fail early if the object
@@ -1773,6 +1775,7 @@ vn_walk_cb_data::push_partial_def (const pd_data &pd, tree vuse,
       first_range.offset = pd.offset;
       first_range.size = pd.size;
       first_vuse = vuse;
+      first_set = set;
       last_vuse_ptr = NULL;
       /* Continue looking for partial defs.  */
       return NULL;
@@ -1903,10 +1906,10 @@ vn_walk_cb_data::push_partial_def (const pd_data &pd, tree vuse,
       if (dump_file && (dump_flags & TDF_DETAILS))
 	fprintf (dump_file,
 		 "Successfully combined %u partial definitions\n", ndefs);
-      /* ???  If we track partial defs alias-set we could use that if it
-         is the same for all.  Use zero for now.  */
+      /* We are using the alias-set of the first store we encounter which
+	 should be appropriate here.  */
       return vn_reference_lookup_or_insert_for_pieces
-		(first_vuse, 0, vr->type, vr->operands, val);
+		(first_vuse, first_set, vr->type, vr->operands, val);
     }
   else
     {
@@ -2492,7 +2495,7 @@ vn_reference_lookup_3 (ao_ref *ref, tree vuse, void *data_,
 	  pd.rhs = build_constructor (NULL_TREE, NULL);
 	  pd.offset = (offset2i - offseti) / BITS_PER_UNIT;
 	  pd.size = leni;
-	  return data->push_partial_def (pd, vuse, maxsizei);
+	  return data->push_partial_def (pd, vuse, 0, maxsizei);
 	}
     }
 
@@ -2553,7 +2556,8 @@ vn_reference_lookup_3 (ao_ref *ref, tree vuse, void *data_,
 	      pd.rhs = gimple_assign_rhs1 (def_stmt);
 	      pd.offset = (offset2i - offseti) / BITS_PER_UNIT;
 	      pd.size = size2i / BITS_PER_UNIT;
-	      return data->push_partial_def (pd, vuse, maxsizei);
+	      return data->push_partial_def (pd, vuse, get_alias_set (lhs),
+					     maxsizei);
 	    }
 	}
     }
@@ -2665,7 +2669,8 @@ vn_reference_lookup_3 (ao_ref *ref, tree vuse, void *data_,
 	      pd.rhs = rhs;
 	      pd.offset = (offset2i - offseti) / BITS_PER_UNIT;
 	      pd.size = size2i / BITS_PER_UNIT;
-	      return data->push_partial_def (pd, vuse, maxsizei);
+	      return data->push_partial_def (pd, vuse, get_alias_set (lhs),
+					     maxsizei);
 	    }
 	}
     }
@@ -2751,7 +2756,8 @@ vn_reference_lookup_3 (ao_ref *ref, tree vuse, void *data_,
 	      pd.rhs = SSA_VAL (def_rhs);
 	      pd.offset = (offset2i - offseti) / BITS_PER_UNIT;
 	      pd.size = size2i / BITS_PER_UNIT;
-	      return data->push_partial_def (pd, vuse, maxsizei);
+	      return data->push_partial_def (pd, vuse, get_alias_set (lhs),
+					     maxsizei);
 	    }
 	}
     }
@@ -2764,6 +2770,7 @@ vn_reference_lookup_3 (ao_ref *ref, tree vuse, void *data_,
 	       || TREE_CODE (gimple_assign_rhs1 (def_stmt)) == MEM_REF
 	       || handled_component_p (gimple_assign_rhs1 (def_stmt))))
     {
+      tree lhs = gimple_assign_lhs (def_stmt);
       tree base2;
       int i, j, k;
       auto_vec<vn_reference_op_s> rhs;
@@ -2870,7 +2877,7 @@ vn_reference_lookup_3 (ao_ref *ref, tree vuse, void *data_,
 	{
 	  if (data->partial_defs.is_empty ())
 	    return vn_reference_lookup_or_insert_for_pieces
-	      (vuse, get_alias_set (rhs1), vr->type, vr->operands, val);
+	      (vuse, get_alias_set (lhs), vr->type, vr->operands, val);
 	  /* This is the only interesting case for partial-def handling
 	     coming from targets that like to gimplify init-ctors as
 	     aggregate copies from constant data like aarch64 for
@@ -2882,7 +2889,8 @@ vn_reference_lookup_3 (ao_ref *ref, tree vuse, void *data_,
 	      pd.rhs = val;
 	      pd.offset = 0;
 	      pd.size = maxsizei / BITS_PER_UNIT;
-	      return data->push_partial_def (pd, vuse, maxsizei);
+	      return data->push_partial_def (pd, vuse, get_alias_set (lhs),
+					     maxsizei);
 	    }
 	}
 
@@ -3205,6 +3213,8 @@ vn_reference_lookup (tree op, tree vuse, vn_lookup_kind kind,
       return NULL_TREE;
     }
 
+  if (last_vuse_ptr)
+    *last_vuse_ptr = vr1.vuse;
   return vn_reference_lookup_1 (&vr1, vnresult);
 }

Patch
diff mbox series

diff --git a/gcc/real.c b/gcc/real.c
index a57e5df113f..46d8126efe4 100644
--- a/gcc/real.c
+++ b/gcc/real.c
@@ -420,7 +420,10 @@  clear_significand_below (REAL_VALUE_TYPE *r, unsigned int n)
   for (i = 0; i < w; ++i)
     r->sig[i] = 0;
 
-  r->sig[w] &= ~(((unsigned long)1 << (n % HOST_BITS_PER_LONG)) - 1);
+  /* We are actually passing N == SIGNIFICAND_BITS which would result
+     in an out-of-bound access below.  */
+  if (n % HOST_BITS_PER_LONG != 0)
+    r->sig[w] &= ~(((unsigned long)1 << (n % HOST_BITS_PER_LONG)) - 1);
 }
 
 /* Divide the significands of A and B, placing the result in R.  Return
diff --git a/gcc/testsuite/gcc.dg/torture/pr93354.c b/gcc/testsuite/gcc.dg/torture/pr93354.c
new file mode 100644
index 00000000000..8ccf1e6d2c0
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/torture/pr93354.c
@@ -0,0 +1,22 @@ 
+/* { dg-do run } */
+
+typedef __INT32_TYPE__ int32_t;
+typedef __INT64_TYPE__ int64_t;
+struct X { int32_t i; int32_t j; };
+void foo (int64_t *z)
+{
+  ((struct X *)z)->i = 0x05060708;
+  ((struct X *)z)->j = 0x01020304;
+  *z = 0x0102030405060708;
+}
+
+int main()
+{
+  int64_t l = 0;
+  int64_t *p;
+  asm ("" : "=r" (p) : "0" (&l));
+  foo (p);
+  if (l != 0x0102030405060708)
+    __builtin_abort ();
+  return 0;
+}
diff --git a/gcc/testsuite/gcc.dg/tree-ssa/ssa-fre-85.c b/gcc/testsuite/gcc.dg/tree-ssa/ssa-fre-85.c
new file mode 100644
index 00000000000..c50770caa2f
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/tree-ssa/ssa-fre-85.c
@@ -0,0 +1,14 @@ 
+/* { dg-do compile } */
+/* { dg-options "-O -fstrict-aliasing -fdump-tree-fre1-details" } */
+
+struct X { int i; int j; };
+
+struct X x, y;
+void foo ()
+{
+  x.i = 1;
+  y = x;
+  y.i = 1; // redundant
+}
+
+/* { dg-final { scan-tree-dump "Deleted redundant store y.i" "fre1" } } */
diff --git a/gcc/tree-ssa-sccvn.c b/gcc/tree-ssa-sccvn.c
index 0b8ee586139..ef272047bb3 100644
--- a/gcc/tree-ssa-sccvn.c
+++ b/gcc/tree-ssa-sccvn.c
@@ -3205,6 +3205,8 @@  vn_reference_lookup (tree op, tree vuse, vn_lookup_kind kind,
       return NULL_TREE;
     }
 
+  if (last_vuse_ptr)
+    *last_vuse_ptr = vr1.vuse;
   return vn_reference_lookup_1 (&vr1, vnresult);
 }
 
@@ -4543,6 +4545,23 @@  visit_reference_op_load (tree lhs, tree op, gimple *stmt)
 }
 
 
+/* Returns whether the store to LHS affects dependence analysis compared
+   to the next preceeding stmt doing so at VUSE.  */
+
+static bool
+store_affects_tbaa_memory_state (tree lhs, tree vuse)
+{
+  gassign *def = dyn_cast <gassign *> (SSA_NAME_DEF_STMT (vuse));
+  if (!def)
+    return true;
+  alias_set_type lhs_set = get_alias_set (lhs);
+  alias_set_type def_set = get_alias_set (gimple_assign_lhs (def));
+  if (lhs_set != def_set
+      && !alias_set_subset_of (lhs_set, def_set))
+    return true;
+  return false;
+}
+
 /* Visit a store to a reference operator LHS, part of STMT, value number it,
    and return true if the value number of the LHS has changed as a result.  */
 
@@ -4575,7 +4594,8 @@  visit_reference_op_store (tree lhs, tree op, gimple *stmt)
      Otherwise, the vdefs for the store are used when inserting into
      the table, since the store generates a new memory state.  */
 
-  vn_reference_lookup (lhs, vuse, VN_NOWALK, &vnresult, false);
+  tree last_vuse = NULL_TREE;
+  vn_reference_lookup (lhs, vuse, VN_NOWALK, &vnresult, false, &last_vuse);
   if (vnresult
       && vnresult->result)
     {
@@ -4587,9 +4607,7 @@  visit_reference_op_store (tree lhs, tree op, gimple *stmt)
 	{
 	  /* If the TBAA state isn't compatible for downstream reads
 	     we cannot value-number the VDEFs the same.  */
-	  alias_set_type set = get_alias_set (lhs);
-	  if (vnresult->set != set
-	      && ! alias_set_subset_of (set, vnresult->set))
+	  if (store_affects_tbaa_memory_state (lhs, last_vuse))
 	    resultsame = false;
 	}
     }
@@ -5644,9 +5662,11 @@  eliminate_dom_walker::eliminate_stmt (basic_block b, gimple_stmt_iterator *gsi)
 	    lookup_lhs = NULL_TREE;
 	}
       tree val = NULL_TREE;
+      tree last_vuse = NULL_TREE;
       if (lookup_lhs)
-	val = vn_reference_lookup (lookup_lhs, gimple_vuse (stmt), VN_WALK,
-				   &vnresult, false);
+	val = vn_reference_lookup (lookup_lhs, gimple_vuse (stmt),
+				   VN_WALKREWRITE, &vnresult, false,
+				   &last_vuse);
       if (TREE_CODE (rhs) == SSA_NAME)
 	rhs = VN_INFO (rhs)->valnum;
       if (val
@@ -5660,13 +5680,12 @@  eliminate_dom_walker::eliminate_stmt (basic_block b, gimple_stmt_iterator *gsi)
 		  && TREE_CODE (val) == VIEW_CONVERT_EXPR
 		  && TREE_OPERAND (val, 0) == rhs)))
 	{
-	  /* We can only remove the later store if the former aliases
-	     at least all accesses the later one does or if the store
+	  /* We now proved the memory contents before and after the store
+	     are the same.  We can only remove the later store if the former
+	     aliases at least all accesses the later one does or if the store
 	     was to readonly memory storing the same value.  */
-	  alias_set_type set = get_alias_set (lhs);
 	  if (! vnresult
-	      || vnresult->set == set
-	      || alias_set_subset_of (set, vnresult->set))
+	      || !store_affects_tbaa_memory_state (lhs, last_vuse))
 	    {
 	      if (dump_file && (dump_flags & TDF_DETAILS))
 		{