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 <mjam...@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 <mjam...@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
