diff mbox series

[4/4] ipa-sra: Fix debug info for removed args passed to other functions (PR 93385, 95343)

Message ID 175b8838128e0adae59d85c30071089bd60d3420.1590667594.git.mjambor@suse.cz
State New
Headers show
Series Make IPA-SRA not depend on tree-dce and related fixes | expand

Commit Message

Martin Jambor May 28, 2020, 12:06 p.m. UTC 
This patch arguably finishes what I was asked to do in bugzilla PR
93385 and remaps *all* occurrences of SSA names discovered to be dead
in the process of parameter removal during clone materialization
either to error_mark_node or to DEBUG_EXPR_DECL that represents the
removed value - including those that appeared as arguments in call
statements.

However, those error_mark_nodes and DEBUG_EXPR_DECLs occurrences are
not removed straight away because arguments are removed only as a part
of call redirection - mostly following the plan for the callee - which
is not part of clone materialization.  Just for the record, this is
not something introduced by IPA-SRA, this has always been that way
since the beginning of IPA infrastructure and for good reasons.

As a consequence, error_mark_node and DEBUG_EXPR_DECL must be allowed
in places where they are normally not, which this patch does but only
during IPA passes. Afterwards, they are again banned.  I am confident
that if some bug allowed one of these to survive until late tree
passes, the compiler would ICE very quickly and so it is a safe thing
to do, even if not exactly nicely consistent.  Perhaps safer than the
temporary decl what the second patch introduced.

Temporarily replacing arguments with associated DEBUG_EXPR_DECL allows
us to produce debug info allowing the debugger to print values of
unused parameters which were removed not only in its function but also
in the caller.  At least sometimes :-) See the removed xfail in
testcase/gcc.dg/guality/ipa-sra-1.c.

I have attempted to achieve the same thing by associating the
DEBUG_EXPR_DECL with the artificial temporary and keep track of this
relationship in on-the side-summaries, constantly remapping both when
a clone of a clone gets its body and it is doable but quite ugly.
Injecting the DEBUG_EXPR_DECL directly into the IL works out of the
box.

Oh, and this patch also fixes PR debug/95343 - a case whee call
redirection can produce bad debug info.  A non-controversial fix is in
the first bugzilla comment but it needs all the other bits of this
patch to really allow debugger to print the value of the removed
parameter and not "value optimized out."  But perhaps that is what we
want to backport?

gcc/Changelog:

2020-05-26  Martin Jambor  <mjambor@suse.cz>

	PR ipa/93385
	PR debug/95343
	* ipa-param-manipulation.c (transitive_split_p): Handle
	error_mark_node.
	(ipa_param_adjustments::modify_call): Use index_map if available.
	Directly use removed argument if it is a DEBUG_EXP_DECL for
	corresponding debug info, assert all are removed.
	(ipa_param_body_adjustments::get_removed_call_arg_placeholder): Return
	corresponding DEBUG_EXP_DECL if there is one, otherwise return
	error_mark_node.
	* tree-ssa-operands.c: Include tree-pass.h.
	(operands_scanner::get_expr_operands): Allow DEBUG_EXPR_DECL and
	error_mark_node in call arguments during simple IPA passes.
	* tree-cfg.c (verify_gimple_call): Likewise.

gcc/testsuite/Changelog:

2020-05-26  Martin Jambor  <mjambor@suse.cz>

	PR ipa/93385
	PR debug/95343
	* gcc.dg/guality/pr95343.c: New test.
	* gcc.dg/guality/ipa-sra-1.c (bar): Remove xfail.
---
 gcc/ipa-param-manipulation.c             | 31 ++++++++++++----
 gcc/testsuite/gcc.dg/guality/ipa-sra-1.c |  2 +-
 gcc/testsuite/gcc.dg/guality/pr95343.c   | 45 ++++++++++++++++++++++++
 gcc/tree-cfg.c                           | 14 ++++++--
 gcc/tree-ssa-operands.c                  | 16 +++++++--
 5 files changed, 95 insertions(+), 13 deletions(-)
 create mode 100644 gcc/testsuite/gcc.dg/guality/pr95343.c

Comments

Martin Jambor June 8, 2020, 11:40 a.m. UTC | #1 
Hi,

On Thu, May 28 2020, Martin Jambor wrote:
> This patch arguably finishes what I was asked to do in bugzilla PR
> 93385 and remaps *all* occurrences of SSA names discovered to be dead
> in the process of parameter removal during clone materialization
> either to error_mark_node or to DEBUG_EXPR_DECL that represents the
> removed value - including those that appeared as arguments in call
> statements.
>
> However, those error_mark_nodes and DEBUG_EXPR_DECLs occurrences are
> not removed straight away because arguments are removed only as a part
> of call redirection - mostly following the plan for the callee - which
> is not part of clone materialization.  Just for the record, this is
> not something introduced by IPA-SRA, this has always been that way
> since the beginning of IPA infrastructure and for good reasons.
>
> As a consequence, error_mark_node and DEBUG_EXPR_DECL must be allowed
> in places where they are normally not, which this patch does but only
> during IPA passes. Afterwards, they are again banned.  I am confident
> that if some bug allowed one of these to survive until late tree
> passes, the compiler would ICE very quickly and so it is a safe thing
> to do, even if not exactly nicely consistent.  Perhaps safer than the
> temporary decl what the second patch introduced.
>
> Temporarily replacing arguments with associated DEBUG_EXPR_DECL allows
> us to produce debug info allowing the debugger to print values of
> unused parameters which were removed not only in its function but also
> in the caller.  At least sometimes :-) See the removed xfail in
> testcase/gcc.dg/guality/ipa-sra-1.c.
>
> I have attempted to achieve the same thing by associating the
> DEBUG_EXPR_DECL with the artificial temporary and keep track of this
> relationship in on-the side-summaries, constantly remapping both when
> a clone of a clone gets its body and it is doable but quite ugly.
> Injecting the DEBUG_EXPR_DECL directly into the IL works out of the
> box.
>
> Oh, and this patch also fixes PR debug/95343 - a case whee call
> redirection can produce bad debug info.  A non-controversial fix is in
> the first bugzilla comment but it needs all the other bits of this
> patch to really allow debugger to print the value of the removed
> parameter and not "value optimized out."  But perhaps that is what we
> want to backport?
>

this patch missed one small test which lead to an ICE during aarch64-linux
bootstrap.  Otherwise everything stated above holds.  Fixed below.

Thanks,

Martin


gcc/Changelog:

2020-06-05  Martin Jambor  <mjambor@suse.cz>

	PR ipa/93385
	PR debug/95343
	* ipa-param-manipulation.c (transitive_split_p): Handle
	error_mark_node.
	(ipa_param_adjustments::modify_call): Use index_map if available.
	Directly use removed argument if it is a DEBUG_EXP_DECL for
	corresponding debug info, assert all are removed.
	(ipa_param_body_adjustments::get_removed_call_arg_placeholder): Return
	corresponding DEBUG_EXP_DECL if there is one, otherwise return
	error_mark_node.
	* tree-ssa-operands.c: Include tree-pass.h.
	(operands_scanner::get_expr_operands): Allow DEBUG_EXPR_DECL and
	error_mark_node in call arguments during simple IPA passes.
	* tree-cfg.c (verify_gimple_call): Likewise.

gcc/testsuite/Changelog:

2020-05-26  Martin Jambor  <mjambor@suse.cz>

	PR ipa/93385
	PR debug/95343
	* gcc.dg/guality/pr95343.c: New test.
	* gcc.dg/guality/ipa-sra-1.c (bar): Remove xfail.
---
 gcc/ipa-param-manipulation.c             | 31 ++++++++++++----
 gcc/testsuite/gcc.dg/guality/ipa-sra-1.c |  2 +-
 gcc/testsuite/gcc.dg/guality/pr95343.c   | 45 ++++++++++++++++++++++++
 gcc/tree-cfg.c                           | 14 ++++++--
 gcc/tree-ssa-operands.c                  | 15 ++++++--
 5 files changed, 94 insertions(+), 13 deletions(-)
 create mode 100644 gcc/testsuite/gcc.dg/guality/pr95343.c

diff --git a/gcc/ipa-param-manipulation.c b/gcc/ipa-param-manipulation.c
index 71ae4fdc16d..c805350e107 100644
--- a/gcc/ipa-param-manipulation.c
+++ b/gcc/ipa-param-manipulation.c
@@ -469,6 +469,8 @@ transitive_split_p (vec<ipa_param_performed_split, va_gc> *performed_splits,
 		    tree expr, unsigned *sm_idx_p, unsigned *unit_offset_p)
 {
   tree base;
+  if (expr == error_mark_node)
+    return false;
   if (!isra_get_ref_base_and_offset (expr, &base, unit_offset_p))
     return false;
 
@@ -620,6 +622,8 @@ ipa_param_adjustments::modify_call (gcall *stmt,
 	    index = index_map[apm->base_index];
 
 	  tree arg = gimple_call_arg (stmt, index);
+	  gcc_assert (arg != error_mark_node
+		      && TREE_CODE (arg) != DEBUG_EXPR_DECL);
 
 	  vargs.quick_push (arg);
 	  kept[index] = true;
@@ -792,7 +796,14 @@ ipa_param_adjustments::modify_call (gcall *stmt,
 	  if (!is_gimple_reg (old_parm) || kept[i])
 	    continue;
 	  tree origin = DECL_ORIGIN (old_parm);
-	  tree arg = gimple_call_arg (stmt, i);
+	  int index;
+	  if (transitive_remapping)
+	    index = index_map[i];
+	  else
+	    index = i;
+	  tree arg = gimple_call_arg (stmt, index);
+	  if (arg == error_mark_node)
+	    continue;
 
 	  if (!useless_type_conversion_p (TREE_TYPE (origin), TREE_TYPE (arg)))
 	    {
@@ -1781,16 +1792,22 @@ remap_split_decl_to_dummy (tree *tp, int *walk_subtrees, void *data)
 
 /* Given ARG which is a SSA_NAME call argument which we are however removing
    from the current function and which will be thus removed from the call
-   statement by ipa_param_adjustments::modify_call, return something that can
-   be used as a placeholder and which the operand scanner will accept until
-   then.  */
+   statement by ipa_param_adjustments::modify_call.  Return either a
+   DEBUG_EXPR_DECL that describes the removed value or error_mark_node if there
+   is none.  */
 
 tree
 ipa_param_body_adjustments::get_removed_call_arg_placeholder (tree arg)
 {
-  tree t = create_tmp_var_raw (TREE_TYPE (arg));
-  insert_decl_map (m_id, t, t);
-  return t;
+  tree *d = m_dead_ssa_debug_equiv.get (arg);
+  if (d && *d)
+    {
+      tree t = *d;
+      insert_decl_map (m_id, t, t);
+      return t;
+    }
+  else
+    return error_mark_node;
 }
 
 /* If the call statement pointed at by STMT_P contains any expressions that
diff --git a/gcc/testsuite/gcc.dg/guality/ipa-sra-1.c b/gcc/testsuite/gcc.dg/guality/ipa-sra-1.c
index 5434b3d7665..5eaf616dd43 100644
--- a/gcc/testsuite/gcc.dg/guality/ipa-sra-1.c
+++ b/gcc/testsuite/gcc.dg/guality/ipa-sra-1.c
@@ -12,7 +12,7 @@ static int __attribute__((noinline))
 bar (int i, int k)
 {
   asm ("" : "+r" (i));
-  use (i);		/* { dg-final { gdb-test . "k" "3" { xfail *-*-* } } } */
+  use (i);		/* { dg-final { gdb-test . "k" "3" } } */
   return 6;
 }
 
diff --git a/gcc/testsuite/gcc.dg/guality/pr95343.c b/gcc/testsuite/gcc.dg/guality/pr95343.c
new file mode 100644
index 00000000000..7670eb87932
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/guality/pr95343.c
@@ -0,0 +1,45 @@
+/* { dg-do run } */
+/* { dg-options "-g -fno-ipa-icf" } */
+
+volatile int v;
+
+int __attribute__((noipa))
+get_val0 (void)  {return 0;}
+int __attribute__((noipa))
+get_val2 (void)  {return 2;}
+
+struct S
+{
+  int a, b, c;
+};
+
+static int __attribute__((noinline))
+bar (struct S s, int x, int y, int z, int i)
+{
+  int r;
+  v = s.a + s.b;		/* { dg-final { gdb-test . "i" "2" } } */
+  return r;
+}
+
+static int __attribute__((noinline))
+foo (struct S s, int i)
+{
+  int r;
+  r = bar (s, 3, 4, 5, i);
+  return r;
+}
+
+
+int
+main (void)
+{
+  struct S s;
+  int i;
+  i = get_val2 ();
+  s.a = get_val0 ();
+  s.b = get_val0 ();
+  s.c = get_val0 ();
+  int r = foo (s, i);
+  v = r + i;
+  return 0;
+}
diff --git a/gcc/tree-cfg.c b/gcc/tree-cfg.c
index d06a479e570..d96fee5bb7f 100644
--- a/gcc/tree-cfg.c
+++ b/gcc/tree-cfg.c
@@ -3433,10 +3433,18 @@ verify_gimple_call (gcall *stmt)
   for (i = 0; i < gimple_call_num_args (stmt); ++i)
     {
       tree arg = gimple_call_arg (stmt, i);
-      if ((is_gimple_reg_type (TREE_TYPE (arg))
+      if (((is_gimple_reg_type (TREE_TYPE (arg))
 	   && !is_gimple_val (arg))
-	  || (!is_gimple_reg_type (TREE_TYPE (arg))
-	      && !is_gimple_lvalue (arg)))
+	   || (!is_gimple_reg_type (TREE_TYPE (arg))
+	       && !is_gimple_lvalue (arg)))
+	  /* DEBUG_EXPR_DECL or error_mark_mode can occur in call statements
+	     for a brief moment when a function clone has been materialized but
+	     call statements have not been updated yet and unused arguments not
+	     removed.  */
+	  && ((TREE_CODE (arg) != DEBUG_EXPR_DECL
+	       && arg != error_mark_node)
+	      || (current_pass->type != SIMPLE_IPA_PASS
+		  && current_pass->type != IPA_PASS)))
 	{
 	  error ("invalid argument to gimple call");
 	  debug_generic_expr (arg);
diff --git a/gcc/tree-ssa-operands.c b/gcc/tree-ssa-operands.c
index f4716d0e36f..bb5cce97b1d 100644
--- a/gcc/tree-ssa-operands.c
+++ b/gcc/tree-ssa-operands.c
@@ -30,7 +30,7 @@ along with GCC; see the file COPYING3.  If not see
 #include "stmt.h"
 #include "print-tree.h"
 #include "dumpfile.h"
-
+#include "tree-pass.h"
 
 /* This file contains the code required to manage the operands cache of the
    SSA optimizer.  For every stmt, we maintain an operand cache in the stmt
@@ -813,7 +813,18 @@ operands_scanner::get_expr_operands (tree *expr_p, int flags)
       return;
 
     case DEBUG_EXPR_DECL:
-      gcc_assert (gimple_debug_bind_p (stmt));
+      /* DEBUG_EXPR_DECL can occur in call statements for a brief moment when a
+	 function clone has been materialized but call statements have not been
+	 updated yet and unused arguments not removed.  */
+      gcc_assert (gimple_debug_bind_p (stmt)
+		  || current_pass->type == SIMPLE_IPA_PASS
+		  || current_pass->type == IPA_PASS);
+      return;
+    case ERROR_MARK:
+      /* When not producing debug info, error_mark_node is used as a
+	 placeholder for removed arguments.  */
+      gcc_assert (current_pass->type == SIMPLE_IPA_PASS
+		  || current_pass->type == IPA_PASS);
       return;
 
     case MEM_REF:
diff mbox series

Patch

diff --git a/gcc/ipa-param-manipulation.c b/gcc/ipa-param-manipulation.c
index 0a265e26c4f..b43c1323ef1 100644
--- a/gcc/ipa-param-manipulation.c
+++ b/gcc/ipa-param-manipulation.c
@@ -466,6 +466,8 @@  transitive_split_p (vec<ipa_param_performed_split, va_gc> *performed_splits,
 		    tree expr, unsigned *sm_idx_p, unsigned *unit_offset_p)
 {
   tree base;
+  if (expr == error_mark_node)
+    return false;
   if (!isra_get_ref_base_and_offset (expr, &base, unit_offset_p))
     return false;
 
@@ -617,6 +619,8 @@  ipa_param_adjustments::modify_call (gcall *stmt,
 	    index = index_map[apm->base_index];
 
 	  tree arg = gimple_call_arg (stmt, index);
+	  gcc_assert (arg != error_mark_node
+		      && TREE_CODE (arg) != DEBUG_EXPR_DECL);
 
 	  vargs.quick_push (arg);
 	  kept[index] = true;
@@ -789,7 +793,14 @@  ipa_param_adjustments::modify_call (gcall *stmt,
 	  if (!is_gimple_reg (old_parm) || kept[i])
 	    continue;
 	  tree origin = DECL_ORIGIN (old_parm);
-	  tree arg = gimple_call_arg (stmt, i);
+	  int index;
+	  if (transitive_remapping)
+	    index = index_map[i];
+	  else
+	    index = i;
+	  tree arg = gimple_call_arg (stmt, index);
+	  if (arg == error_mark_node)
+	    continue;
 
 	  if (!useless_type_conversion_p (TREE_TYPE (origin), TREE_TYPE (arg)))
 	    {
@@ -1778,16 +1789,22 @@  remap_split_decl_to_dummy (tree *tp, int *walk_subtrees, void *data)
 
 /* Given ARG which is a SSA_NAME call argument which we are however removing
    from the current function and which will be thus removed from the call
-   statement by ipa_param_adjustments::modify_call, return something that can
-   be used as a placeholder and which the operand scanner will accept until
-   then.  */
+   statement by ipa_param_adjustments::modify_call.  Return either a
+   DEBUG_EXPR_DECL that describes the removed value or error_mark_node if there
+   is none.  */
 
 tree
 ipa_param_body_adjustments::get_removed_call_arg_placeholder (tree arg)
 {
-  tree t = create_tmp_var (TREE_TYPE (arg));
-  insert_decl_map (m_id, t, t);
-  return t;
+  tree *d = m_dead_ssa_debug_equiv.get (arg);
+  if (d)
+    {
+      tree t = *d;
+      insert_decl_map (m_id, t, t);
+      return t;
+    }
+  else
+    return error_mark_node;
 }
 
 /* If the call statement pointed at by STMT_P contains any expressions that
diff --git a/gcc/testsuite/gcc.dg/guality/ipa-sra-1.c b/gcc/testsuite/gcc.dg/guality/ipa-sra-1.c
index 5434b3d7665..5eaf616dd43 100644
--- a/gcc/testsuite/gcc.dg/guality/ipa-sra-1.c
+++ b/gcc/testsuite/gcc.dg/guality/ipa-sra-1.c
@@ -12,7 +12,7 @@  static int __attribute__((noinline))
 bar (int i, int k)
 {
   asm ("" : "+r" (i));
-  use (i);		/* { dg-final { gdb-test . "k" "3" { xfail *-*-* } } } */
+  use (i);		/* { dg-final { gdb-test . "k" "3" } } */
   return 6;
 }
 
diff --git a/gcc/testsuite/gcc.dg/guality/pr95343.c b/gcc/testsuite/gcc.dg/guality/pr95343.c
new file mode 100644
index 00000000000..7670eb87932
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/guality/pr95343.c
@@ -0,0 +1,45 @@ 
+/* { dg-do run } */
+/* { dg-options "-g -fno-ipa-icf" } */
+
+volatile int v;
+
+int __attribute__((noipa))
+get_val0 (void)  {return 0;}
+int __attribute__((noipa))
+get_val2 (void)  {return 2;}
+
+struct S
+{
+  int a, b, c;
+};
+
+static int __attribute__((noinline))
+bar (struct S s, int x, int y, int z, int i)
+{
+  int r;
+  v = s.a + s.b;		/* { dg-final { gdb-test . "i" "2" } } */
+  return r;
+}
+
+static int __attribute__((noinline))
+foo (struct S s, int i)
+{
+  int r;
+  r = bar (s, 3, 4, 5, i);
+  return r;
+}
+
+
+int
+main (void)
+{
+  struct S s;
+  int i;
+  i = get_val2 ();
+  s.a = get_val0 ();
+  s.b = get_val0 ();
+  s.c = get_val0 ();
+  int r = foo (s, i);
+  v = r + i;
+  return 0;
+}
diff --git a/gcc/tree-cfg.c b/gcc/tree-cfg.c
index d06a479e570..d96fee5bb7f 100644
--- a/gcc/tree-cfg.c
+++ b/gcc/tree-cfg.c
@@ -3433,10 +3433,18 @@  verify_gimple_call (gcall *stmt)
   for (i = 0; i < gimple_call_num_args (stmt); ++i)
     {
       tree arg = gimple_call_arg (stmt, i);
-      if ((is_gimple_reg_type (TREE_TYPE (arg))
+      if (((is_gimple_reg_type (TREE_TYPE (arg))
 	   && !is_gimple_val (arg))
-	  || (!is_gimple_reg_type (TREE_TYPE (arg))
-	      && !is_gimple_lvalue (arg)))
+	   || (!is_gimple_reg_type (TREE_TYPE (arg))
+	       && !is_gimple_lvalue (arg)))
+	  /* DEBUG_EXPR_DECL or error_mark_mode can occur in call statements
+	     for a brief moment when a function clone has been materialized but
+	     call statements have not been updated yet and unused arguments not
+	     removed.  */
+	  && ((TREE_CODE (arg) != DEBUG_EXPR_DECL
+	       && arg != error_mark_node)
+	      || (current_pass->type != SIMPLE_IPA_PASS
+		  && current_pass->type != IPA_PASS)))
 	{
 	  error ("invalid argument to gimple call");
 	  debug_generic_expr (arg);
diff --git a/gcc/tree-ssa-operands.c b/gcc/tree-ssa-operands.c
index f4716d0e36f..4d235af898e 100644
--- a/gcc/tree-ssa-operands.c
+++ b/gcc/tree-ssa-operands.c
@@ -30,7 +30,7 @@  along with GCC; see the file COPYING3.  If not see
 #include "stmt.h"
 #include "print-tree.h"
 #include "dumpfile.h"
-
+#include "tree-pass.h"
 
 /* This file contains the code required to manage the operands cache of the
    SSA optimizer.  For every stmt, we maintain an operand cache in the stmt
@@ -813,9 +813,21 @@  operands_scanner::get_expr_operands (tree *expr_p, int flags)
       return;
 
     case DEBUG_EXPR_DECL:
-      gcc_assert (gimple_debug_bind_p (stmt));
+      /* DEBUG_EXPR_DECL can occur in call statements for a brief moment when a
+	 function clone has been materialized but call statements have not been
+	 updated yet and unused arguments not removed.  */
+      gcc_assert (gimple_debug_bind_p (stmt)
+		  || current_pass->type == SIMPLE_IPA_PASS
+		  || current_pass->type == IPA_PASS);
+      return;
+    case ERROR_MARK:
+      /* When not producing debug info, error_mark_node is used as a
+	 placeholder for removed arguments.  */
+      gcc_assert (current_pass->type == SIMPLE_IPA_PASS
+		  || current_pass->type == IPA_PASS);
       return;
 
+      
     case MEM_REF:
       get_mem_ref_operands (expr, flags);
       return;