phiprop: skip over clobbers [PR116823]

Like the already approved patch at
https://inbox.sourceware.org/gcc-patches/20240924161312.1556293-2-quic_apinski@quicinc.com/
but reworked to fit into the new simplified code and
also fixed a bug noticed for aggregates.

Aggregates include a store when doing phiprop so we need to
check if there are also loads between the original store/load
and the clobber we are skipping. Like the skipping of the
store case, I didn't see this happening enough to add the
extra checks. I did add a testcase (phiprop-5.C) which checks
this.

changes since v2:
* v3: treat aggregates special earlier and don't duplicate code.
* v2: adapt to can_handle_load instead of inline.

	PR tree-optimization/116823

gcc/ChangeLog:

	* tree-ssa-phiprop.cc (can_handle_load): Skip past
	clobbers for !aggregate.

gcc/testsuite/ChangeLog:

	* g++.dg/tree-ssa/phiprop-2.C: New test.
	* g++.dg/tree-ssa/phiprop-4.C: New test.
	* g++.dg/tree-ssa/phiprop-5.C: New test.

Signed-off-by: Andrew Pinski <andrew.pinski@oss.qualcomm.com>

diff --git a/gcc/testsuite/g++.dg/tree-ssa/phiprop-2.C b/gcc/testsuite/g++.dg/tree-ssa/phiprop-2.C
new file mode 100644 (file)
index 0000000..e3388d1
--- /dev/null
+++ b/gcc/testsuite/g++.dg/tree-ssa/phiprop-2.C
@@ -0,0 +1,23 @@
+/* { dg-do compile } */
+/* { dg-options "-O1 -fdump-tree-phiprop1-details -fdump-tree-release_ssa" } */
+
+/* PR tree-optimization/116823 */
+/* The clobber on a should not get in the way of phiprop here even if
+   this is undefined code. */
+/* We should have MIN_EXPR early on then too. */
+
+static inline
+const int &c(const int &d, const int &e) {
+  if (d < e)
+    return d;
+  return e;
+}
+
+int g(int i, struct f *ff)
+{
+  const int &a = c(i, 10);
+  return a;
+}
+/* { dg-final { scan-tree-dump-times "Inserting PHI for result of load" 1 "phiprop1"} } */
+/* { dg-final { scan-tree-dump-times "MIN_EXPR" 1 "release_ssa"} } */
+

diff --git a/gcc/testsuite/g++.dg/tree-ssa/phiprop-4.C b/gcc/testsuite/g++.dg/tree-ssa/phiprop-4.C
new file mode 100644 (file)
index 0000000..b630148
--- /dev/null
+++ b/gcc/testsuite/g++.dg/tree-ssa/phiprop-4.C
@@ -0,0 +1,30 @@
+/* { dg-do compile } */
+/* { dg-options "-O2 -fno-tree-sra -fdump-tree-phiprop-details" } */
+/* PR tree-optimization/123120 */
+/* Make sure the store to *e conflicts with the store to *d */
+
+
+struct s1
+{
+  int a;
+};
+
+void f(int *);
+
+void g(struct s1 i, struct s1 *d, struct s1 *e)
+{
+  const struct s1 t = {10};
+  const struct s1 *a;
+  struct s1 t1 = {2};
+  if (t.a < i.a)
+    a = &t;
+  else
+    a = &i;
+  *e = t1;
+  t1 = *a;
+  *d = t1;
+}
+
+/* { dg-final { scan-tree-dump-not "Inserting PHI for result of load" "phiprop1"} } */
+/* { dg-final { scan-tree-dump-not "Inserting PHI for result of load" "phiprop2"} } */
+

diff --git a/gcc/testsuite/g++.dg/tree-ssa/phiprop-5.C b/gcc/testsuite/g++.dg/tree-ssa/phiprop-5.C
new file mode 100644 (file)
index 0000000..5a3a04d
--- /dev/null
+++ b/gcc/testsuite/g++.dg/tree-ssa/phiprop-5.C
@@ -0,0 +1,36 @@
+/* { dg-do compile } */
+/* { dg-options "-O2 -fdump-tree-phiprop-details" } */
+/* PR tree-optimization/116823 */
+/* Make sure the store to *d conflicts with the load from d->a; */
+
+
+struct s1
+{
+  int a;
+};
+
+void f(int *);
+
+int g(struct s1 i, struct s1 *d, struct s1 *e)
+{
+  int t3;
+  struct s1 t1 = {2};
+  const struct s1 t = {10};
+  const struct s1 *a;
+  {
+    int t67;
+    f(&t67);
+    if (t.a < i.a)
+      a = &t;
+    else
+      a = &i;
+  }
+  t3 = d->a;
+  *d = *a;
+  return t3;
+}
+
+
+/* { dg-final { scan-tree-dump-not "Inserting PHI for result of load" "phiprop1"} } */
+/* { dg-final { scan-tree-dump-not "Inserting PHI for result of load" "phiprop2"} } */
+

diff --git a/gcc/tree-ssa-phiprop.cc b/gcc/tree-ssa-phiprop.cc
index 5985beae26a5189954e5e909c7776b7c7b990ae9..54f3ad8d9f8411af547455783fc43f547990f909 100644 (file)
--- a/gcc/tree-ssa-phiprop.cc
+++ b/gcc/tree-ssa-phiprop.cc
@@ -274,22 +274,57 @@ can_handle_load (gimple *load_stmt,
     expected_vuse = gimple_phi_result (vphi);
   else if (up_vuse)
     expected_vuse = up_vuse;
+  /* Try to see if the store does not effect the load.  */
+  gimple *other_store = SSA_NAME_DEF_STMT (vuse);
+  /* For aggregates, skipping the store is too
+     hard to handle as you need to check for loads
+     and it is not worth the extra checks so just handle expected vuse
+     and the dominated by case.   */
+  if (aggregate)
+    {
+      /* If the vuse on the load is the same as the expected vuse,
+        there are no stores inbetween.  */
+      if (vuse == expected_vuse)
+       return vuse;
+      if (expected_vuse)
+       return NULL_TREE;
+      if (gimple_bb (other_store) != bb
+         && dominated_by_p (CDI_DOMINATORS,
+                            bb, gimple_bb (other_store)))
+       return vuse;
+      return NULL_TREE;
+    }
+
+  /* Skip over clobbers in the same bb as the use
+     as they don't interfere with loads.  */
+  while (!SSA_NAME_IS_DEFAULT_DEF (vuse)
+        && gimple_clobber_p (other_store)
+        && gimple_bb (other_store) == bb)
+    {
+      vuse = gimple_vuse (other_store);
+      other_store = SSA_NAME_DEF_STMT (vuse);
+    }
+  /* If the load does not have a store beforehand,
+     then we can do the load in conditional. */
+  if (SSA_NAME_IS_DEFAULT_DEF (vuse))
+    {
+      /* For loads that have no stores before, there should be no
+        vphi.  */
+      gcc_checking_assert (!vphi);
+      /* The common vuse is the same as the default or there is none. */
+      gcc_checking_assert (!up_vuse || up_vuse == vuse);
+      return vuse;
+    }
 
   /* If the vuse on the load is the same as the expected vuse,
      there are no stores inbetween.  */
   if (vuse == expected_vuse)
     return vuse;
-  /* Try to see if the store does not effect the load.  */
-  gimple *other_store = SSA_NAME_DEF_STMT (vuse);
+
   /* Only handling the case where the store is in the same
      bb as the phi.  */
   if (gimple_bb (other_store) == bb)
     {
-      /* For aggregates, skipping the store is too
-        hard to handle as you need to check for loads
-        and it is not worth the extra checks. */
-      if (aggregate)
-       return NULL_TREE;
       tree src = gimple_assign_rhs1 (load_stmt);
       ao_ref read;
       ao_ref_init (&read, src);