cselim: Don't assume it is safe to cstore replace a store to a local variable with...

As the new testcase shows, it is not safe to assume we can optimize
a conditional store into an automatic non-addressable var, we can do it
only if we can prove that the unconditional load or store actually will
not be outside of the boundaries of the variable.
If the offset and size are constant, we can, but this is already all
checked in !tree_could_trap_p, otherwise we really need to check for
a dominating unconditional store, or for the specific case of automatic
non-addressable variables, it is enough if there is a dominating load
(that is what those 4 testcases have).  tree-ssa-phiopt.c has some
infrastructure for this already, see the add_or_mark_expr method etc.,
but right now it handles only MEM_REFs with SSA_NAME first operand
and some integral offset.  So, I think it can be for GCC11 extended
to handle other memory references, possibly up to just doing
get_inner_reference and hasing based on the base, offset expressions
and bit_offset and bit_size, and have also a special case that for
!TREE_ADDRESSABLE automatic variables it could ignore whether something
is a load or store because the local stack should be always writable.
But it feels way too dangerous to do this this late for GCC10, so this
patch just restricts the optimization to the safe case (where lhs doesn't
trap), and on Richi's request also ignores TREE_ADDRESSABLE bit if
flag_store_data_races, because my understanding the reason for
TREE_ADDRESSABLE check is that we want to avoid introducing
store data races (if address of an automatic var escapes, some other thread
could be accessing it concurrently).

2020-04-25  Jakub Jelinek  <jakub@redhat.com>
	    Richard Biener  <rguenther@suse.de>

	PR tree-optimization/94734
	PR tree-optimization/89430
	* tree-ssa-phiopt.c: Include tree-eh.h.
	(cond_store_replacement): Return false if an automatic variable
	access could trap.  If -fstore-data-races, don't return false
	just because an automatic variable is addressable.

	* gcc.dg/tree-ssa/pr89430-1.c: Add xfail.
	* gcc.dg/tree-ssa/pr89430-2.c: Add xfail.
	* gcc.dg/tree-ssa/pr89430-5.c: Add xfail.
	* gcc.dg/tree-ssa/pr89430-6.c: Add xfail.
	* gcc.c-torture/execute/pr94734.c: New test.

diff --git a/gcc/ChangeLog b/gcc/ChangeLog
index 30e9675693fb0e747205e47c63d5b142fe0705f1..3c0d67b206f3d43829a106a3ff0b11d81b02b88e 100644 (file)
--- a/gcc/ChangeLog
+++ b/gcc/ChangeLog
@@ -1,3 +1,13 @@
+2020-04-25  Jakub Jelinek  <jakub@redhat.com>
+           Richard Biener  <rguenther@suse.de>
+
+       PR tree-optimization/94734
+       PR tree-optimization/89430
+       * tree-ssa-phiopt.c: Include tree-eh.h.
+       (cond_store_replacement): Return false if an automatic variable
+       access could trap.  If -fstore-data-races, don't return false
+       just because an automatic variable is addressable.
+
 2020-04-24  Andrew Stubbs  <ams@codesourcery.com>
 
        * config/gcn/gcn-valu.md (add<mode>_zext_dup2_exec): Fix merge

diff --git a/gcc/testsuite/ChangeLog b/gcc/testsuite/ChangeLog
index 099c76aee60aa0749c0f7d689d8ef594eac4577a..fa583670bafc333e323b60766c1ba3dc476296be 100644 (file)
--- a/gcc/testsuite/ChangeLog
+++ b/gcc/testsuite/ChangeLog
@@ -1,3 +1,13 @@
+2020-04-25  Jakub Jelinek  <jakub@redhat.com>
+
+       PR tree-optimization/94734
+       PR tree-optimization/89430
+       * gcc.dg/tree-ssa/pr89430-1.c: Add xfail.
+       * gcc.dg/tree-ssa/pr89430-2.c: Add xfail.
+       * gcc.dg/tree-ssa/pr89430-5.c: Add xfail.
+       * gcc.dg/tree-ssa/pr89430-6.c: Add xfail.
+       * gcc.c-torture/execute/pr94734.c: New test.
+
 2020-04-24  Jakub Jelinek  <jakub@redhat.com>
 
        PR c++/94383

diff --git a/gcc/testsuite/gcc.c-torture/execute/pr94734.c b/gcc/testsuite/gcc.c-torture/execute/pr94734.c
new file mode 100644 (file)
index 0000000..9df0de6
--- /dev/null
+++ b/gcc/testsuite/gcc.c-torture/execute/pr94734.c
@@ -0,0 +1,59 @@
+/* PR tree-optimization/94734 */
+
+__attribute__((noipa)) int
+foo (int n)
+{
+  int arr[16], s = 0;
+  for (int i = 0; i < n; i++)
+    {
+      if (i < 16)
+       arr[i] = i;
+    }
+  for (int i = 0; i < 16; i++)
+    s += arr[i];
+  return s;
+}
+
+__attribute__((noipa)) int
+bar (int n, int x, unsigned long y, unsigned long z)
+{
+  int arr[16], s = 0;
+  arr[4] = 42;
+  for (int i = 0; i < n; i++)
+    {
+      if (x == (i & 0x25))
+       arr[y] = i;
+    }
+  return arr[z];
+}
+
+__attribute__((noipa)) int
+baz (int n, int x, unsigned long z)
+{
+  int arr[16], s = 0;
+  arr[12] = 42;
+  for (int i = 0; i < n; i++)
+    {
+      if (x == (i & 0x25))
+       arr[7] = i;
+    }
+  return arr[z];
+}
+
+int
+main ()
+{
+  if (foo (10374) != 15 * 16 / 2)
+    __builtin_abort ();
+  if (bar (25, 0x25, (unsigned long) 0xdeadbeefbeefdeadULL, 4) != 42)
+    __builtin_abort ();
+  if (bar (25, 4, 15, 15) != 22)
+    __builtin_abort ();
+  if (baz (25, 0x25, 12) != 42)
+    __builtin_abort ();
+  if (baz (25, 4, 7) != 22)
+    __builtin_abort ();
+  if (baz (25, 4, 12) != 42)
+    __builtin_abort ();
+  return 0;
+}

diff --git a/gcc/testsuite/gcc.dg/tree-ssa/pr89430-1.c b/gcc/testsuite/gcc.dg/tree-ssa/pr89430-1.c
index 8ee1850ac6327c32092c32a5680ba961a0818c0d..ce242ba569b184264c7ce532deab5690a299a908 100644 (file)
--- a/gcc/testsuite/gcc.dg/tree-ssa/pr89430-1.c
+++ b/gcc/testsuite/gcc.dg/tree-ssa/pr89430-1.c
@@ -9,4 +9,4 @@ unsigned test(unsigned k, unsigned b) {
         return a[0]+a[1];
 }
 
-/* { dg-final { scan-tree-dump "Conditional store replacement" "cselim" } } */
+/* { dg-final { scan-tree-dump "Conditional store replacement" "cselim" { xfail *-*-* } } } */

diff --git a/gcc/testsuite/gcc.dg/tree-ssa/pr89430-2.c b/gcc/testsuite/gcc.dg/tree-ssa/pr89430-2.c
index 9b96875ac7ab24b8beaeadedfb84eda9a136b16f..90ae36bfce28a83ed0eac627ee28de3caae57224 100644 (file)
--- a/gcc/testsuite/gcc.dg/tree-ssa/pr89430-2.c
+++ b/gcc/testsuite/gcc.dg/tree-ssa/pr89430-2.c
@@ -11,4 +11,4 @@ unsigned test(unsigned k, unsigned b) {
         return a[0]+a[1];
 }
 
-/* { dg-final { scan-tree-dump "Conditional store replacement" "cselim" } } */
+/* { dg-final { scan-tree-dump "Conditional store replacement" "cselim" { xfail *-*-* } } } */

diff --git a/gcc/testsuite/gcc.dg/tree-ssa/pr89430-5.c b/gcc/testsuite/gcc.dg/tree-ssa/pr89430-5.c
index b2d04119381341e6e8c455d5d77c93c2cac9d53a..c633cbe947dc3b87403190b07d42500f133a1965 100644 (file)
--- a/gcc/testsuite/gcc.dg/tree-ssa/pr89430-5.c
+++ b/gcc/testsuite/gcc.dg/tree-ssa/pr89430-5.c
@@ -13,4 +13,4 @@ int test(int b, int k) {
     return a.data[0] + a.data[1];
 }
 
-/* { dg-final { scan-tree-dump "Conditional store replacement" "cselim" } } */
+/* { dg-final { scan-tree-dump "Conditional store replacement" "cselim" { xfail *-*-* } } } */

diff --git a/gcc/testsuite/gcc.dg/tree-ssa/pr89430-6.c b/gcc/testsuite/gcc.dg/tree-ssa/pr89430-6.c
index 8d3c4f7cc6a3b75ba7c69ec5213a67f90ad6327c..7cad563128dc6afe23b6537a8b7d4b4431333596 100644 (file)
--- a/gcc/testsuite/gcc.dg/tree-ssa/pr89430-6.c
+++ b/gcc/testsuite/gcc.dg/tree-ssa/pr89430-6.c
@@ -16,4 +16,4 @@ int test(int b, int k) {
     return a.data[0].x + a.data[1].x;
 }
 
-/* { dg-final { scan-tree-dump "Conditional store replacement" "cselim" } } */
+/* { dg-final { scan-tree-dump "Conditional store replacement" "cselim" { xfail *-*-* } } } */

diff --git a/gcc/tree-ssa-phiopt.c b/gcc/tree-ssa-phiopt.c
index 969311880a93519865c38fc47c2edd16162dad4f..b1e0dce93d88dc57388a5204c20bc2527dc44664 100644 (file)
--- a/gcc/tree-ssa-phiopt.c
+++ b/gcc/tree-ssa-phiopt.c
@@ -45,6 +45,7 @@ along with GCC; see the file COPYING3.  If not see
 #include "tree-scalar-evolution.h"
 #include "tree-inline.h"
 #include "case-cfn-macros.h"
+#include "tree-eh.h"
 
 static unsigned int tree_ssa_phiopt_worker (bool, bool, bool);
 static bool two_value_replacement (basic_block, basic_block, edge, gphi *,
@@ -2237,10 +2238,13 @@ cond_store_replacement (basic_block middle_bb, basic_block join_bb,
      whose value is not available readily, which we want to avoid.  */
   if (!nontrap->contains (lhs))
     {
-      /* If LHS is a local variable without address-taken, we could
+      /* If LHS is an access to a local variable without address-taken
+        (or when we allow data races) and known not to trap, we could
         always safely move down the store.  */
       tree base = get_base_address (lhs);
-      if (!auto_var_p (base) || TREE_ADDRESSABLE (base))
+      if (!auto_var_p (base)
+         || (TREE_ADDRESSABLE (base) && !flag_store_data_races)
+         || tree_could_trap_p (lhs))
        return false;
     }