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

[Alexandre Oliva-2]

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?


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

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


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

[Richard Guenther-2]

On Sun, Nov 8, 2009 at 8:46 AM, Alexandre Oliva <aoliva@...> 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
>
>

[Alexandre Oliva-2]

On Nov  8, 2009, Richard Guenther <richard.guenther@...> 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.


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

        * 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;
  }
 


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

On Nov  8, 2009, Richard Guenther <richard.guenther@...> 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.


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

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


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

[Richard Guenther-2]

On Mon, Nov 16, 2009 at 9:41 PM, Alexandre Oliva <aoliva@...> wrote:

> On Nov  8, 2009, Richard Guenther <richard.guenther@...> 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
>
>

[Alexandre Oliva-2]

On Nov 17, 2009, Richard Guenther <richard.guenther@...> 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?


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

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


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

On Nov 17, 2009, Richard Guenther <richard.guenther@...> 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.


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

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


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

[Richard Guenther-2]

On Thu, Nov 19, 2009 at 4:05 AM, Alexandre Oliva <aoliva@...> wrote:

> On Nov 17, 2009, Richard Guenther <richard.guenther@...> 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
>
>

[Richard Guenther-2]

On Thu, Nov 19, 2009 at 5:27 AM, Alexandre Oliva <aoliva@...> wrote:

> On Nov 17, 2009, Richard Guenther <richard.guenther@...> 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
>
>

[Alexandre Oliva-2]

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

> On Thu, Nov 19, 2009 at 5:27 AM, Alexandre Oliva <aoliva@...> wrote:
>> On Nov 17, 2009, Richard Guenther <richard.guenther@...> 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.


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

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


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

On Nov 19, 2009, Richard Guenther <richard.guenther@...> 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

[Richard Guenther-2]

On Fri, Nov 20, 2009 at 10:25 AM, Alexandre Oliva <aoliva@...> wrote:

> On Nov 19, 2009, Richard Guenther <richard.guenther@...> 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.

[Alexandre Oliva-2]

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

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

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


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

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


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