diff mbox series

Fix PR87176

Message ID alpine.LSU.2.20.1809031634590.16707@zhemvz.fhfr.qr
State New
Headers show
Series Fix PR87176 | expand

Commit Message

Richard Biener Sept. 3, 2018, 2:38 p.m. UTC
The following fixes a wrong-code issue similar to that in PR87132 where
the alias walk reaches into code parts that are only reachable over
backedges.  This time not via PHI processing but by value-numbering
a virtual PHI to its backedge value.  That doesn't play well with the
way we do iteration for the memory state.

This also adds a testcase showing the desirable way of doing virtual
PHI value-numbering (ideally the walking code would honor edge
executability state but that would tie it even more to VN...).

Bootstrap & regtest running on x86_64-unknown-linux-gnu.

2018-09-03  Richard Biener  <rguenther@suse.de>

	PR tree-optimization/87176
	* tree-ssa-sccvn.c (visit_phi): Remove redundant allsame
	variable.  When value-numbering a virtual PHI node make sure
	to not value-number to the backedge value.

	* gcc.dg/torture/pr87176.c: New testcase.
	* gcc.dg/torture/ssa-fre-1.c: Likewise.

Comments

Richard Biener Sept. 4, 2018, 10:54 a.m. UTC | #1
On Mon, 3 Sep 2018, Richard Biener wrote:

> 
> The following fixes a wrong-code issue similar to that in PR87132 where
> the alias walk reaches into code parts that are only reachable over
> backedges.  This time not via PHI processing but by value-numbering
> a virtual PHI to its backedge value.  That doesn't play well with the
> way we do iteration for the memory state.
> 
> This also adds a testcase showing the desirable way of doing virtual
> PHI value-numbering (ideally the walking code would honor edge
> executability state but that would tie it even more to VN...).
> 
> Bootstrap & regtest running on x86_64-unknown-linux-gnu.

This is what I have applied.

Richard.

2018-09-04  Richard Biener  <rguenther@suse.de>

	PR tree-optimization/87176
	* tree-ssa-sccvn.c (visit_phi): Remove redundant allsame
	variable.  When value-numbering a virtual PHI node make sure
	to not value-number to the backedge value.

	* gcc.dg/torture/pr87176.c: New testcase.
	* gcc.dg/torture/ssa-fre-1.c: Likewise.

Index: gcc/tree-ssa-sccvn.c
===================================================================
--- gcc/tree-ssa-sccvn.c	(revision 264069)
+++ gcc/tree-ssa-sccvn.c	(working copy)
@@ -4110,11 +4110,11 @@ static bool
 visit_phi (gimple *phi, bool *inserted, bool backedges_varying_p)
 {
   tree result, sameval = VN_TOP, seen_undef = NULL_TREE;
-  tree backedge_name = NULL_TREE;
+  tree backedge_val = NULL_TREE;
+  bool seen_non_backedge = false;
   tree sameval_base = NULL_TREE;
   poly_int64 soff, doff;
   unsigned n_executable = 0;
-  bool allsame = true;
   edge_iterator ei;
   edge e;
 
@@ -4139,11 +4139,13 @@ visit_phi (gimple *phi, bool *inserted,
 	++n_executable;
 	if (TREE_CODE (def) == SSA_NAME)
 	  {
-	    if (e->flags & EDGE_DFS_BACK)
-	      backedge_name = def;
 	    if (!backedges_varying_p || !(e->flags & EDGE_DFS_BACK))
 	      def = SSA_VAL (def);
+	    if (e->flags & EDGE_DFS_BACK)
+	      backedge_val = def;
 	  }
+	if (!(e->flags & EDGE_DFS_BACK))
+	  seen_non_backedge = true;
 	if (def == VN_TOP)
 	  ;
 	/* Ignore undefined defs for sameval but record one.  */
@@ -4172,16 +4174,25 @@ visit_phi (gimple *phi, bool *inserted,
 			 && known_eq (soff, doff))
 		  continue;
 	      }
-	    allsame = false;
+	    sameval = NULL_TREE;
 	    break;
 	  }
       }
 
   /* If the value we want to use is the backedge and that wasn't visited
-     yet drop to VARYING.  */ 
-  if (backedge_name
-      && sameval == backedge_name
-      && !SSA_VISITED (backedge_name))
+     yet drop to VARYING.  This only happens when not iterating.
+     If we value-number a virtual operand never value-number to the
+     value from the backedge as that confuses the alias-walking code.
+     See gcc.dg/torture/pr87176.c.  If the value is the same on a
+     non-backedge everything is OK though.  */
+  if (backedge_val
+      && !seen_non_backedge
+      && TREE_CODE (backedge_val) == SSA_NAME
+      && sameval == backedge_val
+      && (SSA_NAME_IS_VIRTUAL_OPERAND (backedge_val)
+	  || !SSA_VISITED (backedge_val)))
+    /* Note this just drops to VARYING without inserting the PHI into
+       the hashes.  */
     result = PHI_RESULT (phi);
   /* If none of the edges was executable keep the value-number at VN_TOP,
      if only a single edge is exectuable use its value.  */
@@ -4212,7 +4223,7 @@ visit_phi (gimple *phi, bool *inserted,
   /* If all values are the same use that, unless we've seen undefined
      values as well and the value isn't constant.
      CCP/copyprop have the same restriction to not remove uninit warnings.  */
-  else if (allsame
+  else if (sameval
 	   && (! seen_undef || is_gimple_min_invariant (sameval)))
     result = sameval;
   else
Index: gcc/testsuite/gcc.dg/torture/pr87176.c
===================================================================
--- gcc/testsuite/gcc.dg/torture/pr87176.c	(nonexistent)
+++ gcc/testsuite/gcc.dg/torture/pr87176.c	(working copy)
@@ -0,0 +1,28 @@
+/* { dg-do run } */
+
+int a, b, c;
+
+int main ()
+{
+  int d = a = 0;
+  while (1)
+    {
+      a = a ^ 6;
+      if (!a)
+	break;
+      if (d)
+	goto L;
+      d = a;
+      for (b = 0; b < 2; b++)
+	{
+	  const int *f[3] = { &c };
+	  const int **g[] = { &f[2] };
+	  int h = ~d;
+	  if (d)
+	    L:
+		if (h > 1)
+		  continue;
+	}
+    }
+  return 0;
+}
Index: gcc/testsuite/gcc.dg/torture/ssa-fre-1.c
===================================================================
--- gcc/testsuite/gcc.dg/torture/ssa-fre-1.c	(nonexistent)
+++ gcc/testsuite/gcc.dg/torture/ssa-fre-1.c	(working copy)
@@ -0,0 +1,16 @@
+/* { dg-do compile } */
+/* { dg-skip-if "" { *-*-* } { "-O0" } { "" } } */
+/* { dg-additional-options "-fstrict-aliasing -fdump-tree-fre1" } */
+
+float f;
+int foo(int *p, int *q)
+{
+  *p = 0;
+  if (*p)
+    *q = 1;
+  else
+    f = 8.0f;
+  return *p;
+}
+
+/* { dg-final { scan-tree-dump "return 0;" "fre1" } } */
diff mbox series

Patch

Index: gcc/tree-ssa-sccvn.c
===================================================================
--- gcc/tree-ssa-sccvn.c	(revision 264058)
+++ gcc/tree-ssa-sccvn.c	(working copy)
@@ -4113,11 +4105,10 @@  static bool
 visit_phi (gimple *phi, bool *inserted, bool backedges_varying_p)
 {
   tree result, sameval = VN_TOP, seen_undef = NULL_TREE;
-  tree backedge_name = NULL_TREE;
+  tree backedge_val = NULL_TREE;
   tree sameval_base = NULL_TREE;
   poly_int64 soff, doff;
   unsigned n_executable = 0;
-  bool allsame = true;
   edge_iterator ei;
   edge e;
 
@@ -4142,10 +4133,10 @@  visit_phi (gimple *phi, bool *inserted,
 	++n_executable;
 	if (TREE_CODE (def) == SSA_NAME)
 	  {
-	    if (e->flags & EDGE_DFS_BACK)
-	      backedge_name = def;
 	    if (!backedges_varying_p || !(e->flags & EDGE_DFS_BACK))
 	      def = SSA_VAL (def);
+	    if (e->flags & EDGE_DFS_BACK)
+	      backedge_val = def;
 	  }
 	if (def == VN_TOP)
 	  ;
@@ -4175,16 +4166,23 @@  visit_phi (gimple *phi, bool *inserted,
 			 && known_eq (soff, doff))
 		  continue;
 	      }
-	    allsame = false;
+	    sameval = NULL_TREE;
 	    break;
 	  }
       }
 
   /* If the value we want to use is the backedge and that wasn't visited
-     yet drop to VARYING.  */ 
-  if (backedge_name
-      && sameval == backedge_name
-      && !SSA_VISITED (backedge_name))
+     yet drop to VARYING.  This only happens when not iterating.
+     If we value-number a virtual operand never value-number to the
+     value from the backedge as that confuses the alias-walking code.
+     See gcc.dg/torture/pr87176.c.  */
+  if (backedge_val
+      && TREE_CODE (backedge_val) == SSA_NAME
+      && sameval == backedge_val
+      && (SSA_NAME_IS_VIRTUAL_OPERAND (backedge_val)
+	  || !SSA_VISITED (backedge_val)))
+    /* Note this just drops to VARYING without inserting the PHI into
+       the hashes.  */
     result = PHI_RESULT (phi);
   /* If none of the edges was executable keep the value-number at VN_TOP,
      if only a single edge is exectuable use its value.  */
@@ -4215,7 +4213,7 @@  visit_phi (gimple *phi, bool *inserted,
   /* If all values are the same use that, unless we've seen undefined
      values as well and the value isn't constant.
      CCP/copyprop have the same restriction to not remove uninit warnings.  */
-  else if (allsame
+  else if (sameval
 	   && (! seen_undef || is_gimple_min_invariant (sameval)))
     result = sameval;
   else
Index: gcc/testsuite/gcc.dg/torture/pr87176.c
===================================================================
--- gcc/testsuite/gcc.dg/torture/pr87176.c	(nonexistent)
+++ gcc/testsuite/gcc.dg/torture/pr87176.c	(working copy)
@@ -0,0 +1,28 @@ 
+/* { dg-do run } */
+
+int a, b, c;
+
+int main ()
+{
+  int d = a = 0;
+  while (1)
+    {
+      a = a ^ 6;
+      if (!a)
+	break;
+      if (d)
+	goto L;
+      d = a;
+      for (b = 0; b < 2; b++)
+	{
+	  const int *f[3] = { &c };
+	  const int **g[] = { &f[2] };
+	  int h = ~d;
+	  if (d)
+	    L:
+		if (h > 1)
+		  continue;
+	}
+    }
+  return 0;
+}
Index: gcc/testsuite/gcc.dg/torture/ssa-fre-1.c
===================================================================
--- gcc/testsuite/gcc.dg/torture/ssa-fre-1.c	(nonexistent)
+++ gcc/testsuite/gcc.dg/torture/ssa-fre-1.c	(working copy)
@@ -0,0 +1,16 @@ 
+/* { dg-do compile } */
+/* { dg-skip-if "" { *-*-* } { "-O0" } { "" } } */
+/* { dg-additional-options "-fstrict-aliasing -fdump-tree-fre1" } */
+
+float f;
+int foo(int *p, int *q)
+{
+  *p = 0;
+  if (*p)
+    *q = 1;
+  else
+    f = 8.0f;
+  return *p;
+}
+
+/* { dg-final { scan-tree-dump "return 0;" "fre1" } } */