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

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

[Alexandre Oliva]

[-- Attachment #1: Type: text/plain, Size: 700 bytes --]

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?


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: vta-remove-ssa-name-before-update-pr46931.patch --]
[-- Type: text/x-diff, Size: 2727 bytes --]

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);

[-- Attachment #3: Type: text/plain, Size: 257 bytes --]


-- 
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

Re: [PR debug/46931] don't crash propagating removed DEFs into debug stmts

[Richard Guenther]

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
>
>

Re: [PR debug/46931] don't crash propagating removed DEFs into debug stmts

[Alexandre Oliva]

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 ;-)

-- 
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

Re: [PR debug/46931] don't crash propagating removed DEFs into debug stmts

[Alexandre Oliva]

[-- Attachment #1: Type: text/plain, Size: 448 bytes --]

On Dec 21, 2010, Alexandre Oliva <aoliva@redhat.com> wrote:

> 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 ;-)

Here's a revised version.  Regression-tested on x86_64-linux-gnu after
bootstrap with BOOT_CFLAGS='-O2 -g -ftree-vectorize'.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: vta-remove-ssa-name-before-update-pr46931.patch --]
[-- Type: text/x-diff, Size: 3266 bytes --]

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-18 06:04:11.804255840 -0200
+++ gcc/tree-ssa.c	2010-12-21 05:05:56.394238678 -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,30 @@ 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_val (value))
+	    value_unshare = value;
+	  else if (TREE_SIDE_EFFECTS (value)
+		   || TREE_THIS_VOLATILE (value)
+		   || !is_gimple_assign (def_stmt))
+	    value_unshare = NULL;
+	  else if (!gimple_assign_single_p (def_stmt))
+	    value_unshare = value;
+	  else if (gimple_has_volatile_ops (def_stmt))
+	    value_unshare = NULL;
+	  else
+	    {
+	      enum tree_code_class tcc;
+	      tcc = TREE_CODE_CLASS (gimple_assign_rhs_code (def_stmt));
+	      if (tcc == tcc_reference || tcc == tcc_declaration)
+		value_unshare = NULL;
+	      else
+		value_unshare = value;
+	    }
+
+	  value = NULL;
+	}
       else
 	{
 	  gimple def_temp;
@@ -454,13 +480,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);

[-- Attachment #3: Type: text/plain, Size: 257 bytes --]


-- 
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

Re: [PR debug/46931] don't crash propagating removed DEFs into debug stmts

[Richard Guenther]

On Wed, Dec 22, 2010 at 4:54 AM, Alexandre Oliva <aoliva@redhat.com> wrote:
> On Dec 21, 2010, Alexandre Oliva <aoliva@redhat.com> wrote:
>
>> 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 ;-)
>
> Here's a revised version.  Regression-tested on x86_64-linux-gnu after
> bootstrap with BOOT_CFLAGS='-O2 -g -ftree-vectorize'.

Hm.  I don't like the awkward flow of operation in your change.  We
on purpose created debug temps for multiple uses to avoid memory
growth.  So, the if (value_unshare) code in the loop adjusting the
uses should go before that loop and unconditionally create a
debug temporary (and thus doesn't need unsharing either).

In fact - for the case of removed statements we simply should
insert the debug temp at the immediate common dominator of
all uses.  Or without implementing this, can't we avoid this in
the simple-DCE code instead?  Like with

Index: tree-vect-loop-manip.c
===================================================================
--- tree-vect-loop-manip.c	(revision 167471)
+++ tree-vect-loop-manip.c	(working copy)
@@ -1442,6 +1442,9 @@
   if (update_first_loop_count)
     slpeel_make_loop_iterate_ntimes (first_loop, first_niters);

+  BITMAP_FREE (definitions);
+  delete_update_ssa ();
+
   /* Remove all pattern statements from the loop copy.  They will confuse
      the expander if DCE is disabled.
      ???  The pattern recognizer should be split into an analysis and
@@ -1451,9 +1454,6 @@

   adjust_vec_debug_stmts ();

-  BITMAP_FREE (definitions);
-  delete_update_ssa ();
-
   return new_loop;
 }


That completely should avoid the names-to-rename issue and I'd be much
more happy with the above for 4.6.

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
>
>

Re: [PR debug/46931] don't crash propagating removed DEFs into debug stmts

[Alexandre Oliva]

On Dec 22, 2010, Richard Guenther <richard.guenther@gmail.com> wrote:

> Hm.  I don't like the awkward flow of operation in your change.  We
> on purpose created debug temps for multiple uses to avoid memory
> growth.

I'm not entirely happy with that either, but the savings kind of only
work if we still know where to put the debug temp.  Considering how rare
the need for this fallback is, I figured it didn't make sense to put too
much effort trying to find a common dominator.

In fact, I'm not sure we can count on a common dominator being the right
insertion point, after certain transformations, or even whether we could
figure out whether or not it is the right point after all.  I figured,
when the DEF was already removed and the RHS is movable, inserting just
before the use is simple and conservatively correct, whereas trying to
locate an insertion point at a common dominator would be trickier, more
expensive and ultimately error prone.

Now, maybe we're better off fixing this one instance of the problem,
like you did (just checked with the given testcase), and leaving the
code that attempts to propagates DEFs into DEBUG STMTs alone, so that it
will still crash should other similar situations arise.

Regstrapping your proposed patch now.

-- 
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

Re: [PR debug/46931] don't crash propagating removed DEFs into debug stmts

[Richard Guenther]

On Thu, Dec 23, 2010 at 9:19 AM, Alexandre Oliva <aoliva@redhat.com> wrote:
> On Dec 22, 2010, Richard Guenther <richard.guenther@gmail.com> wrote:
>
>> Hm.  I don't like the awkward flow of operation in your change.  We
>> on purpose created debug temps for multiple uses to avoid memory
>> growth.
>
> I'm not entirely happy with that either, but the savings kind of only
> work if we still know where to put the debug temp.  Considering how rare
> the need for this fallback is, I figured it didn't make sense to put too
> much effort trying to find a common dominator.
>
> In fact, I'm not sure we can count on a common dominator being the right
> insertion point, after certain transformations, or even whether we could
> figure out whether or not it is the right point after all.  I figured,
> when the DEF was already removed and the RHS is movable, inserting just
> before the use is simple and conservatively correct, whereas trying to
> locate an insertion point at a common dominator would be trickier, more
> expensive and ultimately error prone.

Yeah.  But wasn't there a correctness problem with propagating as well?
Consider

 i_1 = 1;
# i = i_1
 j_2 = i_1 * 2;
# j = j_2
 i_3 = 3;
# i = i_3
 k = j_2;
# k = j_2

and deleting the j = i * 2 statement.  If you propagate into the use
the the debug info will refer to the wrong value of i.  Remember that
we can have overlapping life ranges for SSA names but not for the
decls for which we emit debug info.

> Now, maybe we're better off fixing this one instance of the problem,
> like you did (just checked with the given testcase), and leaving the
> code that attempts to propagates DEFs into DEBUG STMTs alone, so that it
> will still crash should other similar situations arise.
>
> Regstrapping your proposed patch now.

It's ok to commit if it works (with your testcase).

Thanks,
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
>

Re: [PR debug/46931] don't crash propagating removed DEFs into debug stmts

[Alexandre Oliva]

[-- Attachment #1: Type: text/plain, Size: 1016 bytes --]

On Dec 26, 2010, Richard Guenther <richard.guenther@gmail.com> wrote:

> Yeah.  But wasn't there a correctness problem with propagating as well?

> the the debug info will refer to the wrong value of i.  Remember that
> we can have overlapping life ranges for SSA names but not for the
> decls for which we emit debug info.

The value expressions in debug stmts refer to the SSA names, so there's
no problem, at least in this regard.  The overlapping ranges, if
expanded to different pseudos, it will get the correct RTL expressions;
if no longer available at a point, it should not get any RTL expression
(although it would in theory be possible to do better, looking for some
equivalence).

Now, I don't think the latter has been actually verified, especially
after the change that made us go straight from SSA to RTL, so if you
have evidence that we're doing it wrong, I'd love to see it.

>> Regstrapping your proposed patch now.

> It's ok to commit if it works (with your testcase).

Here's what I installed.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: tree-vect-loop-manip-pr46931.patch --]
[-- Type: text/x-diff, Size: 1004 bytes --]

for  gcc/ChangeLog
from  Richard Guenther  <rguenther@suse.de>

	PR debug/46931
	* tree-vect-loop-manip.c (slpeel_tree_peel_loop_to_edge): Update
	SSA before removing dead stmts.

Index: gcc/tree-vect-loop-manip.c
===================================================================
--- gcc/tree-vect-loop-manip.c.orig	2010-12-18 04:28:14.000000000 -0200
+++ gcc/tree-vect-loop-manip.c	2010-12-23 05:50:15.540018010 -0200
@@ -1442,6 +1442,9 @@ slpeel_tree_peel_loop_to_edge (struct lo
   if (update_first_loop_count)
     slpeel_make_loop_iterate_ntimes (first_loop, first_niters);
 
+  BITMAP_FREE (definitions);
+  delete_update_ssa ();
+
   /* Remove all pattern statements from the loop copy.  They will confuse
      the expander if DCE is disabled.
      ???  The pattern recognizer should be split into an analysis and
@@ -1451,9 +1454,6 @@ slpeel_tree_peel_loop_to_edge (struct lo
 
   adjust_vec_debug_stmts ();
 
-  BITMAP_FREE (definitions);
-  delete_update_ssa ();
-
   return new_loop;
 }
 

[-- Attachment #3: Type: text/plain, Size: 257 bytes --]


-- 
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

Re: [PR debug/46931] don't crash propagating removed DEFs into debug stmts

[Richard Guenther]

On Tue, Dec 28, 2010 at 9:26 PM, Alexandre Oliva <aoliva@redhat.com> wrote:
> On Dec 26, 2010, Richard Guenther <richard.guenther@gmail.com> wrote:
>
>> Yeah.  But wasn't there a correctness problem with propagating as well?
>
>> the the debug info will refer to the wrong value of i.  Remember that
>> we can have overlapping life ranges for SSA names but not for the
>> decls for which we emit debug info.
>
> The value expressions in debug stmts refer to the SSA names, so there's
> no problem, at least in this regard.  The overlapping ranges, if
> expanded to different pseudos, it will get the correct RTL expressions;
> if no longer available at a point, it should not get any RTL expression
> (although it would in theory be possible to do better, looking for some
> equivalence).
>
> Now, I don't think the latter has been actually verified, especially
> after the change that made us go straight from SSA to RTL, so if you
> have evidence that we're doing it wrong, I'd love to see it.

I don't have evidence, it's just what I seem to remember (for some
reason).  Btw, we can have memory operands on the RHS of debug
stmts?  For those propagating wouldn't be valid as they are not
in SSA form.

Richard.

>>> Regstrapping your proposed patch now.
>
>> It's ok to commit if it works (with your testcase).
>
> Here's what I installed.
>
>
>
> --
> 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
>
>

Re: [PR debug/46931] don't crash propagating removed DEFs into debug stmts

[Alexandre Oliva]

On Dec 28, 2010, Richard Guenther <richard.guenther@gmail.com> wrote:

> we can have memory operands on the RHS of debug stmts?  For those
> propagating wouldn't be valid as they are not in SSA form.

Yup, the code I posted wouldn't propagate MEMs.

-- 
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