[vta, graphite?] propagate degenerate phi nodes into debug stmts

[vta, graphite?] propagate degenerate phi nodes into debug stmts

[Alexandre Oliva]

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

This patch arranges for PHI nodes to be propagated into debug stmts upon
release, if possible.  This improves debug info quality a bit, for
without this change we'd end up resetting the debug stmt.

It's not clear to me how this helped (if at all) graphite failures with
VTA, but it helped retain debug annotations that enabled me to validate
the fix for PR 41926 in a C testcase.  But that's something for another
patch.

Bootstrapped on x86_64-linux-gnu; regstrapping on that and
ia64-linux-gnu.  Ok to install?


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: vta-ssa-dom-prop-degenerate.patch --]
[-- Type: text/x-diff, Size: 3185 bytes --]

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

	* tree-ssa.c (find_released_ssa_name): Handle NULL wi.
	(insert_debug_temp_for_var_def): Handle degenerate PHI nodes.
	Fix handling of released SSA names.
	(insert_debug_temps_for_defs): Handle PHI nodes.

Index: gcc/tree-ssa.c
===================================================================
--- gcc/tree-ssa.c.orig	2009-11-08 05:09:36.000000000 -0200
+++ gcc/tree-ssa.c	2009-11-08 05:34:12.000000000 -0200
@@ -279,7 +279,7 @@ find_released_ssa_name (tree *tp, int *w
 {
   struct walk_stmt_info *wi = (struct walk_stmt_info *) data_;
 
-  if (wi->is_lhs)
+  if (wi && wi->is_lhs)
     return NULL_TREE;
 
   if (TREE_CODE (*tp) == SSA_NAME)
@@ -346,11 +346,30 @@ insert_debug_temp_for_var_def (gimple_st
   /* If we didn't get an insertion point, and the stmt has already
      been removed, we won't be able to insert the debug bind stmt, so
      we'll have to drop debug information.  */
-  if (is_gimple_assign (def_stmt))
+  if (gimple_code (def_stmt) == GIMPLE_PHI || is_gimple_assign (def_stmt))
     {
       bool no_value = false;
 
-      if (!dom_info_available_p (CDI_DOMINATORS))
+      if (gimple_code (def_stmt) == GIMPLE_PHI)
+	{
+	  struct walk_stmt_info wi;
+	  size_t i;
+
+	  memset (&wi, 0, sizeof (wi));
+
+	  for (i = 0; i < gimple_phi_num_args (def_stmt); i++)
+	    {
+	      tree *argp = gimple_phi_arg_def_ptr (def_stmt, i);
+
+	      if (!*argp
+		  || walk_tree (argp, find_released_ssa_name, NULL, NULL))
+		{
+		  no_value = true;
+		  break;
+		}
+	    }
+	}
+      else if (!dom_info_available_p (CDI_DOMINATORS))
 	{
 	  struct walk_stmt_info wi;
 
@@ -383,13 +402,22 @@ insert_debug_temp_for_var_def (gimple_st
 	     dead SSA NAMEs.  SSA verification shall catch any
 	     errors.  */
 	  if ((!gsi && !gimple_bb (def_stmt))
-	      || !walk_gimple_op (def_stmt, find_released_ssa_name,
-				  &wi))
+	      || walk_gimple_op (def_stmt, find_released_ssa_name, &wi))
 	    no_value = true;
 	}
 
       if (!no_value)
-	value = gimple_assign_rhs_to_tree (def_stmt);
+	{
+	  if (gimple_code (def_stmt) == GIMPLE_PHI)
+	    /* ??? We could handle at least some non-generate PHI
+	       nodes by inserting debug temps on every edge.  It's
+	       not clear how much this would improve debug info.  */
+	    value = degenerate_phi_result (def_stmt);
+	  else if (is_gimple_assign (def_stmt))
+	    value = gimple_assign_rhs_to_tree (def_stmt);
+	  else
+	    gcc_unreachable ();
+	}
     }
 
   if (value)
@@ -409,6 +437,7 @@ insert_debug_temp_for_var_def (gimple_st
 	 at the expense of duplication of expressions.  */
 
       if (CONSTANT_CLASS_P (value)
+	  || gimple_code (def_stmt) == GIMPLE_PHI
 	  || (usecount == 1
 	      && (!gimple_assign_single_p (def_stmt)
 		  || is_gimple_min_invariant (value)))
@@ -479,6 +508,13 @@ insert_debug_temps_for_defs (gimple_stmt
 
   stmt = gsi_stmt (*gsi);
 
+  if (gimple_code (stmt) == GIMPLE_PHI)
+    {
+      tree var = gimple_phi_result (stmt);
+      insert_debug_temp_for_var_def (gsi, var);
+      return;
+    }
+
   FOR_EACH_SSA_DEF_OPERAND (def_p, stmt, op_iter, SSA_OP_DEF)
     {
       tree var = DEF_FROM_PTR (def_p);

[-- 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: [vta, graphite?] propagate degenerate phi nodes into debug stmts

[Richard Guenther]

On Sun, Nov 8, 2009 at 8:46 AM, Alexandre Oliva <aoliva@redhat.com> wrote:
> This patch arranges for PHI nodes to be propagated into debug stmts upon
> release, if possible.  This improves debug info quality a bit, for
> without this change we'd end up resetting the debug stmt.
>
> It's not clear to me how this helped (if at all) graphite failures with
> VTA, but it helped retain debug annotations that enabled me to validate
> the fix for PR 41926 in a C testcase.  But that's something for another
> patch.
>
> Bootstrapped on x86_64-linux-gnu; regstrapping on that and
> ia64-linux-gnu.  Ok to install?

@@ -383,13 +402,22 @@ insert_debug_temp_for_var_def (gimple_st
             dead SSA NAMEs.  SSA verification shall catch any
             errors.  */
          if ((!gsi && !gimple_bb (def_stmt))
-             || !walk_gimple_op (def_stmt, find_released_ssa_name,
-                                 &wi))
+             || walk_gimple_op (def_stmt, find_released_ssa_name, &wi))
            no_value = true;

please install this as a separate revision - it seems to be an unrelated bugfix.

For the rest it would be better to re-organize the code as

-  if (is_gimple_assign (def_stmt))
     {
... keep unchanged ...
     }
   else if (gimple_code (def_stmt) == PHI_NODE)
     {
        value = degenerate_phi_result (def_stmt);
        if (value
            && walk_tree (&value, find_released_ssa_name, NULL, NULL))
          value = NULL_TREE;
     }

that looks simpler and it avoids walking PHI args uselessly.

@@ -479,6 +508,13 @@ insert_debug_temps_for_defs (gimple_stmt

   stmt = gsi_stmt (*gsi);

+  if (gimple_code (stmt) == GIMPLE_PHI)
+    {
+      tree var = gimple_phi_result (stmt);
+      insert_debug_temp_for_var_def (gsi, var);
+      return;
+    }
+

This looks odd.  SSA DEF operand iteration should walk the PHI defs
as well, so the change should not be necessary.

Ok with that changes.

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: [vta, graphite?] propagate degenerate phi nodes into debug stmts

[Alexandre Oliva]

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

On Nov  8, 2009, Richard Guenther <richard.guenther@gmail.com> wrote:

> @@ -383,13 +402,22 @@ insert_debug_temp_for_var_def (gimple_st
>              dead SSA NAMEs.  SSA verification shall catch any
>              errors.  */
>           if ((!gsi && !gimple_bb (def_stmt))
> -             || !walk_gimple_op (def_stmt, find_released_ssa_name,
> -                                 &wi))
> +             || walk_gimple_op (def_stmt, find_released_ssa_name, &wi))
>             no_value = true;

> please install this as a separate revision - it seems to be an unrelated bugfix.

Here's the separate patch for this bit.  I'll check it in once
regtesting is done.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: vta-debug-temp-released-name-fix.patch --]
[-- Type: text/x-diff, Size: 701 bytes --]

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

	* tree-ssa.c (insert_debug_temp_for_var_def): Fix handling of
	released SSA names.

Index: gcc/tree-ssa.c
===================================================================
--- gcc/tree-ssa.c.orig	2009-11-16 18:23:18.000000000 -0200
+++ gcc/tree-ssa.c	2009-11-16 18:24:07.000000000 -0200
@@ -383,8 +383,7 @@ insert_debug_temp_for_var_def (gimple_st
 	     dead SSA NAMEs.  SSA verification shall catch any
 	     errors.  */
 	  if ((!gsi && !gimple_bb (def_stmt))
-	      || !walk_gimple_op (def_stmt, find_released_ssa_name,
-				  &wi))
+	      || walk_gimple_op (def_stmt, find_released_ssa_name, &wi))
 	    no_value = true;
 	}
 

[-- 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: [vta, graphite?] propagate degenerate phi nodes into debug stmts

[Alexandre Oliva]

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

On Nov  8, 2009, Richard Guenther <richard.guenther@gmail.com> wrote:

> For the rest it would be better to re-organize the code as

>    else if (gimple_code (def_stmt) == PHI_NODE)
>      {
>         value = degenerate_phi_result (def_stmt);
>         if (value
>             && walk_tree (&value, find_released_ssa_name, NULL, NULL))
>           value = NULL_TREE;
>      }

> that looks simpler and it avoids walking PHI args uselessly.

That doesn't work.  We crash deep within degenerate_phi_result given
expressions containing released SSA names.  It's the same reason why we
have to test for released SSA names before calling
gimple_assign_rhs_to_tree: IIRC it has to do with testing whether the
already-NULL type of the SSA name is a pointer type.

Regardless, I reorganized the code so as to not have to test for
PHI_NODEs as often.  There were two unrelated code paths intermixed with
tests every now and again.  Now they're two separate code flows.

> @@ -479,6 +508,13 @@ insert_debug_temps_for_defs (gimple_stmt

>    stmt = gsi_stmt (*gsi);

> +  if (gimple_code (stmt) == GIMPLE_PHI)
> +    {
> +      tree var = gimple_phi_result (stmt);
> +      insert_debug_temp_for_var_def (gsi, var);
> +      return;
> +    }
> +

> This looks odd.  SSA DEF operand iteration should walk the PHI defs
> as well, so the change should not be necessary.

I thought so, too, but by the time we get there, the operands of the PHI
stmt have already been disconnected.

Here's what I'm going to test now.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: vta-ssa-dom-prop-degenerate.patch --]
[-- Type: text/x-diff, Size: 2580 bytes --]

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

	* tree-ssa.c (find_released_ssa_name): Handle NULL wi.
	(insert_debug_temp_for_var_def): Handle degenerate PHI nodes.
	(insert_debug_temps_for_defs): Handle PHI nodes.

Index: gcc/tree-ssa.c
===================================================================
--- gcc/tree-ssa.c.orig	2009-11-16 18:24:07.000000000 -0200
+++ gcc/tree-ssa.c	2009-11-16 18:41:04.000000000 -0200
@@ -279,7 +279,7 @@ find_released_ssa_name (tree *tp, int *w
 {
   struct walk_stmt_info *wi = (struct walk_stmt_info *) data_;
 
-  if (wi->is_lhs)
+  if (wi && wi->is_lhs)
     return NULL_TREE;
 
   if (TREE_CODE (*tp) == SSA_NAME)
@@ -346,7 +346,33 @@ insert_debug_temp_for_var_def (gimple_st
   /* If we didn't get an insertion point, and the stmt has already
      been removed, we won't be able to insert the debug bind stmt, so
      we'll have to drop debug information.  */
-  if (is_gimple_assign (def_stmt))
+  if (gimple_code (def_stmt) == GIMPLE_PHI)
+    {
+      bool no_value = false;
+      struct walk_stmt_info wi;
+      size_t i;
+
+      memset (&wi, 0, sizeof (wi));
+
+      for (i = 0; i < gimple_phi_num_args (def_stmt); i++)
+	{
+	  tree *argp = gimple_phi_arg_def_ptr (def_stmt, i);
+
+	  if (!*argp
+	      || walk_tree (argp, find_released_ssa_name, NULL, NULL))
+	    {
+	      no_value = true;
+	      break;
+	    }
+	}
+
+      if (!no_value)
+	/* ??? We could handle at least some non-generate PHI
+	   nodes by inserting debug temps on every edge.  It's
+	   not clear how much this would improve debug info.  */
+	value = degenerate_phi_result (def_stmt);
+    }
+  else if (is_gimple_assign (def_stmt))
     {
       bool no_value = false;
 
@@ -408,6 +434,7 @@ insert_debug_temp_for_var_def (gimple_st
 	 at the expense of duplication of expressions.  */
 
       if (CONSTANT_CLASS_P (value)
+	  || gimple_code (def_stmt) == GIMPLE_PHI
 	  || (usecount == 1
 	      && (!gimple_assign_single_p (def_stmt)
 		  || is_gimple_min_invariant (value)))
@@ -478,6 +505,16 @@ insert_debug_temps_for_defs (gimple_stmt
 
   stmt = gsi_stmt (*gsi);
 
+  /* By the time we get here, the DEF in the PHI node seems to have
+     already been disconnected as an operand, so the FOR_EACH below
+     has no effect.  Handle it explicitly.  */
+  if (gimple_code (stmt) == GIMPLE_PHI)
+    {
+      tree var = gimple_phi_result (stmt);
+      insert_debug_temp_for_var_def (gsi, var);
+      return;
+    }
+
   FOR_EACH_SSA_DEF_OPERAND (def_p, stmt, op_iter, SSA_OP_DEF)
     {
       tree var = DEF_FROM_PTR (def_p);

[-- 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: [vta, graphite?] propagate degenerate phi nodes into debug stmts

[Richard Guenther]

On Mon, Nov 16, 2009 at 9:41 PM, Alexandre Oliva <aoliva@redhat.com> wrote:
> On Nov  8, 2009, Richard Guenther <richard.guenther@gmail.com> wrote:
>
>> For the rest it would be better to re-organize the code as
>
>>    else if (gimple_code (def_stmt) == PHI_NODE)
>>      {
>>         value = degenerate_phi_result (def_stmt);
>>         if (value
>>             && walk_tree (&value, find_released_ssa_name, NULL, NULL))
>>           value = NULL_TREE;
>>      }
>
>> that looks simpler and it avoids walking PHI args uselessly.
>
> That doesn't work.  We crash deep within degenerate_phi_result given
> expressions containing released SSA names.  It's the same reason why we
> have to test for released SSA names before calling
> gimple_assign_rhs_to_tree: IIRC it has to do with testing whether the
> already-NULL type of the SSA name is a pointer type.

Well, just adjust degenerate_phi_result to do instead of calling
operand_equal_p

  else if (TREE_CODE (arg) != TREE_CODE (val)
             || (TREE_CODE (arg) == SSA_NAME
                 && arg != val)
             || !operand_equal_p (arg, val, 0))
    break;

that should fix it.  Alternatively add the SSA_NAME shortcut
to operand_equal_p.

> Regardless, I reorganized the code so as to not have to test for
> PHI_NODEs as often.  There were two unrelated code paths intermixed with
> tests every now and again.  Now they're two separate code flows.
>
>> @@ -479,6 +508,13 @@ insert_debug_temps_for_defs (gimple_stmt
>
>>    stmt = gsi_stmt (*gsi);
>
>> +  if (gimple_code (stmt) == GIMPLE_PHI)
>> +    {
>> +      tree var = gimple_phi_result (stmt);
>> +      insert_debug_temp_for_var_def (gsi, var);
>> +      return;
>> +    }
>> +
>
>> This looks odd.  SSA DEF operand iteration should walk the PHI defs
>> as well, so the change should not be necessary.
>
> I thought so, too, but by the time we get there, the operands of the PHI
> stmt have already been disconnected.

It shouldn't be.  Please try to figure out why instead.

Thanks,
Richard.

> Here's what I'm going to test 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: [vta, graphite?] propagate degenerate phi nodes into debug stmts

[Alexandre Oliva]

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

On Nov 17, 2009, Richard Guenther <richard.guenther@gmail.com> wrote:

>>> This looks odd.  SSA DEF operand iteration should walk the PHI defs
>>> as well, so the change should not be necessary.

>> I thought so, too, but by the time we get there, the operands of the PHI
>> stmt have already been disconnected.

> It shouldn't be.  Please try to figure out why instead.

Gotta use a different FOR_EACH macro to handle PHI nodes.  

s/FOR_EACH_SSA_DEF_OPERAND/FOR_EACH_PHI_OR_STMT_DEF/ fixed it.

In order to make sure no other such mistakes had been made in GCC, I
added an assertion check in the iterator initializer and adjusted the
uses of GIMPLE_PHI nodes that triggered the assertion, but that would
have done nothing whatsoever in its absence.  I haven't looked into
whether doing nothing is correct.

Should I check this in?


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: check-no-nonphi-iter-on-phi.patch --]
[-- Type: text/x-diff, Size: 2111 bytes --]

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

	* tree-flow-inline.h (op_iter_init): Reject GIMPLE_PHI stmts.
	(num_ssa_operands): Skip GIMPLE_PHI.
	(delink_stmt_imm_use): Likewise.
	* tree-ssa-pre.c (remove_dead_inserted_code): Likewise.

Index: gcc/tree-flow-inline.h
===================================================================
--- gcc/tree-flow-inline.h.orig	2009-11-18 18:06:04.000000000 -0200
+++ gcc/tree-flow-inline.h	2009-11-18 18:13:13.000000000 -0200
@@ -801,6 +801,8 @@ op_iter_init (ssa_op_iter *ptr, gimple s
      iterating over defs or uses at the same time.  */
   gcc_assert ((!(flags & SSA_OP_VDEF) || (flags & SSA_OP_DEF))
 	      && (!(flags & SSA_OP_VUSE) || (flags & SSA_OP_USE)));
+  /* PHI nodes require a different iterator initialization path.  */
+  gcc_assert (gimple_code (stmt) != GIMPLE_PHI);
   ptr->defs = (flags & (SSA_OP_DEF|SSA_OP_VDEF)) ? gimple_def_ops (stmt) : NULL;
   if (!(flags & SSA_OP_VDEF)
       && ptr->defs
@@ -928,8 +930,9 @@ num_ssa_operands (gimple stmt, int flags
   tree t;
   int num = 0;
 
-  FOR_EACH_SSA_TREE_OPERAND (t, stmt, iter, flags)
-    num++;
+  if (gimple_code (stmt) != GIMPLE_PHI)
+    FOR_EACH_SSA_TREE_OPERAND (t, stmt, iter, flags)
+      num++;
   return num;
 }
 
@@ -941,7 +944,8 @@ delink_stmt_imm_use (gimple stmt)
    ssa_op_iter iter;
    use_operand_p use_p;
 
-   if (ssa_operands_active ())
+   if (ssa_operands_active ()
+       && gimple_code (stmt) != GIMPLE_PHI)
      FOR_EACH_SSA_USE_OPERAND (use_p, stmt, iter, SSA_OP_ALL_USES)
        delink_imm_use (use_p);
 }
Index: gcc/tree-ssa-pre.c
===================================================================
--- gcc/tree-ssa-pre.c.orig	2009-11-18 18:23:32.000000000 -0200
+++ gcc/tree-ssa-pre.c	2009-11-18 18:23:46.000000000 -0200
@@ -4462,8 +4462,10 @@ remove_dead_inserted_code (void)
 	  if (gimple_code (t) == GIMPLE_PHI)
 	    remove_phi_node (&gsi, true);
 	  else
-	    gsi_remove (&gsi, true);
-	  release_defs (t);
+	    {
+	      gsi_remove (&gsi, true);
+	      release_defs (t);
+	    }
 	}
     }
   VEC_free (gimple, heap, worklist);

[-- 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: [vta, graphite?] propagate degenerate phi nodes into debug stmts

[Alexandre Oliva]

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

On Nov 17, 2009, Richard Guenther <richard.guenther@gmail.com> wrote:

> Well, just adjust degenerate_phi_result to do instead of calling
> operand_equal_p

>   else if (TREE_CODE (arg) != TREE_CODE (val)
>              || (TREE_CODE (arg) == SSA_NAME
>                  && arg != val)
>              || !operand_equal_p (arg, val, 0))
>     break;

> that should fix it.

Good idea.  This wouldn't avoid calling operand_equal_p with two
different SSA_NAMEs, one of which may have already been released, but it
wasn't too hard to arrange for it not to do so.

Tweaking operand_equal_p() itself could have been a nicer change, but I
couldn't figure out how to do it properly.  We test types before
stripping NOPs that could have yielded the same SSA names.  I wouldn't
like to slow things down testing for NULL types or special-casing
SSA_NAMEs too much.

Here's what I'm testing now.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: vta-ssa-dom-prop-degenerate.patch --]
[-- Type: text/x-diff, Size: 2673 bytes --]

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

	* tree-ssa.c (find_released_ssa_name): Handle NULL wi.
	(insert_debug_temp_for_var_def): Handle degenerate PHI nodes.
	(insert_debug_temps_for_defs): Handle PHI nodes.
	* tree-ssa-dom.c (degenerate_phi_result): Don't crash on released
	SSA names.

Index: gcc/tree-ssa.c
===================================================================
--- gcc/tree-ssa.c.orig	2009-11-19 01:14:40.000000000 -0200
+++ gcc/tree-ssa.c	2009-11-19 01:49:29.000000000 -0200
@@ -279,7 +279,7 @@ find_released_ssa_name (tree *tp, int *w
 {
   struct walk_stmt_info *wi = (struct walk_stmt_info *) data_;
 
-  if (wi->is_lhs)
+  if (wi && wi->is_lhs)
     return NULL_TREE;
 
   if (TREE_CODE (*tp) == SSA_NAME)
@@ -346,7 +346,13 @@ insert_debug_temp_for_var_def (gimple_st
   /* If we didn't get an insertion point, and the stmt has already
      been removed, we won't be able to insert the debug bind stmt, so
      we'll have to drop debug information.  */
-  if (is_gimple_assign (def_stmt))
+  if (gimple_code (def_stmt) == GIMPLE_PHI)
+    {
+      value = degenerate_phi_result (def_stmt);
+      if (value && walk_tree (&value, find_released_ssa_name, NULL, NULL))
+	value = NULL;
+    }
+  else if (is_gimple_assign (def_stmt))
     {
       bool no_value = false;
 
@@ -408,6 +414,7 @@ insert_debug_temp_for_var_def (gimple_st
 	 at the expense of duplication of expressions.  */
 
       if (CONSTANT_CLASS_P (value)
+	  || gimple_code (def_stmt) == GIMPLE_PHI
 	  || (usecount == 1
 	      && (!gimple_assign_single_p (def_stmt)
 		  || is_gimple_min_invariant (value)))
@@ -478,7 +485,7 @@ insert_debug_temps_for_defs (gimple_stmt
 
   stmt = gsi_stmt (*gsi);
 
-  FOR_EACH_SSA_DEF_OPERAND (def_p, stmt, op_iter, SSA_OP_DEF)
+  FOR_EACH_PHI_OR_STMT_DEF (def_p, stmt, op_iter, SSA_OP_DEF)
     {
       tree var = DEF_FROM_PTR (def_p);
 
Index: gcc/tree-ssa-dom.c
===================================================================
--- gcc/tree-ssa-dom.c.orig	2009-11-19 01:55:09.000000000 -0200
+++ gcc/tree-ssa-dom.c	2009-11-19 02:00:12.000000000 -0200
@@ -2398,7 +2398,14 @@ degenerate_phi_result (gimple phi)
 	continue;
       else if (!val)
 	val = arg;
-      else if (!operand_equal_p (arg, val, 0))
+      else if (arg == val)
+	continue;
+      /* We bring in some of operand_equal_p not only to speed things
+	 up, but also to avoid crashing when dereferencing the type of
+	 a released SSA name.  */
+      else if (TREE_CODE (val) != TREE_CODE (arg)
+	       || TREE_CODE (val) == SSA_NAME
+	       || !operand_equal_p (arg, val, 0))
 	break;
     }
   return (i == gimple_phi_num_args (phi) ? val : NULL);

[-- 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: [vta, graphite?] propagate degenerate phi nodes into debug stmts

[Richard Guenther]

On Thu, Nov 19, 2009 at 4:05 AM, Alexandre Oliva <aoliva@redhat.com> wrote:
> On Nov 17, 2009, Richard Guenther <richard.guenther@gmail.com> wrote:
>
>>>> This looks odd.  SSA DEF operand iteration should walk the PHI defs
>>>> as well, so the change should not be necessary.
>
>>> I thought so, too, but by the time we get there, the operands of the PHI
>>> stmt have already been disconnected.
>
>> It shouldn't be.  Please try to figure out why instead.
>
> Gotta use a different FOR_EACH macro to handle PHI nodes.
>
> s/FOR_EACH_SSA_DEF_OPERAND/FOR_EACH_PHI_OR_STMT_DEF/ fixed it.
>
> In order to make sure no other such mistakes had been made in GCC, I
> added an assertion check in the iterator initializer and adjusted the
> uses of GIMPLE_PHI nodes that triggered the assertion, but that would
> have done nothing whatsoever in its absence.  I haven't looked into
> whether doing nothing is correct.
>
> Should I check this in?

Ah, hm.  The num_ssa_operands and delink_stmt_imm_use
changes were necessary because they triggered the assert?
I wonder if the callers were expecting them to be a no-op...

The tre-ssa-pre.c change is ok.  I think we should rather
let num_ssa_operands and delink_stmt_imm_use ICE on PHIs,
but I'd rather do this in stage1 - can you queue this patch until then?

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: [vta, graphite?] propagate degenerate phi nodes into debug stmts

[Richard Guenther]

On Thu, Nov 19, 2009 at 5:27 AM, Alexandre Oliva <aoliva@redhat.com> wrote:
> On Nov 17, 2009, Richard Guenther <richard.guenther@gmail.com> wrote:
>
>> Well, just adjust degenerate_phi_result to do instead of calling
>> operand_equal_p
>
>>   else if (TREE_CODE (arg) != TREE_CODE (val)
>>              || (TREE_CODE (arg) == SSA_NAME
>>                  && arg != val)
>>              || !operand_equal_p (arg, val, 0))
>>     break;
>
>> that should fix it.
>
> Good idea.  This wouldn't avoid calling operand_equal_p with two
> different SSA_NAMEs, one of which may have already been released, but it
> wasn't too hard to arrange for it not to do so.
>
> Tweaking operand_equal_p() itself could have been a nicer change, but I
> couldn't figure out how to do it properly.  We test types before
> stripping NOPs that could have yielded the same SSA names.  I wouldn't
> like to slow things down testing for NULL types or special-casing
> SSA_NAMEs too much.
>
> Here's what I'm testing now.

Ok if it passes testing.

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: [vta, graphite?] propagate degenerate phi nodes into debug stmts

[Alexandre Oliva]

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

On Nov 19, 2009, Richard Guenther <richard.guenther@gmail.com> wrote:

> On Thu, Nov 19, 2009 at 5:27 AM, Alexandre Oliva <aoliva@redhat.com> wrote:
>> On Nov 17, 2009, Richard Guenther <richard.guenther@gmail.com> wrote:
>> 
>>> Well, just adjust degenerate_phi_result to do instead of calling
>>> operand_equal_p
>> 
>>>   else if (TREE_CODE (arg) != TREE_CODE (val)
>>>              || (TREE_CODE (arg) == SSA_NAME
>>>                  && arg != val)
>>>              || !operand_equal_p (arg, val, 0))
>>>     break;
>> 
>>> that should fix it.
>> 
>> Good idea.

>> Here's what I'm testing now.

> Ok if it passes testing.

Thanks, but arg could be NULL, and then TREE_CODE (val) would crash.

The fixed patch below corrects the two regressions I got testing the
patch on x86_64-linux-gnu.  Save for new regressions, I'll check in the
revised version as soon as my current round of testing completes.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: vta-ssa-dom-prop-degenerate.patch --]
[-- Type: text/x-diff, Size: 2681 bytes --]

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

	* tree-ssa.c (find_released_ssa_name): Handle NULL wi.
	(insert_debug_temp_for_var_def): Handle degenerate PHI nodes.
	(insert_debug_temps_for_defs): Handle PHI nodes.
	* tree-ssa-dom.c (degenerate_phi_result): Don't crash on released
	SSA names.

Index: gcc/tree-ssa.c
===================================================================
--- gcc/tree-ssa.c.orig	2009-11-19 05:12:30.000000000 -0200
+++ gcc/tree-ssa.c	2009-11-19 05:12:35.000000000 -0200
@@ -279,7 +279,7 @@ find_released_ssa_name (tree *tp, int *w
 {
   struct walk_stmt_info *wi = (struct walk_stmt_info *) data_;
 
-  if (wi->is_lhs)
+  if (wi && wi->is_lhs)
     return NULL_TREE;
 
   if (TREE_CODE (*tp) == SSA_NAME)
@@ -346,7 +346,13 @@ insert_debug_temp_for_var_def (gimple_st
   /* If we didn't get an insertion point, and the stmt has already
      been removed, we won't be able to insert the debug bind stmt, so
      we'll have to drop debug information.  */
-  if (is_gimple_assign (def_stmt))
+  if (gimple_code (def_stmt) == GIMPLE_PHI)
+    {
+      value = degenerate_phi_result (def_stmt);
+      if (value && walk_tree (&value, find_released_ssa_name, NULL, NULL))
+	value = NULL;
+    }
+  else if (is_gimple_assign (def_stmt))
     {
       bool no_value = false;
 
@@ -408,6 +414,7 @@ insert_debug_temp_for_var_def (gimple_st
 	 at the expense of duplication of expressions.  */
 
       if (CONSTANT_CLASS_P (value)
+	  || gimple_code (def_stmt) == GIMPLE_PHI
 	  || (usecount == 1
 	      && (!gimple_assign_single_p (def_stmt)
 		  || is_gimple_min_invariant (value)))
@@ -478,7 +485,7 @@ insert_debug_temps_for_defs (gimple_stmt
 
   stmt = gsi_stmt (*gsi);
 
-  FOR_EACH_SSA_DEF_OPERAND (def_p, stmt, op_iter, SSA_OP_DEF)
+  FOR_EACH_PHI_OR_STMT_DEF (def_p, stmt, op_iter, SSA_OP_DEF)
     {
       tree var = DEF_FROM_PTR (def_p);
 
Index: gcc/tree-ssa-dom.c
===================================================================
--- gcc/tree-ssa-dom.c.orig	2009-11-19 05:12:30.000000000 -0200
+++ gcc/tree-ssa-dom.c	2009-11-20 04:30:10.000000000 -0200
@@ -2398,7 +2398,14 @@ degenerate_phi_result (gimple phi)
 	continue;
       else if (!val)
 	val = arg;
-      else if (!operand_equal_p (arg, val, 0))
+      else if (arg == val)
+	continue;
+      /* We bring in some of operand_equal_p not only to speed things
+	 up, but also to avoid crashing when dereferencing the type of
+	 a released SSA name.  */
+      else if (!arg || TREE_CODE (val) != TREE_CODE (arg)
+	       || TREE_CODE (val) == SSA_NAME
+	       || !operand_equal_p (arg, val, 0))
 	break;
     }
   return (i == gimple_phi_num_args (phi) ? val : NULL);

[-- 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: [vta, graphite?] propagate degenerate phi nodes into debug stmts

[Alexandre Oliva]

On Nov 19, 2009, Richard Guenther <richard.guenther@gmail.com> wrote:

> Ah, hm.  The num_ssa_operands and delink_stmt_imm_use
> changes were necessary because they triggered the assert?

Yup.

> I wonder if the callers were expecting them to be a no-op...

That will be something to be looked into.

> The tre-ssa-pre.c change is ok.  I think we should rather
> let num_ssa_operands and delink_stmt_imm_use ICE on PHIs,
> but I'd rather do this in stage1 - can you queue this patch until then?

You mean queue it all up, including the tree-ssa-pre.c change?

-- 
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: [vta, graphite?] propagate degenerate phi nodes into debug stmts

[Richard Guenther]

On Fri, Nov 20, 2009 at 10:25 AM, Alexandre Oliva <aoliva@redhat.com> wrote:
> On Nov 19, 2009, Richard Guenther <richard.guenther@gmail.com> wrote:
>
>> Ah, hm.  The num_ssa_operands and delink_stmt_imm_use
>> changes were necessary because they triggered the assert?
>
> Yup.
>
>> I wonder if the callers were expecting them to be a no-op...
>
> That will be something to be looked into.
>
>> The tre-ssa-pre.c change is ok.  I think we should rather
>> let num_ssa_operands and delink_stmt_imm_use ICE on PHIs,
>> but I'd rather do this in stage1 - can you queue this patch until then?
>
> You mean queue it all up, including the tree-ssa-pre.c change?

No, the tree-ssa-pre.c change is fine anyways.

Richard.

Re: [vta, graphite?] propagate degenerate phi nodes into debug stmts

[Alexandre Oliva]

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

On Nov 20, 2009, Richard Guenther <richard.guenther@gmail.com> wrote:

> No, the tree-ssa-pre.c change is fine anyways.

Ok, here's what I'm checking in.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: ssa-pre-no-release-after-remove-phi-node.patch --]
[-- Type: text/x-diff, Size: 688 bytes --]

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

	* tree-ssa-pre.c (remove_dead_inserted_code): Don't release_defs
	after remove_phi_node.

Index: gcc/tree-ssa-pre.c
===================================================================
--- gcc/tree-ssa-pre.c.orig	2009-11-19 01:14:18.000000000 -0200
+++ gcc/tree-ssa-pre.c	2009-11-19 01:14:52.000000000 -0200
@@ -4462,8 +4462,10 @@ remove_dead_inserted_code (void)
 	  if (gimple_code (t) == GIMPLE_PHI)
 	    remove_phi_node (&gsi, true);
 	  else
-	    gsi_remove (&gsi, true);
-	  release_defs (t);
+	    {
+	      gsi_remove (&gsi, true);
+	      release_defs (t);
+	    }
 	}
     }
   VEC_free (gimple, heap, worklist);

[-- 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: [vta, graphite?] propagate degenerate phi nodes into debug stmts

[H.J. Lu]

On Fri, Nov 20, 2009 at 1:23 AM, Alexandre Oliva <aoliva@redhat.com> wrote:
> On Nov 19, 2009, Richard Guenther <richard.guenther@gmail.com> wrote:
>
>> On Thu, Nov 19, 2009 at 5:27 AM, Alexandre Oliva <aoliva@redhat.com> wrote:
>>> On Nov 17, 2009, Richard Guenther <richard.guenther@gmail.com> wrote:
>>>
>>>> Well, just adjust degenerate_phi_result to do instead of calling
>>>> operand_equal_p
>>>
>>>>   else if (TREE_CODE (arg) != TREE_CODE (val)
>>>>              || (TREE_CODE (arg) == SSA_NAME
>>>>                  && arg != val)
>>>>              || !operand_equal_p (arg, val, 0))
>>>>     break;
>>>
>>>> that should fix it.
>>>
>>> Good idea.
>
>>> Here's what I'm testing now.
>
>> Ok if it passes testing.
>
> Thanks, but arg could be NULL, and then TREE_CODE (val) would crash.
>
> The fixed patch below corrects the two regressions I got testing the
> patch on x86_64-linux-gnu.  Save for new regressions, I'll check in the
> revised version as soon as my current round of testing completes.
>
>

This patch caused:

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=42299


-- 
H.J.

Re: [vta, graphite?] propagate degenerate phi nodes into debug stmts

[H.J. Lu]

On Sat, Dec 5, 2009 at 5:02 AM, H.J. Lu <hjl.tools@gmail.com> wrote:
> On Fri, Nov 20, 2009 at 1:23 AM, Alexandre Oliva <aoliva@redhat.com> wrote:
>> On Nov 19, 2009, Richard Guenther <richard.guenther@gmail.com> wrote:
>>
>>> On Thu, Nov 19, 2009 at 5:27 AM, Alexandre Oliva <aoliva@redhat.com> wrote:
>>>> On Nov 17, 2009, Richard Guenther <richard.guenther@gmail.com> wrote:
>>>>
>>>>> Well, just adjust degenerate_phi_result to do instead of calling
>>>>> operand_equal_p
>>>>
>>>>>   else if (TREE_CODE (arg) != TREE_CODE (val)
>>>>>              || (TREE_CODE (arg) == SSA_NAME
>>>>>                  && arg != val)
>>>>>              || !operand_equal_p (arg, val, 0))
>>>>>     break;
>>>>
>>>>> that should fix it.
>>>>
>>>> Good idea.
>>
>>>> Here's what I'm testing now.
>>
>>> Ok if it passes testing.
>>
>> Thanks, but arg could be NULL, and then TREE_CODE (val) would crash.
>>
>> The fixed patch below corrects the two regressions I got testing the
>> patch on x86_64-linux-gnu.  Save for new regressions, I'll check in the
>> revised version as soon as my current round of testing completes.
>>
>>
>
> This patch caused:
>
> http://gcc.gnu.org/bugzilla/show_bug.cgi?id=42299
>

This patch also caused:

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=42395

-- 
H.J.

Re: [vta, graphite?] propagate degenerate phi nodes into debug stmts

[H.J. Lu]

On Tue, Dec 22, 2009 at 4:55 PM, H.J. Lu <hjl.tools@gmail.com> wrote:
> On Sat, Dec 5, 2009 at 5:02 AM, H.J. Lu <hjl.tools@gmail.com> wrote:
>> On Fri, Nov 20, 2009 at 1:23 AM, Alexandre Oliva <aoliva@redhat.com> wrote:
>>> On Nov 19, 2009, Richard Guenther <richard.guenther@gmail.com> wrote:
>>>
>>>> On Thu, Nov 19, 2009 at 5:27 AM, Alexandre Oliva <aoliva@redhat.com> wrote:
>>>>> On Nov 17, 2009, Richard Guenther <richard.guenther@gmail.com> wrote:
>>>>>
>>>>>> Well, just adjust degenerate_phi_result to do instead of calling
>>>>>> operand_equal_p
>>>>>
>>>>>>   else if (TREE_CODE (arg) != TREE_CODE (val)
>>>>>>              || (TREE_CODE (arg) == SSA_NAME
>>>>>>                  && arg != val)
>>>>>>              || !operand_equal_p (arg, val, 0))
>>>>>>     break;
>>>>>
>>>>>> that should fix it.
>>>>>
>>>>> Good idea.
>>>
>>>>> Here's what I'm testing now.
>>>
>>>> Ok if it passes testing.
>>>
>>> Thanks, but arg could be NULL, and then TREE_CODE (val) would crash.
>>>
>>> The fixed patch below corrects the two regressions I got testing the
>>> patch on x86_64-linux-gnu.  Save for new regressions, I'll check in the
>>> revised version as soon as my current round of testing completes.
>>>
>>>
>>
>> This patch caused:
>>
>> http://gcc.gnu.org/bugzilla/show_bug.cgi?id=42299
>>
>
> This patch also caused:
>
> http://gcc.gnu.org/bugzilla/show_bug.cgi?id=42395
>

This also caused:

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=42897

-- 
H.J.

Re: [vta, graphite?] propagate degenerate phi nodes into debug stmts

[Alexandre Oliva]

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

According to http://gcc.gnu.org/ml/gcc-patches/2009-11/msg00999.html
on Nov 19, 2009, Richard Guenther <richard.guenther@gmail.com> wrote:

> On Thu, Nov 19, 2009 at 4:05 AM, Alexandre Oliva <aoliva@redhat.com> wrote:
>> On Nov 17, 2009, Richard Guenther <richard.guenther@gmail.com> wrote:
>> 
>>>>> This looks odd.  SSA DEF operand iteration should walk the PHI defs
>>>>> as well, so the change should not be necessary.
>> 
>>>> I thought so, too, but by the time we get there, the operands of the PHI
>>>> stmt have already been disconnected.
>> 
>>> It shouldn't be.  Please try to figure out why instead.
>> 
>> Gotta use a different FOR_EACH macro to handle PHI nodes.
>> 
>> s/FOR_EACH_SSA_DEF_OPERAND/FOR_EACH_PHI_OR_STMT_DEF/ fixed it.
>> 
>> In order to make sure no other such mistakes had been made in GCC, I
>> added an assertion check in the iterator initializer and adjusted the
>> uses of GIMPLE_PHI nodes that triggered the assertion, but that would
>> have done nothing whatsoever in its absence.  I haven't looked into
>> whether doing nothing is correct.
>> 
>> Should I check this in?

> I think we should rather let num_ssa_operands and delink_stmt_imm_use
> ICE on PHIs, but I'd rather do this in stage1 - can you queue this
> patch until then?

You meant 4.6 stage1, but I missed it.  How's it for 4.7 stage1?
Regstrapped on x86_64-linux-gnu and i686-linux-gnu.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: check-no-nonphi-iter-on-phi.patch --]
[-- Type: text/x-diff, Size: 1794 bytes --]

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

	* tree-flow-inline.h (op_iter_init): Reject GIMPLE_PHI stmts.
	(num_ssa_operands): Skip GIMPLE_PHI.
	(delink_stmt_imm_use): Likewise.

Index: gcc/tree-flow-inline.h
===================================================================
--- gcc/tree-flow-inline.h.orig	2010-06-10 07:20:02.000000000 -0300
+++ gcc/tree-flow-inline.h	2010-06-10 15:17:51.000000000 -0300
@@ -716,9 +716,11 @@ clear_and_done_ssa_iter (ssa_op_iter *pt
 static inline void
 op_iter_init (ssa_op_iter *ptr, gimple stmt, int flags)
 {
-  /* We do not support iterating over virtual defs or uses without
+  /* PHI nodes require a different iterator initialization path.  We
+     do not support iterating over virtual defs or uses without
      iterating over defs or uses at the same time.  */
-  gcc_checking_assert ((!(flags & SSA_OP_VDEF) || (flags & SSA_OP_DEF))
+  gcc_checking_assert (gimple_code (stmt) != GIMPLE_PHI
+		       && (!(flags & SSA_OP_VDEF) || (flags & SSA_OP_DEF))
 		       && (!(flags & SSA_OP_VUSE) || (flags & SSA_OP_USE)));
   ptr->defs = (flags & (SSA_OP_DEF|SSA_OP_VDEF)) ? gimple_def_ops (stmt) : NULL;
   if (!(flags & SSA_OP_VDEF)
@@ -847,8 +849,9 @@ num_ssa_operands (gimple stmt, int flags
   tree t;
   int num = 0;
 
-  FOR_EACH_SSA_TREE_OPERAND (t, stmt, iter, flags)
-    num++;
+  if (gimple_code (stmt) != GIMPLE_PHI)
+    FOR_EACH_SSA_TREE_OPERAND (t, stmt, iter, flags)
+      num++;
   return num;
 }
 
@@ -860,7 +863,8 @@ delink_stmt_imm_use (gimple stmt)
    ssa_op_iter iter;
    use_operand_p use_p;
 
-   if (ssa_operands_active ())
+   if (ssa_operands_active ()
+       && gimple_code (stmt) != GIMPLE_PHI)
      FOR_EACH_SSA_USE_OPERAND (use_p, stmt, iter, SSA_OP_ALL_USES)
        delink_imm_use (use_p);
 }

[-- Attachment #3: Type: text/plain, Size: 258 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: [vta, graphite?] propagate degenerate phi nodes into debug stmts

[Richard Guenther]

On Fri, Jun 3, 2011 at 4:33 PM, Alexandre Oliva <aoliva@redhat.com> wrote:
> According to http://gcc.gnu.org/ml/gcc-patches/2009-11/msg00999.html
> on Nov 19, 2009, Richard Guenther <richard.guenther@gmail.com> wrote:
>
>> On Thu, Nov 19, 2009 at 4:05 AM, Alexandre Oliva <aoliva@redhat.com> wrote:
>>> On Nov 17, 2009, Richard Guenther <richard.guenther@gmail.com> wrote:
>>>
>>>>>> This looks odd.  SSA DEF operand iteration should walk the PHI defs
>>>>>> as well, so the change should not be necessary.
>>>
>>>>> I thought so, too, but by the time we get there, the operands of the PHI
>>>>> stmt have already been disconnected.
>>>
>>>> It shouldn't be.  Please try to figure out why instead.
>>>
>>> Gotta use a different FOR_EACH macro to handle PHI nodes.
>>>
>>> s/FOR_EACH_SSA_DEF_OPERAND/FOR_EACH_PHI_OR_STMT_DEF/ fixed it.
>>>
>>> In order to make sure no other such mistakes had been made in GCC, I
>>> added an assertion check in the iterator initializer and adjusted the
>>> uses of GIMPLE_PHI nodes that triggered the assertion, but that would
>>> have done nothing whatsoever in its absence.  I haven't looked into
>>> whether doing nothing is correct.
>>>
>>> Should I check this in?
>
>> I think we should rather let num_ssa_operands and delink_stmt_imm_use
>> ICE on PHIs, but I'd rather do this in stage1 - can you queue this
>> patch until then?
>
> You meant 4.6 stage1, but I missed it.  How's it for 4.7 stage1?
> Regstrapped on x86_64-linux-gnu and i686-linux-gnu.

Isn't exactly ICEing for num_ssa_operands/delink_stmt_imm_use.

So, the op_iter_init change is ok, the other two not - they should
either ICE or work for PHIs (by using FOR_EACH_PHI_OR_STMT_USE
in them).

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: [vta, graphite?] propagate degenerate phi nodes into debug stmts

[Alexandre Oliva]

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

On Jun  6, 2011, Richard Guenther <richard.guenther@gmail.com> wrote:

>> You meant 4.6 stage1, but I missed it.  How's it for 4.7 stage1?
>> Regstrapped on x86_64-linux-gnu and i686-linux-gnu.

> Isn't exactly ICEing for num_ssa_operands/delink_stmt_imm_use.

Uhh.  Looks like I didn't update the patch per your comments before
putting it aside for stage1, after all.  Sorry.

> So, the op_iter_init change is ok, the other two not - they should
> either ICE or work for PHIs (by using FOR_EACH_PHI_OR_STMT_USE
> in them).

The latter is legitimately called for GIMPLE_PHI, so I changed it to use
FOR_EACH_PHI_OR_STMT_USE.  The former was never called for GIMPLE_PHIs,
so I put in an assert there.

Regstrapped on x86_64-linux-gnu and i686-linux-gnu.  Ok?


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: check-no-nonphi-iter-on-phi.patch --]
[-- Type: text/x-diff, Size: 1951 bytes --]

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

	* tree-flow-inline.h (op_iter_init): Reject GIMPLE_PHI stmts.
	(num_ssa_operands): Likewise.
	(op_iter_init_phiuse): Forward-declare.
	(delink_stmt_imm_use): Iterate with FOR_EACH_PHI_OR_STMT_USE.

Index: gcc/tree-flow-inline.h
===================================================================
--- gcc/tree-flow-inline.h.orig	2011-06-03 11:56:24.619482744 -0300
+++ gcc/tree-flow-inline.h	2011-06-07 04:15:18.344729725 -0300
@@ -737,9 +737,11 @@ clear_and_done_ssa_iter (ssa_op_iter *pt
 static inline void
 op_iter_init (ssa_op_iter *ptr, gimple stmt, int flags)
 {
-  /* We do not support iterating over virtual defs or uses without
+  /* PHI nodes require a different iterator initialization path.  We
+     do not support iterating over virtual defs or uses without
      iterating over defs or uses at the same time.  */
-  gcc_checking_assert ((!(flags & SSA_OP_VDEF) || (flags & SSA_OP_DEF))
+  gcc_checking_assert (gimple_code (stmt) != GIMPLE_PHI
+		       && (!(flags & SSA_OP_VDEF) || (flags & SSA_OP_DEF))
 		       && (!(flags & SSA_OP_VUSE) || (flags & SSA_OP_USE)));
   ptr->defs = (flags & (SSA_OP_DEF|SSA_OP_VDEF)) ? gimple_def_ops (stmt) : NULL;
   if (!(flags & SSA_OP_VDEF)
@@ -868,11 +870,14 @@ num_ssa_operands (gimple stmt, int flags
   tree t;
   int num = 0;
 
+  gcc_checking_assert (gimple_code (stmt) != GIMPLE_PHI);
   FOR_EACH_SSA_TREE_OPERAND (t, stmt, iter, flags)
     num++;
   return num;
 }
 
+static inline use_operand_p
+op_iter_init_phiuse (ssa_op_iter *ptr, gimple phi, int flags);
 
 /* Delink all immediate_use information for STMT.  */
 static inline void
@@ -882,7 +887,7 @@ delink_stmt_imm_use (gimple stmt)
    use_operand_p use_p;
 
    if (ssa_operands_active ())
-     FOR_EACH_SSA_USE_OPERAND (use_p, stmt, iter, SSA_OP_ALL_USES)
+     FOR_EACH_PHI_OR_STMT_USE (use_p, stmt, iter, SSA_OP_ALL_USES)
        delink_imm_use (use_p);
 }
 

[-- Attachment #3: Type: text/plain, Size: 258 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: [vta, graphite?] propagate degenerate phi nodes into debug stmts

[Richard Guenther]

On Tue, Jun 7, 2011 at 12:38 PM, Alexandre Oliva <aoliva@redhat.com> wrote:
> On Jun  6, 2011, Richard Guenther <richard.guenther@gmail.com> wrote:
>
>>> You meant 4.6 stage1, but I missed it.  How's it for 4.7 stage1?
>>> Regstrapped on x86_64-linux-gnu and i686-linux-gnu.
>
>> Isn't exactly ICEing for num_ssa_operands/delink_stmt_imm_use.
>
> Uhh.  Looks like I didn't update the patch per your comments before
> putting it aside for stage1, after all.  Sorry.
>
>> So, the op_iter_init change is ok, the other two not - they should
>> either ICE or work for PHIs (by using FOR_EACH_PHI_OR_STMT_USE
>> in them).
>
> The latter is legitimately called for GIMPLE_PHI, so I changed it to use
> FOR_EACH_PHI_OR_STMT_USE.  The former was never called for GIMPLE_PHIs,
> so I put in an assert there.
>
> Regstrapped on x86_64-linux-gnu and i686-linux-gnu.  Ok?

Ok.

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