[PATCH] Fix PR89463 (and dups?), wrong-debug with loop removal

[Richard Biener <rguenther at suse dot de>]

This PR and possibly quite some dups that have been accumulating lately
run into an artifact of DCEs edge removal code when generating debug
stmts.  Here DCE removes edges from the CFG at the same time it removes
dead controlling stmts but before PHI nodes are removed.  This causes
us to remove PHI arguments associated with removed edges and make
those PHI appearing as degenerate, causing wrong debug stmts to be
generated.  Consider for example i_1 = PHI <i_2, 0> where the edge
leading to i_2 is removed - at the time we remove the PHI it looks
like i_1 = PHI <0> and a i = 0 debug-stmt is generated.

The fix is to delay edge removal and move PHI removal back to the
place it had originally been, doing that in reverse dominator order
as intended for debug stmt creation (it jumped from where it'll be
after the patch to all-before to all-after).

The testcase shows up as UNRESOLVED where it formerly was
wrong-debug because we change from i = 0 to i = NULL.  I've seen
no way to mark "optimized-out" as expected and at -O1 we
retain the loop and the correct value of i.

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

OK (well, I think it is, just double-checking).

Thanks,
Richard.

2019-03-25  Richard Biener  <rguenther@suse.de>

	PR tree-optimization/89463
	* tree-ssa-dce.c (remove_dead_stmt): Take output vector to
	queue edges to remove.
	(eliminate_unnecessary_stmts): Remove dead PHIs alongside
	dead stmts.  Delay edge removal until PHIs are removed to
	make debug-stmt creation not confused by seemingly degenerate
	PHIs.

	* gcc.dg/guality/pr89463.c: New testcase.

Index: gcc/tree-ssa-dce.c
===================================================================
--- gcc/tree-ssa-dce.c	(revision 269908)
+++ gcc/tree-ssa-dce.c	(working copy)
@@ -985,7 +985,8 @@ remove_dead_phis (basic_block bb)
    containing I so that we don't have to look it up.  */
 
 static void
-remove_dead_stmt (gimple_stmt_iterator *i, basic_block bb)
+remove_dead_stmt (gimple_stmt_iterator *i, basic_block bb,
+		  vec<edge> &to_remove_edges)
 {
   gimple *stmt = gsi_stmt (*i);
 
@@ -1045,20 +1046,17 @@ remove_dead_stmt (gimple_stmt_iterator *
       e->flags |= EDGE_FALLTHRU;
 
       /* Remove the remaining outgoing edges.  */
-      for (ei = ei_start (bb->succs); (e2 = ei_safe_edge (ei)); )
+      FOR_EACH_EDGE (e2, ei, bb->succs)
 	if (e != e2)
 	  {
-	    cfg_altered = true;
 	    /* If we made a BB unconditionally exit a loop or removed
 	       an entry into an irreducible region, then this transform
 	       alters the set of BBs in the loop.  Schedule a fixup.  */
 	    if (loop_exit_edge_p (bb->loop_father, e)
 		|| (e2->dest->flags & BB_IRREDUCIBLE_LOOP))
 	      loops_state_set (LOOPS_NEED_FIXUP);
-	    remove_edge (e2);
+	    to_remove_edges.safe_push (e2);
 	  }
-	else
-	  ei_next (&ei);
     }
 
   /* If this is a store into a variable that is being optimized away,
@@ -1201,6 +1199,7 @@ eliminate_unnecessary_stmts (void)
   gimple *stmt;
   tree call;
   vec<basic_block> h;
+  auto_vec<edge> to_remove_edges;
 
   if (dump_file && (dump_flags & TDF_DETAILS))
     fprintf (dump_file, "\nEliminating unnecessary statements:\n");
@@ -1287,7 +1286,7 @@ eliminate_unnecessary_stmts (void)
 		}
 	      if (!is_gimple_debug (stmt))
 		something_changed = true;
-	      remove_dead_stmt (&gsi, bb);
+	      remove_dead_stmt (&gsi, bb, to_remove_edges);
 	    }
 	  else if (is_gimple_call (stmt))
 	    {
@@ -1331,7 +1330,7 @@ eliminate_unnecessary_stmts (void)
 		      {
 		      case IFN_GOMP_SIMD_LANE:
 		      case IFN_ASAN_POISON:
-			remove_dead_stmt (&gsi, bb);
+			remove_dead_stmt (&gsi, bb, to_remove_edges);
 			break;
 		      default:
 			break;
@@ -1354,6 +1353,9 @@ eliminate_unnecessary_stmts (void)
 		  }
 	    }
 	}
+
+      /* Remove dead PHI nodes.  */
+      something_changed |= remove_dead_phis (bb);
     }
 
   h.release ();
@@ -1361,10 +1363,16 @@ eliminate_unnecessary_stmts (void)
   /* Since we don't track liveness of virtual PHI nodes, it is possible that we
      rendered some PHI nodes unreachable while they are still in use.
      Mark them for renaming.  */
-  if (cfg_altered)
+  if (!to_remove_edges.is_empty ())
     {
       basic_block prev_bb;
 
+      /* Remove edges.  We've delayed this to not get bogus debug stmts
+         during PHI node removal.  */
+      for (unsigned i = 0; i < to_remove_edges.length (); ++i)
+	remove_edge (to_remove_edges[i]);
+      cfg_altered = true;
+
       find_unreachable_blocks ();
 
       /* Delete all unreachable basic blocks in reverse dominator order.  */
@@ -1430,11 +1438,6 @@ eliminate_unnecessary_stmts (void)
 	    }
 	}
     }
-  FOR_EACH_BB_FN (bb, cfun)
-    {
-      /* Remove dead PHI nodes.  */
-      something_changed |= remove_dead_phis (bb);
-    }
 
   if (bb_postorder)
     free (bb_postorder);
Index: gcc/testsuite/gcc.dg/guality/pr89463.c
===================================================================
--- gcc/testsuite/gcc.dg/guality/pr89463.c	(nonexistent)
+++ gcc/testsuite/gcc.dg/guality/pr89463.c	(working copy)
@@ -0,0 +1,25 @@
+/* { dg-do run } */
+/* { dg-options "-g" } */
+
+void __attribute__((noinline))
+optimize_me_not ()
+{
+  __asm__ volatile ("" : : : "memory");
+}
+int a;
+int main()
+{
+  int i;
+  for (; a < 10; a++)
+    i = 0;
+  for (; i < 6; i++)
+    ;
+  /* i may very well be optimized out, so we cannot test for i == 6.
+     Instead test i + 1 which will make the test UNSUPPORTED if i
+     is optimized out.  Since the test previously had wrong debug
+     with i == 0 this is acceptable.  Optimally we'd produce a
+     debug stmt for the final value of the loop which would fix
+     the UNSUPPORTED cases.  */
+  optimize_me_not(); /* { dg-final { gdb-test . "i + 1" "7" } } */
+  return 0;
+}