* [PATCH 0/4] Make IPA-SRA not depend on tree-dce and related fixes
@ 2020-05-28 12:06 Martin Jambor
2020-05-28 12:06 ` [PATCH 1/4] ipa-sra: Do not remove statements necessary because of non-call EH (PR 95113) Martin Jambor
` (3 more replies)
0 siblings, 4 replies; 9+ messages in thread
From: Martin Jambor @ 2020-05-28 12:06 UTC (permalink / raw)
To: GCC Patches; +Cc: Jan Hubicka, Richard Biener
Hi,
this patch series addresses PR 93385 which exposed that the new
IPA-SRA depends on tree-dce and can leave misbehaving instructions
behind if the user switched it off. It is a series because I also
tried to produce the best debug info possible in such cases while
avoiding unnecessary copying of instructions during IPA-SRA clone
materialization and it seemed best to tackle different problems
independently. We also might not want to backport all of the patches
to GCC 10.
The first patch actually fixes similar but different PR 95113 where
IPA-SRA should switch itself off because of non-call exceptions.
The second patch fixes non-debug instructions in PR 93385, debug
instructions are simply reset.
The third patch fixes up debug statements except for those when a
removed value is passed to another function.
The fourth patch attempts to produce useful debug info even in this
situation and fixes PR debug/95343. However it requires relaxing some
gimple IL rules during IPA passes (and only during IPA passes).
After/if there is a consensus on the above, I would like to proceed
with a little bit of clean-up in the messy parts of tree-inline.c
which are directly involved in this - particularly the debug info
generation.
Finally, in Bugzilla Jakub asked me to make IPA-SRA consider any
arithmetic operation on otherwise unnecessary argument a use - if the
user used the option -fno-tree-dce. I have not done that yet, mostly
because I realized we already differentiate between -fno-dce and
-fno-tree-dce and so none of those options is really for users and GCC
hackers might want to disable a specific pass and not a little bit of
another when they use it. Also, making the testcase fail without
-fno-tree-dce requires using the following exact combination of
options:
-fno-dce -fdisable-tree-cddce1 -fdisable-tree-cdce
-fdisable-tree-cddce3 -fdisable-tree-dce2 -fdisable-tree-dce3
-fdisable-tree-dce4 -fdisable-tree-dce7
And that does not seem very maintainable in the testcase.
Nevertheless, if the consensus is that -fno-tree-dce should also limit
IPA-SRA in this regard, the patch is trivial (Jakup wrote it in
comment 23.
All patches were individually bootstrapped and tested on x86_64-linux
and the whole bundle also passes LTO bootstrap and profiled-LTO
bootstrap on the same platform. Bootstrap on aarch64 and i686 is
underway.
I am looking forward for your comments, questions and suggestions,
Martin
Martin Jambor (4):
ipa-sra: Do not remove statements necessary because of non-call EH (PR
95113)
ipa-sra: Introduce a mini-DCE to tree-inline.c (PR 93385)
ipa-sra: Improve debug info for removed parameters (PR 93385)
ipa-sra: Fix debug info for removed args passed to other functions (PR
93385, 95343)
gcc/ipa-param-manipulation.c | 406 +++++++++++++++++++----
gcc/ipa-param-manipulation.h | 18 +
gcc/ipa-sra.c | 28 +-
gcc/testsuite/gcc.dg/guality/ipa-sra-1.c | 45 +++
gcc/testsuite/gcc.dg/guality/pr95343.c | 45 +++
gcc/testsuite/gcc.dg/ipa/ipa-sra-23.c | 24 ++
gcc/testsuite/gcc.dg/ipa/pr93385.c | 27 ++
gcc/testsuite/gcc.dg/ipa/pr95113.c | 33 ++
gcc/tree-cfg.c | 14 +-
gcc/tree-eh.c | 10 +
gcc/tree-eh.h | 1 +
gcc/tree-inline.c | 51 ++-
gcc/tree-ssa-dce.c | 4 +-
gcc/tree-ssa-operands.c | 16 +-
14 files changed, 635 insertions(+), 87 deletions(-)
create mode 100644 gcc/testsuite/gcc.dg/guality/ipa-sra-1.c
create mode 100644 gcc/testsuite/gcc.dg/guality/pr95343.c
create mode 100644 gcc/testsuite/gcc.dg/ipa/ipa-sra-23.c
create mode 100644 gcc/testsuite/gcc.dg/ipa/pr93385.c
create mode 100644 gcc/testsuite/gcc.dg/ipa/pr95113.c
--
2.26.2
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH 1/4] ipa-sra: Do not remove statements necessary because of non-call EH (PR 95113)
2020-05-28 12:06 [PATCH 0/4] Make IPA-SRA not depend on tree-dce and related fixes Martin Jambor
@ 2020-05-28 12:06 ` Martin Jambor
2020-06-02 11:42 ` Richard Biener
2020-05-28 12:06 ` [PATCH 2/4] ipa-sra: Introduce a mini-DCE to tree-inline.c (PR 93385) Martin Jambor
` (2 subsequent siblings)
3 siblings, 1 reply; 9+ messages in thread
From: Martin Jambor @ 2020-05-28 12:06 UTC (permalink / raw)
To: GCC Patches; +Cc: Jan Hubicka, Richard Biener
PR 95113 revealed that when reasoning about which parameters are dead,
IPA-SRA does not perform the same check related to non-call exceptions
as tree DCE. It most certainly should and so this patch moves the
condition used in tree-ssa-dce.c into a separate predicate (in
tree-eh.c) and uses it from both places.
gcc/ChangeLog:
2020-05-27 Martin Jambor <mjambor@suse.cz>
PR ipa/95113
* gcc/tree-ssa-dce.c (mark_stmt_if_obviously_necessary): Move non-call
exceptions check to...
* gcc/tree-eh.c (stmt_unremovable_because_of_non_call_eh_p): ...this
new function.
* gcc/tree-eh.h (stmt_unremovable_because_of_non_call_eh_p): Declare it.
* gcc/ipa-sra.c (isra_track_scalar_value_uses): Use it. New parameter
fun.
gcc/testsuite/ChangeLog:
2020-05-27 Martin Jambor <mjambor@suse.cz>
PR ipa/95113
* gcc.dg/ipa/pr95113.c: New test.
---
gcc/ipa-sra.c | 28 +++++++++++++------------
gcc/testsuite/gcc.dg/ipa/pr95113.c | 33 ++++++++++++++++++++++++++++++
gcc/tree-eh.c | 10 +++++++++
gcc/tree-eh.h | 1 +
gcc/tree-ssa-dce.c | 4 +---
5 files changed, 60 insertions(+), 16 deletions(-)
create mode 100644 gcc/testsuite/gcc.dg/ipa/pr95113.c
diff --git a/gcc/ipa-sra.c b/gcc/ipa-sra.c
index 7c922e40a4e..c81e8869e7a 100644
--- a/gcc/ipa-sra.c
+++ b/gcc/ipa-sra.c
@@ -795,17 +795,17 @@ get_single_param_flow_source (const isra_param_flow *param_flow)
}
/* Inspect all uses of NAME and simple arithmetic calculations involving NAME
- in NODE and return a negative number if any of them is used for something
- else than either an actual call argument, simple arithmetic operation or
- debug statement. If there are no such uses, return the number of actual
- arguments that this parameter eventually feeds to (or zero if there is none).
- For any such parameter, mark PARM_NUM as one of its sources. ANALYZED is a
- bitmap that tracks which SSA names we have already started
- investigating. */
+ in FUN represented with NODE and return a negative number if any of them is
+ used for something else than either an actual call argument, simple
+ arithmetic operation or debug statement. If there are no such uses, return
+ the number of actual arguments that this parameter eventually feeds to (or
+ zero if there is none). For any such parameter, mark PARM_NUM as one of its
+ sources. ANALYZED is a bitmap that tracks which SSA names we have already
+ started investigating. */
static int
-isra_track_scalar_value_uses (cgraph_node *node, tree name, int parm_num,
- bitmap analyzed)
+isra_track_scalar_value_uses (function *fun, cgraph_node *node, tree name,
+ int parm_num, bitmap analyzed)
{
int res = 0;
imm_use_iterator imm_iter;
@@ -859,8 +859,9 @@ isra_track_scalar_value_uses (cgraph_node *node, tree name, int parm_num,
}
res += all_uses;
}
- else if ((is_gimple_assign (stmt) && !gimple_has_volatile_ops (stmt))
- || gimple_code (stmt) == GIMPLE_PHI)
+ else if (!stmt_unremovable_because_of_non_call_eh_p (fun, stmt)
+ && ((is_gimple_assign (stmt) && !gimple_has_volatile_ops (stmt))
+ || gimple_code (stmt) == GIMPLE_PHI))
{
tree lhs;
if (gimple_code (stmt) == GIMPLE_PHI)
@@ -876,7 +877,7 @@ isra_track_scalar_value_uses (cgraph_node *node, tree name, int parm_num,
gcc_assert (!gimple_vdef (stmt));
if (bitmap_set_bit (analyzed, SSA_NAME_VERSION (lhs)))
{
- int tmp = isra_track_scalar_value_uses (node, lhs, parm_num,
+ int tmp = isra_track_scalar_value_uses (fun, node, lhs, parm_num,
analyzed);
if (tmp < 0)
{
@@ -927,7 +928,8 @@ isra_track_scalar_param_local_uses (function *fun, cgraph_node *node, tree parm,
return true;
bitmap analyzed = BITMAP_ALLOC (NULL);
- int call_uses = isra_track_scalar_value_uses (node, name, parm_num, analyzed);
+ int call_uses = isra_track_scalar_value_uses (fun, node, name, parm_num,
+ analyzed);
BITMAP_FREE (analyzed);
if (call_uses < 0)
return true;
diff --git a/gcc/testsuite/gcc.dg/ipa/pr95113.c b/gcc/testsuite/gcc.dg/ipa/pr95113.c
new file mode 100644
index 00000000000..a8f8c901ebe
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/ipa/pr95113.c
@@ -0,0 +1,33 @@
+/* { dg-do run } */
+/* { dg-options "-O2 -fexceptions -fnon-call-exceptions" } */
+/* { dg-require-effective-target exceptions } */
+
+int a, b;
+
+static inline long int
+foo (long int x, int y)
+{
+ if (y == 0)
+ return 0;
+
+ if (x == -1 && y == -1)
+ return 0;
+
+ return x / y;
+}
+
+static inline int
+bar (int *p)
+{
+ int c = foo (a, 1) + *p;
+ return b;
+}
+
+int
+main ()
+{
+ int d = 0;
+ b = foo (1, 1);
+ bar (&d);
+ return 0;
+}
diff --git a/gcc/tree-eh.c b/gcc/tree-eh.c
index 10ef2e3157c..4246dca8806 100644
--- a/gcc/tree-eh.c
+++ b/gcc/tree-eh.c
@@ -2936,6 +2936,16 @@ stmt_could_throw_p (function *fun, gimple *stmt)
}
}
+/* Return true if STMT in function FUN must be assumed necessary because of
+ non-call exceptions. */
+
+bool
+stmt_unremovable_because_of_non_call_eh_p (function *fun, gimple *stmt)
+{
+ return (fun->can_throw_non_call_exceptions
+ && !fun->can_delete_dead_exceptions
+ && stmt_could_throw_p (fun, stmt));
+}
/* Return true if expression T could throw an exception. */
diff --git a/gcc/tree-eh.h b/gcc/tree-eh.h
index cb1019e0d87..ba911cadbe7 100644
--- a/gcc/tree-eh.h
+++ b/gcc/tree-eh.h
@@ -39,6 +39,7 @@ extern bool operation_could_trap_p (enum tree_code, bool, bool, tree);
extern bool tree_could_trap_p (tree);
extern tree rewrite_to_non_trapping_overflow (tree);
extern bool stmt_could_throw_p (function *, gimple *);
+extern bool stmt_unremovable_because_of_non_call_eh_p (function *, gimple *);
extern bool tree_could_throw_p (tree);
extern bool stmt_can_throw_external (function *, gimple *);
extern bool stmt_can_throw_internal (function *, gimple *);
diff --git a/gcc/tree-ssa-dce.c b/gcc/tree-ssa-dce.c
index 757cfad5b5e..fae5ae72340 100644
--- a/gcc/tree-ssa-dce.c
+++ b/gcc/tree-ssa-dce.c
@@ -201,9 +201,7 @@ mark_stmt_if_obviously_necessary (gimple *stmt, bool aggressive)
{
/* With non-call exceptions, we have to assume that all statements could
throw. If a statement could throw, it can be deemed necessary. */
- if (cfun->can_throw_non_call_exceptions
- && !cfun->can_delete_dead_exceptions
- && stmt_could_throw_p (cfun, stmt))
+ if (stmt_unremovable_because_of_non_call_eh_p (cfun, stmt))
{
mark_stmt_necessary (stmt, true);
return;
--
2.26.2
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH 2/4] ipa-sra: Introduce a mini-DCE to tree-inline.c (PR 93385)
2020-05-28 12:06 [PATCH 0/4] Make IPA-SRA not depend on tree-dce and related fixes Martin Jambor
2020-05-28 12:06 ` [PATCH 1/4] ipa-sra: Do not remove statements necessary because of non-call EH (PR 95113) Martin Jambor
@ 2020-05-28 12:06 ` Martin Jambor
2020-06-02 11:56 ` Richard Biener
2020-05-28 12:06 ` [PATCH 3/4] ipa-sra: Improve debug info for removed parameters " Martin Jambor
2020-05-28 12:06 ` [PATCH 4/4] ipa-sra: Fix debug info for removed args passed to other functions (PR 93385, 95343) Martin Jambor
3 siblings, 1 reply; 9+ messages in thread
From: Martin Jambor @ 2020-05-28 12:06 UTC (permalink / raw)
To: GCC Patches; +Cc: Jan Hubicka, Richard Biener
PR 93385 reveals that if the user explicitely disables DCE, IPA-SRA
can leave behind statements which are useless because their results
are eventually not used but can have problematic side effects,
especially since their inputs are now bogus that useless parameters
were removed.
This patch fixes the problem by doing a similar def-use walk when
materializing clones, marking which statements should not be copied
and which SSA_NAMEs will be removed by call redirections and now need
to be replaced with anything valid. Default-definition SSA_NAMEs of
parameters which are removed and all SSA_NAMEs derived from them (in a
phi node or a simple assignment statement) are then remapped to
error_mark_node - a sure way to spot it if any is left in place.
There is one exception to the above rule, if such SSA_NAMEs appear as
an argument of a call, they need to be removed by call redirection and
not as part of clone materialization. So to have something valid
there until that time, this patch pulls out dummy declarations out of
thin air. If you do not like that, see patch number 4 in the series,
which changes this, but probably in a controversial way.
This patch only resets debug statements using the removed SSA_NAMEs.
The first follow-up patch adjusts debug statements in the current
function to still try to make the removed values available in debugger
in the current function and the subsequent one also in other functions
where they are passed.
gcc/ChangeLog:
2020-05-14 Martin Jambor <mjambor@suse.cz>
PR ipa/93385
* ipa-param-manipulation.h (class ipa_param_body_adjustments): New
members m_dead_stmts, m_dead_ssas, mark_dead_statements and
get_removed_call_arg_placeholder.
* ipa-param-manipulation.c (phi_arg_will_live_p): New function.
(ipa_param_body_adjustments::mark_dead_statements): New method.
(ipa_param_body_adjustments::common_initialization): Call it.
(ipa_param_body_adjustments::ipa_param_body_adjustments): Initialize
new mwmbers.
(ipa_param_body_adjustments::get_removed_call_arg_placeholder): New.
(ipa_param_body_adjustments::modify_call_stmt): Replace dead SSAs
with dummy decls.
* tree-inline.c (remap_gimple_stmt): Do not copy dead statements,
reset dead debug statements.
(copy_phis_for_bb): Do not copy dead PHI nodes.
gcc/testsuite/ChangeLog:
2020-05-14 Martin Jambor <mjambor@suse.cz>
PR ipa/93385
* gcc.dg/ipa/pr93385.c: New test.
* gcc.dg/ipa/ipa-sra-23.c: Likewise.
---
gcc/ipa-param-manipulation.c | 142 ++++++++++++++++++++++++--
gcc/ipa-param-manipulation.h | 8 ++
gcc/testsuite/gcc.dg/ipa/ipa-sra-23.c | 24 +++++
gcc/testsuite/gcc.dg/ipa/pr93385.c | 27 +++++
gcc/tree-inline.c | 18 +++-
5 files changed, 205 insertions(+), 14 deletions(-)
create mode 100644 gcc/testsuite/gcc.dg/ipa/ipa-sra-23.c
create mode 100644 gcc/testsuite/gcc.dg/ipa/pr93385.c
diff --git a/gcc/ipa-param-manipulation.c b/gcc/ipa-param-manipulation.c
index 978916057f0..1f47f3a4268 100644
--- a/gcc/ipa-param-manipulation.c
+++ b/gcc/ipa-param-manipulation.c
@@ -953,6 +953,99 @@ ipa_param_body_adjustments::carry_over_param (tree t)
return new_parm;
}
+/* Return true if BLOCKS_TO_COPY is NULL or if PHI has an argument ARG in
+ position that corresponds to an edge that is coming from a block that has
+ the corresponding bit set in BLOCKS_TO_COPY. */
+
+static bool
+phi_arg_will_live_p (gphi *phi, bitmap blocks_to_copy, tree arg)
+{
+ bool arg_will_survive = false;
+ if (!blocks_to_copy)
+ arg_will_survive = true;
+ else
+ for (unsigned i = 0; i < gimple_phi_num_args (phi); i++)
+ if (gimple_phi_arg_def (phi, i) == arg
+ && bitmap_bit_p (blocks_to_copy,
+ gimple_phi_arg_edge (phi, i)->src->index))
+ {
+ arg_will_survive = true;
+ break;
+ }
+ return arg_will_survive;
+}
+
+/* Populate m_dead_stmts given that DEAD_PARAM is going to be removed without
+ any replacement or splitting. */
+
+void
+ipa_param_body_adjustments::mark_dead_statements (tree dead_param)
+{
+ if (!is_gimple_reg (dead_param))
+ return;
+ tree parm_ddef = ssa_default_def (m_id->src_cfun, dead_param);
+ if (!parm_ddef || has_zero_uses (parm_ddef))
+ return;
+
+ auto_vec<tree, 4> stack;
+ m_dead_ssas.add (parm_ddef);
+ stack.safe_push (parm_ddef);
+ while (!stack.is_empty ())
+ {
+ tree t = stack.pop ();
+
+ imm_use_iterator imm_iter;
+ gimple *stmt;
+
+ insert_decl_map (m_id, t, error_mark_node);
+ FOR_EACH_IMM_USE_STMT (stmt, imm_iter, t)
+ {
+ if (is_gimple_call (stmt)
+ || (m_id->blocks_to_copy
+ && !bitmap_bit_p (m_id->blocks_to_copy,
+ gimple_bb (stmt)->index)))
+ continue;
+
+ if (is_gimple_debug (stmt))
+ {
+ m_dead_stmts.add (stmt);
+ gcc_assert (gimple_debug_bind_p (stmt));
+ }
+ else if (gimple_code (stmt) == GIMPLE_PHI)
+ {
+ gphi *phi = as_a <gphi *> (stmt);
+ if (phi_arg_will_live_p (phi, m_id->blocks_to_copy, t))
+ {
+ m_dead_stmts.add (phi);
+ tree res = gimple_phi_result (phi);
+ if (!m_dead_ssas.contains (res))
+ {
+ stack.safe_push (res);
+ m_dead_ssas.add (res);
+ }
+ }
+ }
+ else if (is_gimple_assign (stmt))
+ {
+ m_dead_stmts.add (stmt);
+ if (!gimple_clobber_p (stmt))
+ {
+ tree lhs = gimple_assign_lhs (stmt);
+ gcc_assert (TREE_CODE (lhs) == SSA_NAME);
+ if (!m_dead_ssas.contains (lhs))
+ {
+ m_dead_ssas.add (lhs);
+ stack.safe_push (lhs);
+ }
+ }
+ }
+ else
+ /* IPA-SRA does not analyze other types of statements. */
+ gcc_unreachable ();
+ }
+ }
+}
+
/* Common initialization performed by all ipa_param_body_adjustments
constructors. OLD_FNDECL is the declaration we take original arguments
from, (it may be the same as M_FNDECL). VARS, if non-NULL, is a pointer to
@@ -1095,6 +1188,11 @@ ipa_param_body_adjustments::common_initialization (tree old_fndecl,
/* Declare this new variable. */
DECL_CHAIN (var) = *vars;
*vars = var;
+
+ /* If this is not a split but a real removal, init hash sets
+ that will guide what not to copy to the new body. */
+ if (!isra_dummy_decls[i])
+ mark_dead_statements (m_oparms[i]);
}
}
else
@@ -1151,9 +1249,10 @@ ipa_param_body_adjustments
::ipa_param_body_adjustments (vec<ipa_adjusted_param, va_gc> *adj_params,
tree fndecl)
: m_adj_params (adj_params), m_adjustments (NULL), m_reset_debug_decls (),
- m_split_modifications_p (false), m_fndecl (fndecl), m_id (NULL),
- m_oparms (), m_new_decls (), m_new_types (), m_replacements (),
- m_removed_decls (), m_removed_map (), m_method2func (false)
+ m_split_modifications_p (false), m_dead_stmts (), m_dead_ssas (),
+ m_fndecl (fndecl), m_id (NULL), m_oparms (), m_new_decls (),
+ m_new_types (), m_replacements (), m_removed_decls (), m_removed_map (),
+ m_method2func (false)
{
common_initialization (fndecl, NULL, NULL);
}
@@ -1167,9 +1266,9 @@ ipa_param_body_adjustments
::ipa_param_body_adjustments (ipa_param_adjustments *adjustments,
tree fndecl)
: m_adj_params (adjustments->m_adj_params), m_adjustments (adjustments),
- m_reset_debug_decls (), m_split_modifications_p (false), m_fndecl (fndecl),
- m_id (NULL), m_oparms (), m_new_decls (), m_new_types (),
- m_replacements (), m_removed_decls (), m_removed_map (),
+ m_reset_debug_decls (), m_split_modifications_p (false), m_dead_stmts (),
+ m_dead_ssas (), m_fndecl (fndecl), m_id (NULL), m_oparms (), m_new_decls (),
+ m_new_types (), m_replacements (), m_removed_decls (), m_removed_map (),
m_method2func (false)
{
common_initialization (fndecl, NULL, NULL);
@@ -1190,9 +1289,10 @@ ipa_param_body_adjustments
copy_body_data *id, tree *vars,
vec<ipa_replace_map *, va_gc> *tree_map)
: m_adj_params (adjustments->m_adj_params), m_adjustments (adjustments),
- m_reset_debug_decls (), m_split_modifications_p (false), m_fndecl (fndecl),
- m_id (id), m_oparms (), m_new_decls (), m_new_types (), m_replacements (),
- m_removed_decls (), m_removed_map (), m_method2func (false)
+ m_reset_debug_decls (), m_split_modifications_p (false), m_dead_stmts (),
+ m_dead_ssas (),m_fndecl (fndecl), m_id (id), m_oparms (), m_new_decls (),
+ m_new_types (), m_replacements (), m_removed_decls (), m_removed_map (),
+ m_method2func (false)
{
common_initialization (old_fndecl, vars, tree_map);
}
@@ -1519,6 +1619,19 @@ remap_split_decl_to_dummy (tree *tp, int *walk_subtrees, void *data)
return NULL_TREE;
}
+/* 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. */
+
+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;
+}
/* If the call statement pointed at by STMT_P contains any expressions that
need to replaced with a different one as noted by ADJUSTMENTS, do so. f the
@@ -1658,7 +1771,10 @@ ipa_param_body_adjustments::modify_call_stmt (gcall **stmt_p)
else
{
tree t = gimple_call_arg (stmt, i);
- modify_expression (&t, true);
+ if (TREE_CODE (t) == SSA_NAME && m_dead_ssas.contains(t))
+ t = get_removed_call_arg_placeholder (t);
+ else
+ modify_expression (&t, true);
vargs.safe_push (t);
}
}
@@ -1680,7 +1796,11 @@ ipa_param_body_adjustments::modify_call_stmt (gcall **stmt_p)
for (unsigned i = 0; i < nargs; i++)
{
tree *t = gimple_call_arg_ptr (stmt, i);
- modified |= modify_expression (t, true);
+
+ if (TREE_CODE (*t) == SSA_NAME && m_dead_ssas.contains(*t))
+ *t = get_removed_call_arg_placeholder (*t);
+ else
+ modified |= modify_expression (t, true);
}
if (gimple_call_lhs (stmt))
diff --git a/gcc/ipa-param-manipulation.h b/gcc/ipa-param-manipulation.h
index 0b038ea57f1..59060ae5dcc 100644
--- a/gcc/ipa-param-manipulation.h
+++ b/gcc/ipa-param-manipulation.h
@@ -370,6 +370,12 @@ public:
/* Set to true if there are any IPA_PARAM_OP_SPLIT adjustments among stored
adjustments. */
bool m_split_modifications_p;
+
+ /* Sets of statements and SSA_NAMEs that only manipulate data from parameters
+ removed because they are not necessary. */
+ hash_set<gimple *> m_dead_stmts;
+ hash_set<tree> m_dead_ssas;
+
private:
void common_initialization (tree old_fndecl, tree *vars,
vec<ipa_replace_map *, va_gc> *tree_map);
@@ -383,6 +389,8 @@ private:
bool modify_call_stmt (gcall **stmt_p);
bool modify_cfun_body ();
void reset_debug_stmts ();
+ void mark_dead_statements (tree dead_param);
+ tree get_removed_call_arg_placeholder (tree arg);
/* Declaration of the function that is being transformed. */
diff --git a/gcc/testsuite/gcc.dg/ipa/ipa-sra-23.c b/gcc/testsuite/gcc.dg/ipa/ipa-sra-23.c
new file mode 100644
index 00000000000..f438b509614
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/ipa/ipa-sra-23.c
@@ -0,0 +1,24 @@
+/* { dg-do compile } */
+/* { dg-options "-O2" } */
+
+extern int g;
+
+static int __attribute__((noinline))
+bar (int i, int j)
+{
+ return 2*g + i;
+}
+
+static int __attribute__((noinline))
+foo (int i, int j)
+{
+ if (i > 5)
+ j = 22;
+ return bar (i, j) + 1;
+}
+
+int
+entry (int l, int k)
+{
+ return foo (l, k);
+}
diff --git a/gcc/testsuite/gcc.dg/ipa/pr93385.c b/gcc/testsuite/gcc.dg/ipa/pr93385.c
new file mode 100644
index 00000000000..6d1d0d7cd27
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/ipa/pr93385.c
@@ -0,0 +1,27 @@
+/* { dg-do run } */
+/* { dg-options "-O2 -fno-dce -fno-ipa-cp -fno-tree-dce" } */
+
+char a, b;
+
+#ifdef __SIZEOF_INT128__
+#define T unsigned __int128
+#else
+#define T unsigned
+#endif
+
+static inline int
+c (T d)
+{
+ char e = 0;
+ d %= (unsigned) d;
+ e -= 0;
+ __builtin_strncpy (&a, &e, 1);
+ return e + b;
+}
+
+int
+main (void)
+{
+ c (~0);
+ return 0;
+}
diff --git a/gcc/tree-inline.c b/gcc/tree-inline.c
index 3160ca3f88a..60087dd5e7b 100644
--- a/gcc/tree-inline.c
+++ b/gcc/tree-inline.c
@@ -1524,6 +1524,11 @@ remap_gimple_stmt (gimple *stmt, copy_body_data *id)
: !opt_for_fn (id->dst_fn, flag_var_tracking_assignments)))
return NULL;
+ if (!is_gimple_debug (stmt)
+ && id->param_body_adjs
+ && id->param_body_adjs->m_dead_stmts.contains (stmt))
+ return NULL;
+
/* Begin by recognizing trees that we'll completely rewrite for the
inlining context. Our output for these trees is completely
different from our input (e.g. RETURN_EXPR is deleted and morphs
@@ -1788,10 +1793,15 @@ remap_gimple_stmt (gimple *stmt, copy_body_data *id)
if (gimple_debug_bind_p (stmt))
{
+ tree value;
+ if (id->param_body_adjs
+ && id->param_body_adjs->m_dead_stmts.contains (stmt))
+ value = NULL_TREE;
+ else
+ value = gimple_debug_bind_get_value (stmt);
gdebug *copy
= gimple_build_debug_bind (gimple_debug_bind_get_var (stmt),
- gimple_debug_bind_get_value (stmt),
- stmt);
+ value, stmt);
if (id->reset_location)
gimple_set_location (copy, input_location);
id->debug_stmts.safe_push (copy);
@@ -2671,7 +2681,9 @@ copy_phis_for_bb (basic_block bb, copy_body_data *id)
phi = si.phi ();
res = PHI_RESULT (phi);
new_res = res;
- if (!virtual_operand_p (res))
+ if (!virtual_operand_p (res)
+ && (!id->param_body_adjs
+ || !id->param_body_adjs->m_dead_stmts.contains (phi)))
{
walk_tree (&new_res, copy_tree_body_r, id, NULL);
if (EDGE_COUNT (new_bb->preds) == 0)
--
2.26.2
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH 3/4] ipa-sra: Improve debug info for removed parameters (PR 93385)
2020-05-28 12:06 [PATCH 0/4] Make IPA-SRA not depend on tree-dce and related fixes Martin Jambor
2020-05-28 12:06 ` [PATCH 1/4] ipa-sra: Do not remove statements necessary because of non-call EH (PR 95113) Martin Jambor
2020-05-28 12:06 ` [PATCH 2/4] ipa-sra: Introduce a mini-DCE to tree-inline.c (PR 93385) Martin Jambor
@ 2020-05-28 12:06 ` Martin Jambor
2020-05-28 12:06 ` [PATCH 4/4] ipa-sra: Fix debug info for removed args passed to other functions (PR 93385, 95343) Martin Jambor
3 siblings, 0 replies; 9+ messages in thread
From: Martin Jambor @ 2020-05-28 12:06 UTC (permalink / raw)
To: GCC Patches; +Cc: Jan Hubicka, Richard Biener
Whereas the previous patch fixed issues with code left behind after
IPA-SRA removed a parameter but only reset all affected debug bind
statements, this one updates them with expressions which can allow the
debugger to print the removed value - see the added test-case.
Even though I originally did not want to create DEBUG_EXPR_DECLs for
intermediate values, I ended up doing so, because otherwise the code
started creating statements like
# DEBUG __aD.198693 => &MEM[(const struct _Alloc_nodeD.171110 *)D#195]._M_tD.184726->_M_implD.171154
which not only is a bit scary but gimple-fold also ICEs on
it. Therefore I decided they are probably quite necessary and have
them.
The patch simply notes each removed SSA name present in a debug
statement and then works from it backwards, looking if it can
reconstruct the expression it represents (which can fail if a
non-degenerate PHI node is in the way). If it can, it populates two
hash maps with those expressions so that 1) removed assignments are
replaced with a debug bind defining a new intermediate debug_decl_expr
and 2) existing debug binds that refer to SSA names that are bing
removed now refer to corresponding debug_decl_exprs.
If a removed parameter is passed to another function, the debugging
information still cannot describe its value there - see the xfailed
test in the testcase. This will is addressed in the following patch
which removes the xfail.
---
gcc/ipa-param-manipulation.c | 271 ++++++++++++++++++-----
gcc/ipa-param-manipulation.h | 12 +-
gcc/testsuite/gcc.dg/guality/ipa-sra-1.c | 45 ++++
gcc/tree-inline.c | 45 ++--
4 files changed, 302 insertions(+), 71 deletions(-)
create mode 100644 gcc/testsuite/gcc.dg/guality/ipa-sra-1.c
diff --git a/gcc/ipa-param-manipulation.c b/gcc/ipa-param-manipulation.c
index 1f47f3a4268..0a265e26c4f 100644
--- a/gcc/ipa-param-manipulation.c
+++ b/gcc/ipa-param-manipulation.c
@@ -40,6 +40,8 @@ along with GCC; see the file COPYING3. If not see
#include "builtins.h"
#include "tree-ssa.h"
#include "tree-inline.h"
+#include "tree-phinodes.h"
+#include "cfgexpand.h"
/* Actual prefixes of different newly synthetized parameters. Keep in sync
@@ -979,7 +981,8 @@ phi_arg_will_live_p (gphi *phi, bitmap blocks_to_copy, tree arg)
any replacement or splitting. */
void
-ipa_param_body_adjustments::mark_dead_statements (tree dead_param)
+ipa_param_body_adjustments::mark_dead_statements (tree dead_param,
+ vec<tree> *debugstack)
{
if (!is_gimple_reg (dead_param))
return;
@@ -988,6 +991,7 @@ ipa_param_body_adjustments::mark_dead_statements (tree dead_param)
return;
auto_vec<tree, 4> stack;
+ hash_set<tree> used_in_debug;
m_dead_ssas.add (parm_ddef);
stack.safe_push (parm_ddef);
while (!stack.is_empty ())
@@ -1010,6 +1014,11 @@ ipa_param_body_adjustments::mark_dead_statements (tree dead_param)
{
m_dead_stmts.add (stmt);
gcc_assert (gimple_debug_bind_p (stmt));
+ if (!used_in_debug.contains (t))
+ {
+ used_in_debug.add (t);
+ debugstack->safe_push (t);
+ }
}
else if (gimple_code (stmt) == GIMPLE_PHI)
{
@@ -1044,6 +1053,155 @@ ipa_param_body_adjustments::mark_dead_statements (tree dead_param)
gcc_unreachable ();
}
}
+
+ if (!MAY_HAVE_DEBUG_STMTS)
+ {
+ gcc_assert (debugstack->is_empty ());
+ return;
+ }
+
+ tree dp_ddecl = make_node (DEBUG_EXPR_DECL);
+ DECL_ARTIFICIAL (dp_ddecl) = 1;
+ TREE_TYPE (dp_ddecl) = TREE_TYPE (dead_param);
+ SET_DECL_MODE (dp_ddecl, DECL_MODE (dead_param));
+ m_dead_ssa_debug_equiv.put (parm_ddef, dp_ddecl);
+}
+
+/* Callback to walk_tree. If REMAP is an SSA_NAME that is present in hash_map
+ passed in DATA, replace it with unshared version of what it was mapped
+ to. */
+
+static tree
+replace_with_mapped_expr (tree *remap, int *walk_subtrees, void *data)
+{
+ if (TYPE_P (*remap))
+ {
+ *walk_subtrees = 0;
+ return 0;
+ }
+ if (TREE_CODE (*remap) != SSA_NAME)
+ return 0;
+
+ *walk_subtrees = 0;
+
+ hash_map<tree, tree> *equivs = (hash_map<tree, tree> *) data;
+ if (tree *p = equivs->get (*remap))
+ *remap = unshare_expr (*p);
+ return 0;
+}
+
+/* Replace all occurances of SSAs in m_dead_ssa_debug_equiv in t with what they
+ are mapped to. */
+
+void
+ipa_param_body_adjustments::remap_with_debug_expressions (tree *t)
+{
+ /* If *t is an SSA_NAME which should have its debug statements reset, it is
+ mapped to NULL in the hash_map. We need to handle that case separately or
+ otherwise the walker would segfault. No expression that is more
+ complicated than that can have its operands mapped to NULL. */
+ if (TREE_CODE (*t) == SSA_NAME)
+ {
+ if (tree *p = m_dead_ssa_debug_equiv.get (*t))
+ *t = *p;
+ }
+ else
+ walk_tree (t, replace_with_mapped_expr, &m_dead_ssa_debug_equiv, NULL);
+}
+
+/* For an SSA_NAME DEAD_SSA which is about to be DCEd because it is based on a
+ useless parameter, prepare an expression that should represent it in
+ debug_binds in the cloned function and add a mapping from DEAD_SSA to
+ m_dead_ssa_debug_equiv. That mapping is to NULL when the associated
+ debug_statement has to be reset instead. In such case return false,
+ ottherwise return true. If DEAD_SSA comes from a basic block which is not
+ about to be copied, ignore it and return true. */
+
+bool
+ipa_param_body_adjustments::prepare_debug_expressions (tree dead_ssa)
+{
+ gcc_checking_assert (m_dead_ssas.contains (dead_ssa));
+ if (tree *d = m_dead_ssa_debug_equiv.get (dead_ssa))
+ return (*d != NULL_TREE);
+
+ gcc_assert (!SSA_NAME_IS_DEFAULT_DEF (dead_ssa));
+ gimple *def = SSA_NAME_DEF_STMT (dead_ssa);
+ if (m_id->blocks_to_copy
+ && !bitmap_bit_p (m_id->blocks_to_copy, gimple_bb (def)->index))
+ return true;
+
+ if (gimple_code (def) == GIMPLE_PHI)
+ {
+ /* In theory, we could ignore all SSAs coming from BBs not in
+ m_id->blocks_to_copy but at the time of the writing this code that
+ should never really be the case because only fnsplit uses that bitmap,
+ so don't bother. */
+ tree value = degenerate_phi_result (as_a <gphi *> (def));
+ if (!value
+ || (m_dead_ssas.contains (value)
+ && !prepare_debug_expressions (value)))
+ {
+ m_dead_ssa_debug_equiv.put (dead_ssa, NULL_TREE);
+ return false;
+ }
+
+ /* PHI operand can be either an invariant or an SSA_NAME, but we are
+ looking at a degenarete phi node having value from a removed
+ parameter, so it has to be the latter. */
+ gcc_assert (TREE_CODE (value) == SSA_NAME);
+
+ tree *d = m_dead_ssa_debug_equiv.get (value);
+ m_dead_ssa_debug_equiv.put (dead_ssa, *d);
+ return true;
+ }
+
+ bool lost = false;
+ use_operand_p use_p;
+ ssa_op_iter oi;
+ FOR_EACH_PHI_OR_STMT_USE (use_p, def, oi, SSA_OP_USE)
+ {
+ tree use = USE_FROM_PTR (use_p);
+ if (m_dead_ssas.contains (use)
+ && !prepare_debug_expressions (use))
+ {
+ lost = true;
+ break;
+ }
+ }
+
+ if (lost)
+ {
+ m_dead_ssa_debug_equiv.put (dead_ssa, NULL_TREE);
+ return false;
+ }
+
+ if (is_gimple_assign (def))
+ {
+ gcc_checking_assert (!gimple_clobber_p (def)
+ && gimple_assign_lhs (def) == dead_ssa);
+
+ if (gimple_assign_copy_p (def)
+ && TREE_CODE (gimple_assign_rhs1 (def)) == SSA_NAME)
+ {
+ tree *d = m_dead_ssa_debug_equiv.get (gimple_assign_rhs1 (def));
+ m_dead_ssa_debug_equiv.put (dead_ssa, *d);
+ return (*d != NULL_TREE);
+ }
+
+ tree val = gimple_assign_rhs_to_tree (def);
+ SET_EXPR_LOCATION (val, UNKNOWN_LOCATION);
+ remap_with_debug_expressions (&val);
+
+ tree vexpr = make_node (DEBUG_EXPR_DECL);
+ DECL_ARTIFICIAL (vexpr) = 1;
+ TREE_TYPE (vexpr) = TREE_TYPE (val);
+ SET_DECL_MODE (vexpr, TYPE_MODE (TREE_TYPE (val)));
+ m_dead_stmt_debug_equiv.put (def, val);
+ m_dead_ssa_debug_equiv.put (dead_ssa, vexpr);
+ return true;
+ }
+ else
+ gcc_unreachable ();
}
/* Common initialization performed by all ipa_param_body_adjustments
@@ -1161,6 +1319,30 @@ ipa_param_body_adjustments::common_initialization (tree old_fndecl,
gcc_unreachable ();
}
+ if (tree_map)
+ {
+ /* Do not treat parameters which were replaced with a constant as
+ completely vanished. */
+ auto_vec <int, 16> index_mapping;
+ bool need_remap = false;
+
+ if (m_id && m_id->src_node->clone.param_adjustments)
+ {
+ ipa_param_adjustments *prev_adjustments
+ = m_id->src_node->clone.param_adjustments;
+ prev_adjustments->get_updated_indices (&index_mapping);
+ need_remap = true;
+ }
+
+ for (unsigned i = 0; i < tree_map->length (); i++)
+ {
+ int parm_num = (*tree_map)[i]->parm_num;
+ gcc_assert (parm_num >= 0);
+ if (need_remap)
+ parm_num = index_mapping[parm_num];
+ kept[parm_num] = true;
+ }
+ }
/* As part of body modifications, we will also have to replace remaining uses
of remaining uses of removed PARM_DECLs (which do not however use the
@@ -1173,70 +1355,43 @@ ipa_param_body_adjustments::common_initialization (tree old_fndecl,
replace_removed_params_ssa_names or perform_cfun_body_modifications when
you construct with ID not equal to NULL. */
+ auto_vec<tree, 8> ssas_to_process_debug;
unsigned op_len = m_oparms.length ();
for (unsigned i = 0; i < op_len; i++)
if (!kept[i])
{
if (m_id)
{
- if (!m_id->decl_map->get (m_oparms[i]))
- {
- /* TODO: Perhaps at least aggregate-type params could re-use
- their isra_dummy_decl here? */
- tree var = copy_decl_to_var (m_oparms[i], m_id);
- insert_decl_map (m_id, m_oparms[i], var);
- /* Declare this new variable. */
- DECL_CHAIN (var) = *vars;
- *vars = var;
-
- /* If this is not a split but a real removal, init hash sets
- that will guide what not to copy to the new body. */
- if (!isra_dummy_decls[i])
- mark_dead_statements (m_oparms[i]);
- }
+ gcc_assert (!m_id->decl_map->get (m_oparms[i]));
+ /* TODO: Perhaps at least aggregate-type params could re-use
+ their isra_dummy_decl here? */
+ tree var = copy_decl_to_var (m_oparms[i], m_id);
+ insert_decl_map (m_id, m_oparms[i], var);
+ /* Declare this new variable. */
+ DECL_CHAIN (var) = *vars;
+ *vars = var;
+
+ /* If this is not a split but a real removal, init hash sets
+ that will guide what not to copy to the new body. */
+ if (!isra_dummy_decls[i])
+ mark_dead_statements (m_oparms[i], &ssas_to_process_debug);
+ if (MAY_HAVE_DEBUG_STMTS
+ && is_gimple_reg (m_oparms[i]))
+ m_reset_debug_decls.safe_push (m_oparms[i]);
}
else
{
m_removed_decls.safe_push (m_oparms[i]);
m_removed_map.put (m_oparms[i], m_removed_decls.length () - 1);
+ if (MAY_HAVE_DEBUG_STMTS
+ && !kept[i]
+ && is_gimple_reg (m_oparms[i]))
+ m_reset_debug_decls.safe_push (m_oparms[i]);
}
}
- if (!MAY_HAVE_DEBUG_STMTS)
- return;
-
- /* Finally, when generating debug info, we fill vector m_reset_debug_decls
- with removed parameters declarations. We do this in order to re-map their
- debug bind statements and create debug decls for them. */
-
- if (tree_map)
- {
- /* Do not output debuginfo for parameter declarations as if they vanished
- when they were in fact replaced by a constant. */
- auto_vec <int, 16> index_mapping;
- bool need_remap = false;
-
- if (m_id && m_id->src_node->clone.param_adjustments)
- {
- ipa_param_adjustments *prev_adjustments
- = m_id->src_node->clone.param_adjustments;
- prev_adjustments->get_updated_indices (&index_mapping);
- need_remap = true;
- }
-
- for (unsigned i = 0; i < tree_map->length (); i++)
- {
- int parm_num = (*tree_map)[i]->parm_num;
- gcc_assert (parm_num >= 0);
- if (need_remap)
- parm_num = index_mapping[parm_num];
- kept[parm_num] = true;
- }
- }
-
- for (unsigned i = 0; i < op_len; i++)
- if (!kept[i] && is_gimple_reg (m_oparms[i]))
- m_reset_debug_decls.safe_push (m_oparms[i]);
+ while (!ssas_to_process_debug.is_empty ())
+ prepare_debug_expressions (ssas_to_process_debug.pop ());
}
/* Constructor of ipa_param_body_adjustments from a simple list of
@@ -1250,9 +1405,9 @@ ipa_param_body_adjustments
tree fndecl)
: m_adj_params (adj_params), m_adjustments (NULL), m_reset_debug_decls (),
m_split_modifications_p (false), m_dead_stmts (), m_dead_ssas (),
- m_fndecl (fndecl), m_id (NULL), m_oparms (), m_new_decls (),
- m_new_types (), m_replacements (), m_removed_decls (), m_removed_map (),
- m_method2func (false)
+ m_dead_ssa_debug_equiv (), m_dead_stmt_debug_equiv (), m_fndecl (fndecl),
+ m_id (NULL), m_oparms (), m_new_decls (), m_new_types (), m_replacements (),
+ m_removed_decls (), m_removed_map (), m_method2func (false)
{
common_initialization (fndecl, NULL, NULL);
}
@@ -1267,7 +1422,8 @@ ipa_param_body_adjustments
tree fndecl)
: m_adj_params (adjustments->m_adj_params), m_adjustments (adjustments),
m_reset_debug_decls (), m_split_modifications_p (false), m_dead_stmts (),
- m_dead_ssas (), m_fndecl (fndecl), m_id (NULL), m_oparms (), m_new_decls (),
+ m_dead_ssas (), m_dead_ssa_debug_equiv (), m_dead_stmt_debug_equiv (),
+ m_fndecl (fndecl), m_id (NULL), m_oparms (), m_new_decls (),
m_new_types (), m_replacements (), m_removed_decls (), m_removed_map (),
m_method2func (false)
{
@@ -1290,8 +1446,9 @@ ipa_param_body_adjustments
vec<ipa_replace_map *, va_gc> *tree_map)
: m_adj_params (adjustments->m_adj_params), m_adjustments (adjustments),
m_reset_debug_decls (), m_split_modifications_p (false), m_dead_stmts (),
- m_dead_ssas (),m_fndecl (fndecl), m_id (id), m_oparms (), m_new_decls (),
- m_new_types (), m_replacements (), m_removed_decls (), m_removed_map (),
+ m_dead_ssas (), m_dead_ssa_debug_equiv (), m_dead_stmt_debug_equiv (),
+ m_fndecl (fndecl), m_id (id), m_oparms (), m_new_decls (), m_new_types (),
+ m_replacements (), m_removed_decls (), m_removed_map (),
m_method2func (false)
{
common_initialization (old_fndecl, vars, tree_map);
diff --git a/gcc/ipa-param-manipulation.h b/gcc/ipa-param-manipulation.h
index 59060ae5dcc..240cc1583ea 100644
--- a/gcc/ipa-param-manipulation.h
+++ b/gcc/ipa-param-manipulation.h
@@ -356,6 +356,9 @@ public:
bool modify_gimple_stmt (gimple **stmt, gimple_seq *extra_stmts);
/* Return the new chain of parameters. */
tree get_new_param_chain ();
+ /* Replace all occurances of SSAs in m_dead_ssa_debug_equiv in t with what
+ they are mapped to. */
+ void remap_with_debug_expressions (tree *t);
/* Pointers to data structures defining how the function should be
modified. */
@@ -376,6 +379,12 @@ public:
hash_set<gimple *> m_dead_stmts;
hash_set<tree> m_dead_ssas;
+ /* Mapping from DCEd SSAs to what their potential debug_binds should be. */
+ hash_map<tree, tree> m_dead_ssa_debug_equiv;
+ /* Mapping from DCEd statements to debug expressions that will be placed on
+ the RHS of debug statement that will replace this one. */
+ hash_map<gimple *, tree> m_dead_stmt_debug_equiv;
+
private:
void common_initialization (tree old_fndecl, tree *vars,
vec<ipa_replace_map *, va_gc> *tree_map);
@@ -389,7 +398,8 @@ private:
bool modify_call_stmt (gcall **stmt_p);
bool modify_cfun_body ();
void reset_debug_stmts ();
- void mark_dead_statements (tree dead_param);
+ void mark_dead_statements (tree dead_param, vec<tree> *debugstack);
+ bool prepare_debug_expressions (tree dead_ssa);
tree get_removed_call_arg_placeholder (tree arg);
/* Declaration of the function that is being transformed. */
diff --git a/gcc/testsuite/gcc.dg/guality/ipa-sra-1.c b/gcc/testsuite/gcc.dg/guality/ipa-sra-1.c
new file mode 100644
index 00000000000..5434b3d7665
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/guality/ipa-sra-1.c
@@ -0,0 +1,45 @@
+/* { dg-do run } */
+/* { dg-options "-g -fno-ipa-icf" } */
+
+
+void __attribute__((noipa))
+use (int x)
+{
+ asm volatile ("" : : "r" (x) : "memory");
+}
+
+static int __attribute__((noinline))
+bar (int i, int k)
+{
+ asm ("" : "+r" (i));
+ use (i); /* { dg-final { gdb-test . "k" "3" { xfail *-*-* } } } */
+ return 6;
+}
+
+volatile int v;
+
+static int __attribute__((noinline))
+foo (int i, int k)
+{
+ int r;
+ v = 9;
+ k = (k + 14)/k;
+ r = bar (i, k); /* { dg-final { gdb-test . "k" "3" } } */
+ return r;
+}
+
+volatile int v;
+
+int __attribute__((noipa))
+get_val1 (void) {return 20;}
+int __attribute__((noipa))
+get_val2 (void) {return 7;}
+
+int
+main (void)
+{
+ int k = get_val2 ();
+ int r = foo (get_val1 (), k);
+ v = r + k; /* k has to live accross the call or all is probably lost */
+ return 0;
+}
diff --git a/gcc/tree-inline.c b/gcc/tree-inline.c
index 60087dd5e7b..33995eaa9b5 100644
--- a/gcc/tree-inline.c
+++ b/gcc/tree-inline.c
@@ -1527,7 +1527,21 @@ remap_gimple_stmt (gimple *stmt, copy_body_data *id)
if (!is_gimple_debug (stmt)
&& id->param_body_adjs
&& id->param_body_adjs->m_dead_stmts.contains (stmt))
- return NULL;
+ {
+ tree *dval = id->param_body_adjs->m_dead_stmt_debug_equiv.get (stmt);
+ if (!dval)
+ return NULL;
+
+ gcc_assert (is_gimple_assign (stmt));
+ tree lhs = gimple_assign_lhs (stmt);
+ tree *dvar = id->param_body_adjs->m_dead_ssa_debug_equiv.get (lhs);
+ gdebug *bind = gimple_build_debug_bind (*dvar, *dval, stmt);
+ if (id->reset_location)
+ gimple_set_location (bind, input_location);
+ id->debug_stmts.safe_push (bind);
+ gimple_seq_add_stmt (&stmts, bind);
+ return stmts;
+ }
/* Begin by recognizing trees that we'll completely rewrite for the
inlining context. Our output for these trees is completely
@@ -1793,15 +1807,13 @@ remap_gimple_stmt (gimple *stmt, copy_body_data *id)
if (gimple_debug_bind_p (stmt))
{
- tree value;
+ tree var = gimple_debug_bind_get_var (stmt);
+ tree value = gimple_debug_bind_get_value (stmt);
if (id->param_body_adjs
&& id->param_body_adjs->m_dead_stmts.contains (stmt))
- value = NULL_TREE;
- else
- value = gimple_debug_bind_get_value (stmt);
- gdebug *copy
- = gimple_build_debug_bind (gimple_debug_bind_get_var (stmt),
- value, stmt);
+ id->param_body_adjs->remap_with_debug_expressions (&value);
+
+ gdebug *copy = gimple_build_debug_bind (var, value, stmt);
if (id->reset_location)
gimple_set_location (copy, input_location);
id->debug_stmts.safe_push (copy);
@@ -6468,7 +6480,6 @@ tree_function_versioning (tree old_decl, tree new_decl,
in the debug info that var (whole DECL_ORIGIN is the parm
PARM_DECL) is optimized away, but could be looked up at the
call site as value of D#X there. */
- tree vexpr;
gimple_stmt_iterator cgsi
= gsi_after_labels (single_succ (ENTRY_BLOCK_PTR_FOR_FN (cfun)));
gimple *def_temp;
@@ -6476,17 +6487,25 @@ tree_function_versioning (tree old_decl, tree new_decl,
i = vec_safe_length (*debug_args);
do
{
+ tree vexpr = NULL_TREE;
i -= 2;
while (var != NULL_TREE
&& DECL_ABSTRACT_ORIGIN (var) != (**debug_args)[i])
var = TREE_CHAIN (var);
if (var == NULL_TREE)
break;
- vexpr = make_node (DEBUG_EXPR_DECL);
tree parm = (**debug_args)[i];
- DECL_ARTIFICIAL (vexpr) = 1;
- TREE_TYPE (vexpr) = TREE_TYPE (parm);
- SET_DECL_MODE (vexpr, DECL_MODE (parm));
+ if (tree parm_ddef = ssa_default_def (id.src_cfun, parm))
+ if (tree *d
+ = param_body_adjs->m_dead_ssa_debug_equiv.get (parm_ddef))
+ vexpr = *d;
+ if (!vexpr)
+ {
+ vexpr = make_node (DEBUG_EXPR_DECL);
+ DECL_ARTIFICIAL (vexpr) = 1;
+ TREE_TYPE (vexpr) = TREE_TYPE (parm);
+ SET_DECL_MODE (vexpr, DECL_MODE (parm));
+ }
def_temp = gimple_build_debug_bind (var, vexpr, NULL);
gsi_insert_before (&cgsi, def_temp, GSI_NEW_STMT);
def_temp = gimple_build_debug_source_bind (vexpr, parm, NULL);
--
2.26.2
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH 4/4] ipa-sra: Fix debug info for removed args passed to other functions (PR 93385, 95343)
2020-05-28 12:06 [PATCH 0/4] Make IPA-SRA not depend on tree-dce and related fixes Martin Jambor
` (2 preceding siblings ...)
2020-05-28 12:06 ` [PATCH 3/4] ipa-sra: Improve debug info for removed parameters " Martin Jambor
@ 2020-05-28 12:06 ` Martin Jambor
2020-06-08 11:40 ` Martin Jambor
3 siblings, 1 reply; 9+ messages in thread
From: Martin Jambor @ 2020-05-28 12:06 UTC (permalink / raw)
To: GCC Patches; +Cc: Jan Hubicka, Richard Biener
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
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;
--
2.26.2
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/4] ipa-sra: Do not remove statements necessary because of non-call EH (PR 95113)
2020-05-28 12:06 ` [PATCH 1/4] ipa-sra: Do not remove statements necessary because of non-call EH (PR 95113) Martin Jambor
@ 2020-06-02 11:42 ` Richard Biener
0 siblings, 0 replies; 9+ messages in thread
From: Richard Biener @ 2020-06-02 11:42 UTC (permalink / raw)
To: Martin Jambor; +Cc: GCC Patches, Jan Hubicka
On Thu, 28 May 2020, Martin Jambor wrote:
> PR 95113 revealed that when reasoning about which parameters are dead,
> IPA-SRA does not perform the same check related to non-call exceptions
> as tree DCE. It most certainly should and so this patch moves the
> condition used in tree-ssa-dce.c into a separate predicate (in
> tree-eh.c) and uses it from both places.
OK.
Thanks,
Richard.
> gcc/ChangeLog:
>
> 2020-05-27 Martin Jambor <mjambor@suse.cz>
>
> PR ipa/95113
> * gcc/tree-ssa-dce.c (mark_stmt_if_obviously_necessary): Move non-call
> exceptions check to...
> * gcc/tree-eh.c (stmt_unremovable_because_of_non_call_eh_p): ...this
> new function.
> * gcc/tree-eh.h (stmt_unremovable_because_of_non_call_eh_p): Declare it.
> * gcc/ipa-sra.c (isra_track_scalar_value_uses): Use it. New parameter
> fun.
>
> gcc/testsuite/ChangeLog:
>
> 2020-05-27 Martin Jambor <mjambor@suse.cz>
>
> PR ipa/95113
> * gcc.dg/ipa/pr95113.c: New test.
> ---
> gcc/ipa-sra.c | 28 +++++++++++++------------
> gcc/testsuite/gcc.dg/ipa/pr95113.c | 33 ++++++++++++++++++++++++++++++
> gcc/tree-eh.c | 10 +++++++++
> gcc/tree-eh.h | 1 +
> gcc/tree-ssa-dce.c | 4 +---
> 5 files changed, 60 insertions(+), 16 deletions(-)
> create mode 100644 gcc/testsuite/gcc.dg/ipa/pr95113.c
>
> diff --git a/gcc/ipa-sra.c b/gcc/ipa-sra.c
> index 7c922e40a4e..c81e8869e7a 100644
> --- a/gcc/ipa-sra.c
> +++ b/gcc/ipa-sra.c
> @@ -795,17 +795,17 @@ get_single_param_flow_source (const isra_param_flow *param_flow)
> }
>
> /* Inspect all uses of NAME and simple arithmetic calculations involving NAME
> - in NODE and return a negative number if any of them is used for something
> - else than either an actual call argument, simple arithmetic operation or
> - debug statement. If there are no such uses, return the number of actual
> - arguments that this parameter eventually feeds to (or zero if there is none).
> - For any such parameter, mark PARM_NUM as one of its sources. ANALYZED is a
> - bitmap that tracks which SSA names we have already started
> - investigating. */
> + in FUN represented with NODE and return a negative number if any of them is
> + used for something else than either an actual call argument, simple
> + arithmetic operation or debug statement. If there are no such uses, return
> + the number of actual arguments that this parameter eventually feeds to (or
> + zero if there is none). For any such parameter, mark PARM_NUM as one of its
> + sources. ANALYZED is a bitmap that tracks which SSA names we have already
> + started investigating. */
>
> static int
> -isra_track_scalar_value_uses (cgraph_node *node, tree name, int parm_num,
> - bitmap analyzed)
> +isra_track_scalar_value_uses (function *fun, cgraph_node *node, tree name,
> + int parm_num, bitmap analyzed)
> {
> int res = 0;
> imm_use_iterator imm_iter;
> @@ -859,8 +859,9 @@ isra_track_scalar_value_uses (cgraph_node *node, tree name, int parm_num,
> }
> res += all_uses;
> }
> - else if ((is_gimple_assign (stmt) && !gimple_has_volatile_ops (stmt))
> - || gimple_code (stmt) == GIMPLE_PHI)
> + else if (!stmt_unremovable_because_of_non_call_eh_p (fun, stmt)
> + && ((is_gimple_assign (stmt) && !gimple_has_volatile_ops (stmt))
> + || gimple_code (stmt) == GIMPLE_PHI))
> {
> tree lhs;
> if (gimple_code (stmt) == GIMPLE_PHI)
> @@ -876,7 +877,7 @@ isra_track_scalar_value_uses (cgraph_node *node, tree name, int parm_num,
> gcc_assert (!gimple_vdef (stmt));
> if (bitmap_set_bit (analyzed, SSA_NAME_VERSION (lhs)))
> {
> - int tmp = isra_track_scalar_value_uses (node, lhs, parm_num,
> + int tmp = isra_track_scalar_value_uses (fun, node, lhs, parm_num,
> analyzed);
> if (tmp < 0)
> {
> @@ -927,7 +928,8 @@ isra_track_scalar_param_local_uses (function *fun, cgraph_node *node, tree parm,
> return true;
>
> bitmap analyzed = BITMAP_ALLOC (NULL);
> - int call_uses = isra_track_scalar_value_uses (node, name, parm_num, analyzed);
> + int call_uses = isra_track_scalar_value_uses (fun, node, name, parm_num,
> + analyzed);
> BITMAP_FREE (analyzed);
> if (call_uses < 0)
> return true;
> diff --git a/gcc/testsuite/gcc.dg/ipa/pr95113.c b/gcc/testsuite/gcc.dg/ipa/pr95113.c
> new file mode 100644
> index 00000000000..a8f8c901ebe
> --- /dev/null
> +++ b/gcc/testsuite/gcc.dg/ipa/pr95113.c
> @@ -0,0 +1,33 @@
> +/* { dg-do run } */
> +/* { dg-options "-O2 -fexceptions -fnon-call-exceptions" } */
> +/* { dg-require-effective-target exceptions } */
> +
> +int a, b;
> +
> +static inline long int
> +foo (long int x, int y)
> +{
> + if (y == 0)
> + return 0;
> +
> + if (x == -1 && y == -1)
> + return 0;
> +
> + return x / y;
> +}
> +
> +static inline int
> +bar (int *p)
> +{
> + int c = foo (a, 1) + *p;
> + return b;
> +}
> +
> +int
> +main ()
> +{
> + int d = 0;
> + b = foo (1, 1);
> + bar (&d);
> + return 0;
> +}
> diff --git a/gcc/tree-eh.c b/gcc/tree-eh.c
> index 10ef2e3157c..4246dca8806 100644
> --- a/gcc/tree-eh.c
> +++ b/gcc/tree-eh.c
> @@ -2936,6 +2936,16 @@ stmt_could_throw_p (function *fun, gimple *stmt)
> }
> }
>
> +/* Return true if STMT in function FUN must be assumed necessary because of
> + non-call exceptions. */
> +
> +bool
> +stmt_unremovable_because_of_non_call_eh_p (function *fun, gimple *stmt)
> +{
> + return (fun->can_throw_non_call_exceptions
> + && !fun->can_delete_dead_exceptions
> + && stmt_could_throw_p (fun, stmt));
> +}
>
> /* Return true if expression T could throw an exception. */
>
> diff --git a/gcc/tree-eh.h b/gcc/tree-eh.h
> index cb1019e0d87..ba911cadbe7 100644
> --- a/gcc/tree-eh.h
> +++ b/gcc/tree-eh.h
> @@ -39,6 +39,7 @@ extern bool operation_could_trap_p (enum tree_code, bool, bool, tree);
> extern bool tree_could_trap_p (tree);
> extern tree rewrite_to_non_trapping_overflow (tree);
> extern bool stmt_could_throw_p (function *, gimple *);
> +extern bool stmt_unremovable_because_of_non_call_eh_p (function *, gimple *);
> extern bool tree_could_throw_p (tree);
> extern bool stmt_can_throw_external (function *, gimple *);
> extern bool stmt_can_throw_internal (function *, gimple *);
> diff --git a/gcc/tree-ssa-dce.c b/gcc/tree-ssa-dce.c
> index 757cfad5b5e..fae5ae72340 100644
> --- a/gcc/tree-ssa-dce.c
> +++ b/gcc/tree-ssa-dce.c
> @@ -201,9 +201,7 @@ mark_stmt_if_obviously_necessary (gimple *stmt, bool aggressive)
> {
> /* With non-call exceptions, we have to assume that all statements could
> throw. If a statement could throw, it can be deemed necessary. */
> - if (cfun->can_throw_non_call_exceptions
> - && !cfun->can_delete_dead_exceptions
> - && stmt_could_throw_p (cfun, stmt))
> + if (stmt_unremovable_because_of_non_call_eh_p (cfun, stmt))
> {
> mark_stmt_necessary (stmt, true);
> return;
>
--
Richard Biener <rguenther@suse.de>
SUSE Software Solutions Germany GmbH, Maxfeldstrasse 5, 90409 Nuernberg,
Germany; GF: Felix Imendörffer; HRB 36809 (AG Nuernberg)
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 2/4] ipa-sra: Introduce a mini-DCE to tree-inline.c (PR 93385)
2020-05-28 12:06 ` [PATCH 2/4] ipa-sra: Introduce a mini-DCE to tree-inline.c (PR 93385) Martin Jambor
@ 2020-06-02 11:56 ` Richard Biener
2020-06-08 11:39 ` Martin Jambor
0 siblings, 1 reply; 9+ messages in thread
From: Richard Biener @ 2020-06-02 11:56 UTC (permalink / raw)
To: Martin Jambor; +Cc: GCC Patches, Jan Hubicka
On Thu, 28 May 2020, Martin Jambor wrote:
> PR 93385 reveals that if the user explicitely disables DCE, IPA-SRA
> can leave behind statements which are useless because their results
> are eventually not used but can have problematic side effects,
> especially since their inputs are now bogus that useless parameters
> were removed.
>
> This patch fixes the problem by doing a similar def-use walk when
> materializing clones, marking which statements should not be copied
> and which SSA_NAMEs will be removed by call redirections and now need
> to be replaced with anything valid. Default-definition SSA_NAMEs of
> parameters which are removed and all SSA_NAMEs derived from them (in a
> phi node or a simple assignment statement) are then remapped to
> error_mark_node - a sure way to spot it if any is left in place.
>
> There is one exception to the above rule, if such SSA_NAMEs appear as
> an argument of a call, they need to be removed by call redirection and
> not as part of clone materialization. So to have something valid
> there until that time, this patch pulls out dummy declarations out of
> thin air. If you do not like that, see patch number 4 in the series,
> which changes this, but probably in a controversial way.
>
> This patch only resets debug statements using the removed SSA_NAMEs.
> The first follow-up patch adjusts debug statements in the current
> function to still try to make the removed values available in debugger
> in the current function and the subsequent one also in other functions
> where they are passed.
>
> gcc/ChangeLog:
>
> 2020-05-14 Martin Jambor <mjambor@suse.cz>
>
> PR ipa/93385
> * ipa-param-manipulation.h (class ipa_param_body_adjustments): New
> members m_dead_stmts, m_dead_ssas, mark_dead_statements and
> get_removed_call_arg_placeholder.
> * ipa-param-manipulation.c (phi_arg_will_live_p): New function.
> (ipa_param_body_adjustments::mark_dead_statements): New method.
> (ipa_param_body_adjustments::common_initialization): Call it.
> (ipa_param_body_adjustments::ipa_param_body_adjustments): Initialize
> new mwmbers.
> (ipa_param_body_adjustments::get_removed_call_arg_placeholder): New.
> (ipa_param_body_adjustments::modify_call_stmt): Replace dead SSAs
> with dummy decls.
> * tree-inline.c (remap_gimple_stmt): Do not copy dead statements,
> reset dead debug statements.
> (copy_phis_for_bb): Do not copy dead PHI nodes.
>
> gcc/testsuite/ChangeLog:
>
> 2020-05-14 Martin Jambor <mjambor@suse.cz>
>
> PR ipa/93385
> * gcc.dg/ipa/pr93385.c: New test.
> * gcc.dg/ipa/ipa-sra-23.c: Likewise.
> ---
> gcc/ipa-param-manipulation.c | 142 ++++++++++++++++++++++++--
> gcc/ipa-param-manipulation.h | 8 ++
> gcc/testsuite/gcc.dg/ipa/ipa-sra-23.c | 24 +++++
> gcc/testsuite/gcc.dg/ipa/pr93385.c | 27 +++++
> gcc/tree-inline.c | 18 +++-
> 5 files changed, 205 insertions(+), 14 deletions(-)
> create mode 100644 gcc/testsuite/gcc.dg/ipa/ipa-sra-23.c
> create mode 100644 gcc/testsuite/gcc.dg/ipa/pr93385.c
>
> diff --git a/gcc/ipa-param-manipulation.c b/gcc/ipa-param-manipulation.c
> index 978916057f0..1f47f3a4268 100644
> --- a/gcc/ipa-param-manipulation.c
> +++ b/gcc/ipa-param-manipulation.c
> @@ -953,6 +953,99 @@ ipa_param_body_adjustments::carry_over_param (tree t)
> return new_parm;
> }
>
> +/* Return true if BLOCKS_TO_COPY is NULL or if PHI has an argument ARG in
> + position that corresponds to an edge that is coming from a block that has
> + the corresponding bit set in BLOCKS_TO_COPY. */
> +
> +static bool
> +phi_arg_will_live_p (gphi *phi, bitmap blocks_to_copy, tree arg)
> +{
> + bool arg_will_survive = false;
> + if (!blocks_to_copy)
> + arg_will_survive = true;
> + else
> + for (unsigned i = 0; i < gimple_phi_num_args (phi); i++)
> + if (gimple_phi_arg_def (phi, i) == arg
> + && bitmap_bit_p (blocks_to_copy,
> + gimple_phi_arg_edge (phi, i)->src->index))
> + {
> + arg_will_survive = true;
> + break;
> + }
> + return arg_will_survive;
> +}
> +
> +/* Populate m_dead_stmts given that DEAD_PARAM is going to be removed without
> + any replacement or splitting. */
> +
> +void
> +ipa_param_body_adjustments::mark_dead_statements (tree dead_param)
> +{
> + if (!is_gimple_reg (dead_param))
Hmm, so non-registers are not a problem? I guess IPA SRA simply
ensures there are no actual uses (but call arguments) in that case?
Please clearly document this function matches IPA SRA capabilities.
> + return;
> + tree parm_ddef = ssa_default_def (m_id->src_cfun, dead_param);
> + if (!parm_ddef || has_zero_uses (parm_ddef))
> + return;
> +
> + auto_vec<tree, 4> stack;
> + m_dead_ssas.add (parm_ddef);
> + stack.safe_push (parm_ddef);
> + while (!stack.is_empty ())
> + {
> + tree t = stack.pop ();
> +
> + imm_use_iterator imm_iter;
> + gimple *stmt;
> +
> + insert_decl_map (m_id, t, error_mark_node);
> + FOR_EACH_IMM_USE_STMT (stmt, imm_iter, t)
> + {
> + if (is_gimple_call (stmt)
> + || (m_id->blocks_to_copy
> + && !bitmap_bit_p (m_id->blocks_to_copy,
> + gimple_bb (stmt)->index)))
> + continue;
> +
> + if (is_gimple_debug (stmt))
> + {
> + m_dead_stmts.add (stmt);
> + gcc_assert (gimple_debug_bind_p (stmt));
> + }
> + else if (gimple_code (stmt) == GIMPLE_PHI)
> + {
> + gphi *phi = as_a <gphi *> (stmt);
> + if (phi_arg_will_live_p (phi, m_id->blocks_to_copy, t))
> + {
> + m_dead_stmts.add (phi);
> + tree res = gimple_phi_result (phi);
> + if (!m_dead_ssas.contains (res))
You can combine this with the add below which returns false if the
item was not already present.
> + {
> + stack.safe_push (res);
> + m_dead_ssas.add (res);
> + }
> + }
> + }
> + else if (is_gimple_assign (stmt))
> + {
> + m_dead_stmts.add (stmt);
> + if (!gimple_clobber_p (stmt))
> + {
> + tree lhs = gimple_assign_lhs (stmt);
> + gcc_assert (TREE_CODE (lhs) == SSA_NAME);
> + if (!m_dead_ssas.contains (lhs))
> + {
> + m_dead_ssas.add (lhs);
> + stack.safe_push (lhs);
> + }
> + }
> + }
> + else
> + /* IPA-SRA does not analyze other types of statements. */
> + gcc_unreachable ();
> + }
> + }
> +}
> +
> /* Common initialization performed by all ipa_param_body_adjustments
> constructors. OLD_FNDECL is the declaration we take original arguments
> from, (it may be the same as M_FNDECL). VARS, if non-NULL, is a pointer to
> @@ -1095,6 +1188,11 @@ ipa_param_body_adjustments::common_initialization (tree old_fndecl,
> /* Declare this new variable. */
> DECL_CHAIN (var) = *vars;
> *vars = var;
> +
> + /* If this is not a split but a real removal, init hash sets
> + that will guide what not to copy to the new body. */
> + if (!isra_dummy_decls[i])
> + mark_dead_statements (m_oparms[i]);
> }
> }
> else
> @@ -1151,9 +1249,10 @@ ipa_param_body_adjustments
> ::ipa_param_body_adjustments (vec<ipa_adjusted_param, va_gc> *adj_params,
> tree fndecl)
> : m_adj_params (adj_params), m_adjustments (NULL), m_reset_debug_decls (),
> - m_split_modifications_p (false), m_fndecl (fndecl), m_id (NULL),
> - m_oparms (), m_new_decls (), m_new_types (), m_replacements (),
> - m_removed_decls (), m_removed_map (), m_method2func (false)
> + m_split_modifications_p (false), m_dead_stmts (), m_dead_ssas (),
> + m_fndecl (fndecl), m_id (NULL), m_oparms (), m_new_decls (),
> + m_new_types (), m_replacements (), m_removed_decls (), m_removed_map (),
> + m_method2func (false)
> {
> common_initialization (fndecl, NULL, NULL);
> }
> @@ -1167,9 +1266,9 @@ ipa_param_body_adjustments
> ::ipa_param_body_adjustments (ipa_param_adjustments *adjustments,
> tree fndecl)
> : m_adj_params (adjustments->m_adj_params), m_adjustments (adjustments),
> - m_reset_debug_decls (), m_split_modifications_p (false), m_fndecl (fndecl),
> - m_id (NULL), m_oparms (), m_new_decls (), m_new_types (),
> - m_replacements (), m_removed_decls (), m_removed_map (),
> + m_reset_debug_decls (), m_split_modifications_p (false), m_dead_stmts (),
> + m_dead_ssas (), m_fndecl (fndecl), m_id (NULL), m_oparms (), m_new_decls (),
> + m_new_types (), m_replacements (), m_removed_decls (), m_removed_map (),
> m_method2func (false)
> {
> common_initialization (fndecl, NULL, NULL);
> @@ -1190,9 +1289,10 @@ ipa_param_body_adjustments
> copy_body_data *id, tree *vars,
> vec<ipa_replace_map *, va_gc> *tree_map)
> : m_adj_params (adjustments->m_adj_params), m_adjustments (adjustments),
> - m_reset_debug_decls (), m_split_modifications_p (false), m_fndecl (fndecl),
> - m_id (id), m_oparms (), m_new_decls (), m_new_types (), m_replacements (),
> - m_removed_decls (), m_removed_map (), m_method2func (false)
> + m_reset_debug_decls (), m_split_modifications_p (false), m_dead_stmts (),
> + m_dead_ssas (),m_fndecl (fndecl), m_id (id), m_oparms (), m_new_decls (),
> + m_new_types (), m_replacements (), m_removed_decls (), m_removed_map (),
> + m_method2func (false)
> {
> common_initialization (old_fndecl, vars, tree_map);
> }
> @@ -1519,6 +1619,19 @@ remap_split_decl_to_dummy (tree *tp, int *walk_subtrees, void *data)
> return NULL_TREE;
> }
>
> +/* 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. */
> +
> +tree
> +ipa_param_body_adjustments::get_removed_call_arg_placeholder (tree arg)
> +{
> + tree t = create_tmp_var (TREE_TYPE (arg));
If it is temporarily then use _raw. It looks like you can get called
multiple times for the same arg and each time you get a new temporary
decl? Ugh.
> + insert_decl_map (m_id, t, t);
I wonder why you can't use/keep the SSA default def of the removed
parameter instead? Or a literal zero? That is, below you don't
seem to be anything special during inlining itself so I presume
those are all removed by call redirection? Why are the actual
parameters then still needed? (and error_mark_node doesn't work?)
> + return t;
> +}
>
> /* If the call statement pointed at by STMT_P contains any expressions that
> need to replaced with a different one as noted by ADJUSTMENTS, do so. f the
> @@ -1658,7 +1771,10 @@ ipa_param_body_adjustments::modify_call_stmt (gcall **stmt_p)
> else
> {
> tree t = gimple_call_arg (stmt, i);
> - modify_expression (&t, true);
> + if (TREE_CODE (t) == SSA_NAME && m_dead_ssas.contains(t))
space after function call (likewise elsewhere).
> + t = get_removed_call_arg_placeholder (t);
> + else
> + modify_expression (&t, true);
> vargs.safe_push (t);
> }
> }
> @@ -1680,7 +1796,11 @@ ipa_param_body_adjustments::modify_call_stmt (gcall **stmt_p)
> for (unsigned i = 0; i < nargs; i++)
> {
> tree *t = gimple_call_arg_ptr (stmt, i);
> - modified |= modify_expression (t, true);
> +
> + if (TREE_CODE (*t) == SSA_NAME && m_dead_ssas.contains(*t))
> + *t = get_removed_call_arg_placeholder (*t);
> + else
> + modified |= modify_expression (t, true);
> }
>
> if (gimple_call_lhs (stmt))
> diff --git a/gcc/ipa-param-manipulation.h b/gcc/ipa-param-manipulation.h
> index 0b038ea57f1..59060ae5dcc 100644
> --- a/gcc/ipa-param-manipulation.h
> +++ b/gcc/ipa-param-manipulation.h
> @@ -370,6 +370,12 @@ public:
> /* Set to true if there are any IPA_PARAM_OP_SPLIT adjustments among stored
> adjustments. */
> bool m_split_modifications_p;
> +
> + /* Sets of statements and SSA_NAMEs that only manipulate data from parameters
> + removed because they are not necessary. */
> + hash_set<gimple *> m_dead_stmts;
> + hash_set<tree> m_dead_ssas;
> +
> private:
> void common_initialization (tree old_fndecl, tree *vars,
> vec<ipa_replace_map *, va_gc> *tree_map);
> @@ -383,6 +389,8 @@ private:
> bool modify_call_stmt (gcall **stmt_p);
> bool modify_cfun_body ();
> void reset_debug_stmts ();
> + void mark_dead_statements (tree dead_param);
> + tree get_removed_call_arg_placeholder (tree arg);
>
> /* Declaration of the function that is being transformed. */
>
> diff --git a/gcc/testsuite/gcc.dg/ipa/ipa-sra-23.c b/gcc/testsuite/gcc.dg/ipa/ipa-sra-23.c
> new file mode 100644
> index 00000000000..f438b509614
> --- /dev/null
> +++ b/gcc/testsuite/gcc.dg/ipa/ipa-sra-23.c
> @@ -0,0 +1,24 @@
> +/* { dg-do compile } */
> +/* { dg-options "-O2" } */
> +
> +extern int g;
> +
> +static int __attribute__((noinline))
> +bar (int i, int j)
> +{
> + return 2*g + i;
> +}
> +
> +static int __attribute__((noinline))
> +foo (int i, int j)
> +{
> + if (i > 5)
> + j = 22;
> + return bar (i, j) + 1;
> +}
> +
> +int
> +entry (int l, int k)
> +{
> + return foo (l, k);
> +}
> diff --git a/gcc/testsuite/gcc.dg/ipa/pr93385.c b/gcc/testsuite/gcc.dg/ipa/pr93385.c
> new file mode 100644
> index 00000000000..6d1d0d7cd27
> --- /dev/null
> +++ b/gcc/testsuite/gcc.dg/ipa/pr93385.c
> @@ -0,0 +1,27 @@
> +/* { dg-do run } */
> +/* { dg-options "-O2 -fno-dce -fno-ipa-cp -fno-tree-dce" } */
> +
> +char a, b;
> +
> +#ifdef __SIZEOF_INT128__
> +#define T unsigned __int128
> +#else
> +#define T unsigned
> +#endif
> +
> +static inline int
> +c (T d)
> +{
> + char e = 0;
> + d %= (unsigned) d;
> + e -= 0;
> + __builtin_strncpy (&a, &e, 1);
> + return e + b;
> +}
> +
> +int
> +main (void)
> +{
> + c (~0);
> + return 0;
> +}
> diff --git a/gcc/tree-inline.c b/gcc/tree-inline.c
> index 3160ca3f88a..60087dd5e7b 100644
> --- a/gcc/tree-inline.c
> +++ b/gcc/tree-inline.c
> @@ -1524,6 +1524,11 @@ remap_gimple_stmt (gimple *stmt, copy_body_data *id)
> : !opt_for_fn (id->dst_fn, flag_var_tracking_assignments)))
> return NULL;
>
> + if (!is_gimple_debug (stmt)
> + && id->param_body_adjs
> + && id->param_body_adjs->m_dead_stmts.contains (stmt))
> + return NULL;
> +
> /* Begin by recognizing trees that we'll completely rewrite for the
> inlining context. Our output for these trees is completely
> different from our input (e.g. RETURN_EXPR is deleted and morphs
> @@ -1788,10 +1793,15 @@ remap_gimple_stmt (gimple *stmt, copy_body_data *id)
>
> if (gimple_debug_bind_p (stmt))
> {
> + tree value;
> + if (id->param_body_adjs
> + && id->param_body_adjs->m_dead_stmts.contains (stmt))
> + value = NULL_TREE;
> + else
> + value = gimple_debug_bind_get_value (stmt);
> gdebug *copy
> = gimple_build_debug_bind (gimple_debug_bind_get_var (stmt),
> - gimple_debug_bind_get_value (stmt),
> - stmt);
> + value, stmt);
> if (id->reset_location)
> gimple_set_location (copy, input_location);
> id->debug_stmts.safe_push (copy);
> @@ -2671,7 +2681,9 @@ copy_phis_for_bb (basic_block bb, copy_body_data *id)
> phi = si.phi ();
> res = PHI_RESULT (phi);
> new_res = res;
> - if (!virtual_operand_p (res))
> + if (!virtual_operand_p (res)
> + && (!id->param_body_adjs
> + || !id->param_body_adjs->m_dead_stmts.contains (phi)))
> {
> walk_tree (&new_res, copy_tree_body_r, id, NULL);
> if (EDGE_COUNT (new_bb->preds) == 0)
>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 2/4] ipa-sra: Introduce a mini-DCE to tree-inline.c (PR 93385)
2020-06-02 11:56 ` Richard Biener
@ 2020-06-08 11:39 ` Martin Jambor
0 siblings, 0 replies; 9+ messages in thread
From: Martin Jambor @ 2020-06-08 11:39 UTC (permalink / raw)
To: Richard Biener; +Cc: GCC Patches, Jan Hubicka
Hi,
On Tue, Jun 02 2020, Richard Biener wrote:
> On Thu, 28 May 2020, Martin Jambor wrote:
>
>> PR 93385 reveals that if the user explicitely disables DCE, IPA-SRA
>> can leave behind statements which are useless because their results
>> are eventually not used but can have problematic side effects,
>> especially since their inputs are now bogus that useless parameters
>> were removed.
>>
>> This patch fixes the problem by doing a similar def-use walk when
>> materializing clones, marking which statements should not be copied
>> and which SSA_NAMEs will be removed by call redirections and now need
>> to be replaced with anything valid. Default-definition SSA_NAMEs of
>> parameters which are removed and all SSA_NAMEs derived from them (in a
>> phi node or a simple assignment statement) are then remapped to
>> error_mark_node - a sure way to spot it if any is left in place.
>>
>> There is one exception to the above rule, if such SSA_NAMEs appear as
>> an argument of a call, they need to be removed by call redirection and
>> not as part of clone materialization. So to have something valid
>> there until that time, this patch pulls out dummy declarations out of
>> thin air. If you do not like that, see patch number 4 in the series,
>> which changes this, but probably in a controversial way.
>>
>> This patch only resets debug statements using the removed SSA_NAMEs.
>> The first follow-up patch adjusts debug statements in the current
>> function to still try to make the removed values available in debugger
>> in the current function and the subsequent one also in other functions
>> where they are passed.
>>
>> gcc/ChangeLog:
>>
>> 2020-05-14 Martin Jambor <mjambor@suse.cz>
>>
>> PR ipa/93385
>> * ipa-param-manipulation.h (class ipa_param_body_adjustments): New
>> members m_dead_stmts, m_dead_ssas, mark_dead_statements and
>> get_removed_call_arg_placeholder.
>> * ipa-param-manipulation.c (phi_arg_will_live_p): New function.
>> (ipa_param_body_adjustments::mark_dead_statements): New method.
>> (ipa_param_body_adjustments::common_initialization): Call it.
>> (ipa_param_body_adjustments::ipa_param_body_adjustments): Initialize
>> new mwmbers.
>> (ipa_param_body_adjustments::get_removed_call_arg_placeholder): New.
>> (ipa_param_body_adjustments::modify_call_stmt): Replace dead SSAs
>> with dummy decls.
>> * tree-inline.c (remap_gimple_stmt): Do not copy dead statements,
>> reset dead debug statements.
>> (copy_phis_for_bb): Do not copy dead PHI nodes.
>>
>> gcc/testsuite/ChangeLog:
>>
[...]
>>
>> diff --git a/gcc/ipa-param-manipulation.c b/gcc/ipa-param-manipulation.c
>> index 978916057f0..1f47f3a4268 100644
>> --- a/gcc/ipa-param-manipulation.c
>> +++ b/gcc/ipa-param-manipulation.c
>> @@ -953,6 +953,99 @@ ipa_param_body_adjustments::carry_over_param (tree t)
>> return new_parm;
>> }
>>
>> +/* Return true if BLOCKS_TO_COPY is NULL or if PHI has an argument ARG in
>> + position that corresponds to an edge that is coming from a block that has
>> + the corresponding bit set in BLOCKS_TO_COPY. */
>> +
>> +static bool
>> +phi_arg_will_live_p (gphi *phi, bitmap blocks_to_copy, tree arg)
>> +{
>> + bool arg_will_survive = false;
>> + if (!blocks_to_copy)
>> + arg_will_survive = true;
>> + else
>> + for (unsigned i = 0; i < gimple_phi_num_args (phi); i++)
>> + if (gimple_phi_arg_def (phi, i) == arg
>> + && bitmap_bit_p (blocks_to_copy,
>> + gimple_phi_arg_edge (phi, i)->src->index))
>> + {
>> + arg_will_survive = true;
>> + break;
>> + }
>> + return arg_will_survive;
>> +}
>> +
>> +/* Populate m_dead_stmts given that DEAD_PARAM is going to be removed without
>> + any replacement or splitting. */
>> +
>> +void
>> +ipa_param_body_adjustments::mark_dead_statements (tree dead_param)
>> +{
>> + if (!is_gimple_reg (dead_param))
>
> Hmm, so non-registers are not a problem? I guess IPA SRA simply
> ensures there are no actual uses (but call arguments) in that case?
> Please clearly document this function matches IPA SRA capabilities.
OK, added.
>
>> + return;
>> + tree parm_ddef = ssa_default_def (m_id->src_cfun, dead_param);
>> + if (!parm_ddef || has_zero_uses (parm_ddef))
>> + return;
>> +
>> + auto_vec<tree, 4> stack;
>> + m_dead_ssas.add (parm_ddef);
>> + stack.safe_push (parm_ddef);
>> + while (!stack.is_empty ())
>> + {
>> + tree t = stack.pop ();
>> +
>> + imm_use_iterator imm_iter;
>> + gimple *stmt;
>> +
>> + insert_decl_map (m_id, t, error_mark_node);
>> + FOR_EACH_IMM_USE_STMT (stmt, imm_iter, t)
>> + {
>> + if (is_gimple_call (stmt)
>> + || (m_id->blocks_to_copy
>> + && !bitmap_bit_p (m_id->blocks_to_copy,
>> + gimple_bb (stmt)->index)))
>> + continue;
>> +
>> + if (is_gimple_debug (stmt))
>> + {
>> + m_dead_stmts.add (stmt);
>> + gcc_assert (gimple_debug_bind_p (stmt));
>> + }
>> + else if (gimple_code (stmt) == GIMPLE_PHI)
>> + {
>> + gphi *phi = as_a <gphi *> (stmt);
>> + if (phi_arg_will_live_p (phi, m_id->blocks_to_copy, t))
>> + {
>> + m_dead_stmts.add (phi);
>> + tree res = gimple_phi_result (phi);
>> + if (!m_dead_ssas.contains (res))
>
> You can combine this with the add below which returns false if the
> item was not already present.
I see, done.
>
>> + {
>> + stack.safe_push (res);
>> + m_dead_ssas.add (res);
>> + }
>> + }
>> + }
[...]
>> @@ -1519,6 +1619,19 @@ remap_split_decl_to_dummy (tree *tp, int *walk_subtrees, void *data)
>> return NULL_TREE;
>> }
>>
>> +/* 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. */
>> +
>> +tree
>> +ipa_param_body_adjustments::get_removed_call_arg_placeholder (tree arg)
>> +{
>> + tree t = create_tmp_var (TREE_TYPE (arg));
>
> If it is temporarily then use _raw. It looks like you can get called
> multiple times for the same arg and each time you get a new temporary
> decl? Ugh.
>
>> + insert_decl_map (m_id, t, t);
>
> I wonder why you can't use/keep the SSA default def of the removed
> parameter instead? Or a literal zero? That is, below you don't
> seem to be anything special during inlining itself so I presume
> those are all removed by call redirection? Why are the actual
> parameters then still needed? (and error_mark_node doesn't work?)
Please see the last patch in the series, which removes the ugly decl and
puts here - for a defined time period - either error_mark_node (with
-g0) or DEBUG_EXPR_DECL (when generating debug info). But I expect that
patch to be at least slightly controversial because it basically extends
gimple during IPA passes.
I tried but cannot cannot use literal zero here because that then leaks
into debug information and debugger thinks the parameter really was
zero.
So if the last patch in the series is not acceptable, I can see two
options:
1) We can decide we will simply never attempt to generate debug info for
such removed parameters and then we can either have one such dummy
decl per function (type mismatch hopefully would not matter) or
remove the parameters outright and mark them as already removed in
some bitmap on associated cgraph_edges.
2) Or we decide we do want to attempt generating debug info about the
value of the removed parameter but not extend IL during IPA passes
and then my plan was to create a unique dummy decl here and create a
mapping between this decl and the DEBUG_EXPR_DECL containing debug
info - and keep it up to date for the rest of IPA machinery. But the
code keeping things up to date when the created clone is itself
cloned or inlined quickly got so ugly I decided to try and propose
the IL extension where all of this happens automatically (as the
testcase in the patch shows).
>
>> + return t;
>> +}
>>
>> /* If the call statement pointed at by STMT_P contains any expressions that
>> need to replaced with a different one as noted by ADJUSTMENTS, do so. f the
>> @@ -1658,7 +1771,10 @@ ipa_param_body_adjustments::modify_call_stmt (gcall **stmt_p)
>> else
>> {
>> tree t = gimple_call_arg (stmt, i);
>> - modify_expression (&t, true);
>> + if (TREE_CODE (t) == SSA_NAME && m_dead_ssas.contains(t))
>
> space after function call (likewise elsewhere).
Oops, fixed.
Thanks,
Martin
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 4/4] ipa-sra: Fix debug info for removed args passed to other functions (PR 93385, 95343)
2020-05-28 12:06 ` [PATCH 4/4] ipa-sra: Fix debug info for removed args passed to other functions (PR 93385, 95343) Martin Jambor
@ 2020-06-08 11:40 ` Martin Jambor
0 siblings, 0 replies; 9+ messages in thread
From: Martin Jambor @ 2020-06-08 11:40 UTC (permalink / raw)
To: GCC Patches; +Cc: Richard Biener, Jan Hubicka
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
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2020-06-08 11:40 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-05-28 12:06 [PATCH 0/4] Make IPA-SRA not depend on tree-dce and related fixes Martin Jambor
2020-05-28 12:06 ` [PATCH 1/4] ipa-sra: Do not remove statements necessary because of non-call EH (PR 95113) Martin Jambor
2020-06-02 11:42 ` Richard Biener
2020-05-28 12:06 ` [PATCH 2/4] ipa-sra: Introduce a mini-DCE to tree-inline.c (PR 93385) Martin Jambor
2020-06-02 11:56 ` Richard Biener
2020-06-08 11:39 ` Martin Jambor
2020-05-28 12:06 ` [PATCH 3/4] ipa-sra: Improve debug info for removed parameters " Martin Jambor
2020-05-28 12:06 ` [PATCH 4/4] ipa-sra: Fix debug info for removed args passed to other functions (PR 93385, 95343) Martin Jambor
2020-06-08 11:40 ` Martin Jambor
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).