Patchwork [PR,debug/46931] don't crash propagating removed DEFs into debug stmts

login
register
mail settings
Submitter Alexandre Oliva
Date Dec. 18, 2010, 7:34 a.m.
Message ID <orpqsz1qm3.fsf@livre.localdomain>
Download mbox | patch
Permalink /patch/76058/
State New
Headers show

Comments

Alexandre Oliva - Dec. 18, 2010, 7:34 a.m.
The tree vectorizer sometimes creates DEFs and removes them before the
end of the pass.  By the time of the removal, the SSA name is still
marked as needing updates, so we skip the code that might introduce a
debug temp before the DEF.

By the time we actually release the DEF, calling that code again, the
DEF is no longer in the stmt seq, so we can't tell where to insert the
debug temp.

We could just drop the debug info on the floor, but if the value is
unchanging, we can still propagate it to debug stmts that use that SSA
name.  This is what this patch does, also avoiding the ICE we got trying
to figure out where to insert the debug temp.

Regstrapped on x86_64-linux-gnu.  Ok to install?
Richard Guenther - Dec. 18, 2010, 8:53 p.m.
On Sat, Dec 18, 2010 at 8:34 AM, Alexandre Oliva <aoliva@redhat.com> wrote:
> The tree vectorizer sometimes creates DEFs and removes them before the
> end of the pass.  By the time of the removal, the SSA name is still
> marked as needing updates, so we skip the code that might introduce a
> debug temp before the DEF.
>
> By the time we actually release the DEF, calling that code again, the
> DEF is no longer in the stmt seq, so we can't tell where to insert the
> debug temp.
>
> We could just drop the debug info on the floor, but if the value is
> unchanging, we can still propagate it to debug stmts that use that SSA
> name.  This is what this patch does, also avoiding the ICE we got trying
> to figure out where to insert the debug temp.
>
> Regstrapped on x86_64-linux-gnu.  Ok to install?

The statements are artificially created by the vectorizer, we should not
create extra debug info for them.  They are also the only statements that
are supposed to be removed by the very simple DCE - but we cannot
identify them reliably.  Is this patch not adding another hack above
the DCE hack I installed?

Richard.

>
>
> --
> 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
>
>
Alexandre Oliva - Dec. 21, 2010, 6:58 a.m.
On Dec 18, 2010, Richard Guenther <richard.guenther@gmail.com> wrote:

> The statements are artificially created by the vectorizer,

Not really.

The loop contains these statements and debug stmts.

SCCP replaces the end-of-loop PHI nodes with final value computations,
making the in-loop DEF dead, but not removing it.

The vectorizer actually vectorizes the loop.

While doing so, it duplicates the original loop into pre- and post-
loops, for proper loop alignment.

Because of the renaming in the post-vectorized loop, we examine the
copied DEF in there and realize it is dead, so we remove it.

If we don't propagate the DEF into the copied debug stmts, we'll make
the variable incorrect within the post-loop.

> They are also the only statements that are supposed to be removed by
> the very simple DCE - but we cannot identify them reliably.  Is this
> patch not adding another hack above the DCE hack I installed?

Dunno, really.  Sure we run this very simple DCE right after SCCP, so as
to remove the dead in-loop DEF then?  It might obviate this change, but
it's not unreasonable to put it in anyway, I think, especially after
reinstate a more general condition to enable the copying.  The
is_gimple_min_invariant() test was a last-minute thought that was
supposed to be extended so as to handle the case at hand but not other
expressions that can't be moved about, but that I forgot about before
testing and posting the patch.  Oops ;-)

Patch

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

	PR debug/46931
	* tree-ssa.c (insert_debug_temp_for_var_def): Handle removed
	assignments.

Index: gcc/tree-ssa.c
===================================================================
--- gcc/tree-ssa.c.orig	2010-12-17 01:07:57.818499387 -0200
+++ gcc/tree-ssa.c	2010-12-17 04:01:43.577493644 -0200
@@ -305,6 +305,8 @@  insert_debug_temp_for_var_def (gimple_st
   gimple def_stmt = NULL;
   int usecount = 0;
   tree value = NULL;
+  tree value_unshare = NULL;
+  tree temp_value = NULL;
 
   if (!MAY_HAVE_DEBUG_STMTS)
     return;
@@ -421,6 +423,12 @@  insert_debug_temp_for_var_def (gimple_st
 		  || is_gimple_min_invariant (value)))
 	  || is_gimple_reg (value))
 	value = unshare_expr (value);
+      else if (!gsi && !gimple_bb (def_stmt))
+	{
+	  if (is_gimple_min_invariant (value))
+	    value_unshare = value;
+	  value = NULL;
+	}
       else
 	{
 	  gimple def_temp;
@@ -454,13 +462,48 @@  insert_debug_temp_for_var_def (gimple_st
       if (!gimple_debug_bind_p (stmt))
 	continue;
 
+      /* If we have a value that needs unsharing, unshare it.  Then,
+	 if the debug stmt binds to VAR, we can replace it, otherwise
+	 we'll create a debug temp, bind it to the unshared value
+	 right before STMT, and replace uses of VAR with the debug
+	 temp.  We reuse the same temp for multiple uses, but we don't
+	 attempt to avoid emitting debug temps that would be dominated
+	 by identical debug bind stmts.  */
+      if (value_unshare)
+	{
+	  value = unshare_expr (value_unshare);
+	  if (gimple_debug_bind_get_value (stmt) != var)
+	    {
+	      gimple def_temp;
+	      gimple_stmt_iterator ngsi = gsi_for_stmt (stmt);
+
+	      if (!temp_value)
+		{
+		  temp_value = make_node (DEBUG_EXPR_DECL);
+
+		  DECL_ARTIFICIAL (temp_value) = 1;
+		  TREE_TYPE (temp_value) = TREE_TYPE (value);
+		  if (DECL_P (value))
+		    DECL_MODE (temp_value) = DECL_MODE (value);
+		  else
+		    DECL_MODE (temp_value) = TYPE_MODE (TREE_TYPE (value));
+		}
+
+	      def_temp = gimple_build_debug_bind (temp_value, value,
+						  def_stmt);
+
+	      gsi_insert_before (&ngsi, def_temp, GSI_SAME_STMT);
+
+	      value = temp_value;
+	    }
+	}
+
       if (value)
 	FOR_EACH_IMM_USE_ON_STMT (use_p, imm_iter)
-	  /* unshare_expr is not needed here.  vexpr is either a
+	  /* unshare_expr is not needed here.  value is either a
 	     SINGLE_RHS, that can be safely shared, some other RHS
 	     that was unshared when we found it had a single debug
-	     use, or a DEBUG_EXPR_DECL, that can be safely
-	     shared.  */
+	     use, or a DEBUG_EXPR_DECL, that can be safely shared.  */
 	  SET_USE (use_p, value);
       else
 	gimple_debug_bind_reset_value (stmt);