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

Martin Jambor mjambor@suse.cz
Mon Jun 8 11:40:12 GMT 2020


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






More information about the Gcc-patches mailing list