Re: [PATCH] Fix DEBUG stmt handling in the vectorizer (PR debug/42604)

[Alexandre Oliva <aoliva at redhat dot com>]

This patch, on top of yours, also fixes PR debug/42395.

On Jan  4, 2010, Jakub Jelinek <jakub@redhat.com> wrote:

> As loop closed ssa form apparently allows debug stmts outside of the loop
> using SSA vars defined in the loop, vectorizer can't assume if
> !flow_bb_inside_loop_p that the stmt is a PHI node.  Other places in the
> vectorizer already handle the debug stmts, but these two don't.  If the
> second one doesn't reset the debug stmt, we get a checking ICE afterwards.

Arguably, the fact that we allow debug stmts in non-loop-closed form
could be regarded as a bug.  If we didn't introduce a PHI node at the
loop exit, the variable is dead at the end of the loop, and so dropping
the debug stmt shouldn't be a problem.  However, if the value happens to
still be available somewhere, it might be desirable to keep use it.

So, instead of just dropping such values, I've made some effort to
preserve them.

In the case at hand, the non-loop-closed debug use came about because
the PHI node _78, introduced along with the original g$1 in 078t.sra,
was determined to be dead in 098.dceloop1, because all its uses were in
also-dead PHI nodes.  Its single argument, g$1_50, was thus propagated
to the debug use right after it.  So far so good.

Before we got to that point, 093t.pre had introduced other
non-loop-closed uses of g$1_50 after the _78 PHI node and the debug use.
096t.loopinit introduced the single-argument _128 PHI node right next to
_78, and replaced the uses of _50 with _128 to make them loop-closed.

By the end of 109t.ifcvt, the configuration is still the same: _128 maps
to g$1_50 right before the debug use of g$1_50, and the PRE-introduced
non-debug uses of _128 remain in other blocks.

I decided that, since vect was smart enough to update the use of g$1_50
in the definition of _128, it might as well search for other debug uses
of _50 dominated y _128 and apply the same transformation.  And then, if
other uses of _50 remained that were not dominated by _128, we'd drop
them on the floor.

It turned out that this last bit, the looking for debug uses dominated
by the loop exit block, was enough to fix PR42395 too.  It might even
obviate the need for the debug stmt resetting that you introduced in
your patch, but I wasn't bold enough to remove it altogether.  Instead,
I moved it a bit up, to the point in which we collect the PHI nodes.  I
added an assertion check that we actually got PHI nodes, but only after
testing for debug stmts and resetting them.

I'm bootstrapping this now on x86_64-linux-gnu and ia64-linux-gnu, with
-O2 and with -O3.  Ok to install if it regstraps?

There's a trivial fix for vec.h âhiddenâ in the patch, that might as
well go in regardless of the rest of the patch.

for  gcc/ChangeLog
from  Alexandre Oliva  <aoliva@redhat.com>

	PR debug/42604
	PR debug/42395
	* tree-vect-loop-manip.c (adjust_info): New type.
	(adjust_vec): New pointer to vector.
	(adjust_debug_stmts_now, adjust_vec_debug_stmts): New.
	(adjust_debug_stmts, adjust_phi_and_debug_stmts): New.
	(slpeel_update_phis_for_duplicate_loop): Use them.
	(slpeel_update_phi_nodes_for_guard1): Likewise.
	(slpeel_update_phi_nodes_for_guard2): Likewise.
	(slpeel_tree_peel_loop_to_edge): Likewise.
	(vect_update_ivs_after_vectorizer): Likewise.
	* vec.h (VEC_OP (T,stack,alloc1)): Drop excess paren.
	* tree-vect-loop.c (vect_finalize_reduction): Detect debug
	stmts earlier.  Check that anything else is a PHI node.

for  gcc/testsuite/ChangeLog
from  Alexandre Oliva  <aoliva@redhat.com>

	PR debug/42604
	PR debug/42395
	* gcc.dg/vect/pr42395.c: New.

Index: gcc/tree-vect-loop-manip.c
===================================================================
--- gcc/tree-vect-loop-manip.c.orig	2010-01-06 09:34:37.000000000 -0200
+++ gcc/tree-vect-loop-manip.c	2010-01-06 13:25:05.000000000 -0200
@@ -113,6 +113,126 @@ rename_variables_in_loop (struct loop *l
   free (bbs);
 }
 
+typedef struct
+{
+  tree from, to;
+  basic_block bb;
+} adjust_info;
+
+DEF_VEC_O(adjust_info);
+DEF_VEC_ALLOC_O_STACK(adjust_info);
+#define VEC_adjust_info_stack_alloc(alloc) VEC_stack_alloc (adjust_info, alloc)
+
+/* A stack of values to be adjusted in debug stmts.  */
+static VEC(adjust_info, stack) *adjust_vec;
+
+/* Adjust any debug stmts that referenced AI->from values to use the
+   loop-closed AI->to, if the references are dominated by AI->bb and
+   not by the definition of AI->from.  */
+
+static void
+adjust_debug_stmts_now (adjust_info *ai)
+{
+  basic_block bbphi = ai->bb;
+  tree orig_def = ai->from;
+  tree new_def = ai->to;
+  imm_use_iterator imm_iter;
+  gimple stmt;
+  basic_block bbdef = gimple_bb (SSA_NAME_DEF_STMT (orig_def));
+
+  gcc_assert (dom_info_available_p (CDI_DOMINATORS));
+
+  /* Adjust any debug stmts that held onto non-loop-closed
+     references.  */
+  FOR_EACH_IMM_USE_STMT (stmt, imm_iter, orig_def)
+    {
+      use_operand_p use_p;
+      basic_block bbuse;
+
+      if (!is_gimple_debug (stmt))
+	continue;
+
+      gcc_assert (gimple_debug_bind_p (stmt));
+
+      bbuse = gimple_bb (stmt);
+
+      if ((bbuse == bbphi
+	   || dominated_by_p (CDI_DOMINATORS, bbuse, bbphi))
+	  && !(bbuse == bbdef
+	       || dominated_by_p (CDI_DOMINATORS, bbuse, bbdef)))
+	{
+	  if (new_def)
+	    FOR_EACH_IMM_USE_ON_STMT (use_p, imm_iter)
+	      SET_USE (use_p, new_def);
+	  else
+	    {
+	      gimple_debug_bind_reset_value (stmt);
+	      update_stmt (stmt);
+	    }
+	}
+    }
+}
+
+/* Adjust debug stmts as scheduled before.  */
+
+static void
+adjust_vec_debug_stmts (void)
+{
+  if (!MAY_HAVE_DEBUG_STMTS)
+    return;
+
+  gcc_assert (adjust_vec);
+
+  while (!VEC_empty (adjust_info, adjust_vec))
+    {
+      adjust_debug_stmts_now (VEC_last (adjust_info, adjust_vec));
+      VEC_pop (adjust_info, adjust_vec);
+    }
+
+  VEC_free (adjust_info, stack, adjust_vec);
+}
+
+/* Adjust any debug stmts that referenced FROM values to use the
+   loop-closed TO, if the references are dominated by BB and not by
+   the definition of FROM.  If adjust_vec is non-NULL, adjustments
+   will be postponed until adjust_vec_debug_stmts is called.  */
+
+static void
+adjust_debug_stmts (tree from, tree to, basic_block bb)
+{
+  adjust_info ai;
+
+  if (MAY_HAVE_DEBUG_STMTS && TREE_CODE (from) == SSA_NAME
+      && SSA_NAME_VAR (from) != gimple_vop (cfun))
+    {
+      ai.from = from;
+      ai.to = to;
+      ai.bb = bb;
+
+      if (adjust_vec)
+	VEC_safe_push (adjust_info, stack, adjust_vec, &ai);
+      else
+	adjust_debug_stmts_now (&ai);
+    }
+}
+
+/* Change E's phi arg in UPDATE_PHI to NEW_DEF, and record information
+   to adjust any debug stmts that referenced the old phi arg,
+   presumably non-loop-closed references left over from other
+   transformations.  */
+
+static void
+adjust_phi_and_debug_stmts (gimple update_phi, edge e, tree new_def)
+{
+  tree orig_def = PHI_ARG_DEF_FROM_EDGE (update_phi, e);
+
+  SET_PHI_ARG_DEF (update_phi, e->dest_idx, new_def);
+
+  if (MAY_HAVE_DEBUG_STMTS)
+    adjust_debug_stmts (orig_def, PHI_RESULT (update_phi),
+			gimple_bb (update_phi));
+}
+
 
 /* Update the PHI nodes of NEW_LOOP.
 
@@ -195,13 +315,15 @@ slpeel_update_phis_for_duplicate_loop (s
       /* An ordinary ssa name defined in the loop.  */
       add_phi_arg (phi_new, new_ssa_name, loop_latch_edge (new_loop), locus);
 
+      /* Drop any debug references outside the loop, if they would
+	 become ill-formed SSA.  */
+      adjust_debug_stmts (def, NULL, single_exit (orig_loop)->dest);
+
       /* step 3 (case 1).  */
       if (!after)
         {
           gcc_assert (new_loop_exit_e == orig_entry_e);
-          SET_PHI_ARG_DEF (phi_orig,
-                           new_loop_exit_e->dest_idx,
-                           new_ssa_name);
+	  adjust_phi_and_debug_stmts (phi_orig, new_loop_exit_e, new_ssa_name);
         }
     }
 }
@@ -413,7 +535,7 @@ slpeel_update_phi_nodes_for_guard1 (edge
       /* 1.3. Update phi in successor block.  */
       gcc_assert (PHI_ARG_DEF_FROM_EDGE (update_phi, e) == loop_arg
                   || PHI_ARG_DEF_FROM_EDGE (update_phi, e) == guard_arg);
-      SET_PHI_ARG_DEF (update_phi, e->dest_idx, PHI_RESULT (new_phi));
+      adjust_phi_and_debug_stmts (update_phi, e, PHI_RESULT (new_phi));
       update_phi2 = new_phi;
 
 
@@ -431,7 +553,8 @@ slpeel_update_phi_nodes_for_guard1 (edge
 
       /* 2.3. Update phi in successor of NEW_EXIT_BB:  */
       gcc_assert (PHI_ARG_DEF_FROM_EDGE (update_phi2, new_exit_e) == loop_arg);
-      SET_PHI_ARG_DEF (update_phi2, new_exit_e->dest_idx, PHI_RESULT (new_phi));
+      adjust_phi_and_debug_stmts (update_phi2, new_exit_e,
+				  PHI_RESULT (new_phi));
 
       /* 2.4. Record the newly created name with set_current_def.
          We want to find a name such that
@@ -560,7 +683,7 @@ slpeel_update_phi_nodes_for_guard2 (edge
 
       /* 1.3. Update phi in successor block.  */
       gcc_assert (PHI_ARG_DEF_FROM_EDGE (update_phi, e) == orig_def);
-      SET_PHI_ARG_DEF (update_phi, e->dest_idx, PHI_RESULT (new_phi));
+      adjust_phi_and_debug_stmts (update_phi, e, PHI_RESULT (new_phi));
       update_phi2 = new_phi;
 
 
@@ -575,7 +698,8 @@ slpeel_update_phi_nodes_for_guard2 (edge
 
       /* 2.3. Update phi in successor of NEW_EXIT_BB:  */
       gcc_assert (PHI_ARG_DEF_FROM_EDGE (update_phi2, new_exit_e) == loop_arg);
-      SET_PHI_ARG_DEF (update_phi2, new_exit_e->dest_idx, PHI_RESULT (new_phi));
+      adjust_phi_and_debug_stmts (update_phi2, new_exit_e,
+				  PHI_RESULT (new_phi));
 
 
       /** 3. Handle loop-closed-ssa-form phis for first loop  **/
@@ -612,7 +736,8 @@ slpeel_update_phi_nodes_for_guard2 (edge
       /* 3.4. Update phi in successor of GUARD_BB:  */
       gcc_assert (PHI_ARG_DEF_FROM_EDGE (update_phi2, guard_edge)
                                                                 == guard_arg);
-      SET_PHI_ARG_DEF (update_phi2, guard_edge->dest_idx, PHI_RESULT (new_phi));
+      adjust_phi_and_debug_stmts (update_phi2, guard_edge,
+				  PHI_RESULT (new_phi));
     }
 }
 
@@ -1083,6 +1208,12 @@ slpeel_tree_peel_loop_to_edge (struct lo
       return NULL;
     }
 
+  if (MAY_HAVE_DEBUG_STMTS)
+    {
+      gcc_assert (!adjust_vec);
+      adjust_vec = VEC_alloc (adjust_info, stack, 32);
+    }
+
   if (e == exit_e)
     {
       /* NEW_LOOP was placed after LOOP.  */
@@ -1278,6 +1409,8 @@ slpeel_tree_peel_loop_to_edge (struct lo
   if (update_first_loop_count)
     slpeel_make_loop_iterate_ntimes (first_loop, first_niters);
 
+  adjust_vec_debug_stmts ();
+
   BITMAP_FREE (definitions);
   delete_update_ssa ();
 
@@ -1668,7 +1801,7 @@ vect_update_ivs_after_vectorizer (loop_v
 					  true, GSI_SAME_STMT);
 
       /* Fix phi expressions in the successor bb.  */
-      SET_PHI_ARG_DEF (phi1, update_e->dest_idx, ni_name);
+      adjust_phi_and_debug_stmts (phi1, update_e, ni_name);
     }
 }
 
@@ -2399,7 +2532,7 @@ vect_loop_versioning (loop_vec_info loop
       arg = PHI_ARG_DEF_FROM_EDGE (orig_phi, e);
       add_phi_arg (new_phi, arg, new_exit_e,
 		   gimple_phi_arg_location_from_edge (orig_phi, e));
-      SET_PHI_ARG_DEF (orig_phi, e->dest_idx, PHI_RESULT (new_phi));
+      adjust_phi_and_debug_stmts (orig_phi, e, PHI_RESULT (new_phi));
     }
 
   /* End loop-exit-fixes after versioning.  */
Index: gcc/vec.h
===================================================================
--- gcc/vec.h.orig	2010-01-06 11:11:37.000000000 -0200
+++ gcc/vec.h	2010-01-06 11:12:00.000000000 -0200
@@ -1262,7 +1262,7 @@ struct vec_swallow_trailing_semi
 static inline VEC(T,stack) *VEC_OP (T,stack,alloc1)			  \
      (int alloc_, VEC(T,stack)* space)					  \
 {									  \
-  return ((VEC(T,stack) *) vec_stack_p_reserve_exact_1 (alloc_, space);	  \
+  return (VEC(T,stack) *) vec_stack_p_reserve_exact_1 (alloc_, space);	  \
 }
 
 #define DEF_VEC_ALLOC_I_STACK(T)					  \
Index: gcc/tree-vect-loop.c
===================================================================
--- gcc/tree-vect-loop.c.orig	2010-01-06 11:34:49.000000000 -0200
+++ gcc/tree-vect-loop.c	2010-01-06 11:38:11.000000000 -0200
@@ -3274,6 +3274,22 @@ vect_finalize_reduction:
       if (!flow_bb_inside_loop_p (loop, gimple_bb (USE_STMT (use_p))))
 	{
 	  exit_phi = USE_STMT (use_p);
+	  /* ??? Does this ever hit?  */
+	  if (is_gimple_debug (exit_phi))
+	    {
+	      if (gimple_debug_bind_p (exit_phi))
+		{
+		  if (vect_print_dump_info (REPORT_DETAILS))
+		    fprintf (vect_dump, "killing debug use");
+
+		  gimple_debug_bind_reset_value (exit_phi);
+		  update_stmt (exit_phi);
+		}
+	      else
+		gcc_unreachable ();
+	      continue;
+	    }
+	  gcc_assert (gimple_code (exit_phi) == GIMPLE_PHI);
 	  VEC_quick_push (gimple, phis, exit_phi);
 	}
     }
@@ -3283,20 +3299,6 @@ vect_finalize_reduction:
 
   for (i = 0; VEC_iterate (gimple, phis, i, exit_phi); i++)
     {
-      if (is_gimple_debug (exit_phi))
-	{
-	  if (gimple_debug_bind_p (exit_phi))
-	    {
-	      if (vect_print_dump_info (REPORT_DETAILS))
-		fprintf (vect_dump, "killing debug use");
-
-	      gimple_debug_bind_reset_value (exit_phi);
-	      update_stmt (exit_phi);
-	    }
-	  else
-	    gcc_unreachable ();
-	  continue;
-	}
       if (nested_in_vect_loop)
 	{
 	  stmt_vec_info stmt_vinfo = vinfo_for_stmt (exit_phi);
Index: gcc/testsuite/gcc.dg/vect/pr42395.c
===================================================================
--- /dev/null	1970-01-01 00:00:00.000000000 +0000
+++ gcc/testsuite/gcc.dg/vect/pr42395.c	2010-01-06 13:16:15.000000000 -0200
@@ -0,0 +1,10 @@
+/* PR debug/42395 */
+/* { dg-do compile } */
+/* { dg-options "-O3 -ftree-vectorize -g" } */
+
+void foo(int j, int *A)
+{
+  int i;
+  for (i = 0; i < j; i ++) A[i] = i;
+  for (; i < 4096; i ++) A[i] = 0;
+}

-- 
Alexandre Oliva, freedom fighter    http://FSFLA.org/~lxoliva/
You must be the change you wish to see in the world. -- Gandhi
Be Free! -- http://FSFLA.org/   FSF Latin America board member
Free Software Evangelist      Red Hat Brazil Compiler Engineer