Hello,
On Wed, Aug 02 2023, Richard Biener wrote:
> On Mon, Jul 31, 2023 at 7:05 PM Martin Jambor <mjam...@suse.cz> wrote:
>>
>> Hi,
>>
>> when IPA-SRA detects whether a parameter passed by reference is
>> written to, it does not special case CLOBBERs which means it often
>> bails out unnecessarily, especially when dealing with C++ destructors.
>> Fixed by the obvious continue in the two relevant loops.
>>
>> The (slightly) more complex testcases in the PR need surprisingly more
>> effort but the simple one can be fixed now easily by this patch and I'll
>> work on the others incrementally.
>>
>> Bootstrapped and currently undergoing testsuite run on x86_64-linux. OK
>> if it passes too?
>
> LGTM, btw - how are the clobbers handled during transform?
it turns out your question is spot on. I assumed that the mini-DCE that
I implemented into IPA-SRA transform would delete but I had a closer
look and it is not invoked on split parameters,only on removed ones.
What was actually happening is that the parameter got remapped to a
default definition of a replacement VAR_DECL and we were thus
gimple-clobbering a pointer pointing to nowhere. The clobber then got
DSEd and so I originally did not notice looking at the optimized dump.
Still that is of course not ideal and so I added a simple function
removing clobbers when splitting. I as considering adding that
functionality to ipa_param_body_adjustments::mark_dead_statements but
that would make the function harder to read without much gain.
So thanks again for the remark. The following passes bootstrap and
testing on x86_64-linux. I am running LTO bootstrap now. OK if it
passes?
Martin
When IPA-SRA detects whether a parameter passed by reference is
written to, it does not special case CLOBBERs which means it often
bails out unnecessarily, especially when dealing with C++ destructors.
Fixed by the obvious continue in the two relevant loops and by adding
a simple function that marks the clobbers in the transformation code
as statements to be removed.
gcc/ChangeLog:
2023-08-04 Martin Jambor <mjam...@suse.cz>
PR ipa/110378
* ipa-param-manipulation.h (class ipa_param_body_adjustments): New
members get_ddef_if_exists_and_is_used and mark_clobbers_dead.
* ipa-sra.cc (isra_track_scalar_value_uses): Ignore clobbers.
(ptr_parm_has_nonarg_uses): Likewise.
* ipa-param-manipulation.cc
(ipa_param_body_adjustments::get_ddef_if_exists_and_is_used): New.
(ipa_param_body_adjustments::mark_dead_statements): Move initial
checks to get_ddef_if_exists_and_is_used.
(ipa_param_body_adjustments::mark_clobbers_dead): New.
(ipa_param_body_adjustments::common_initialization): Call
mark_clobbers_dead when splitting.
gcc/testsuite/ChangeLog:
2023-07-31 Martin Jambor <mjam...@suse.cz>
PR ipa/110378
* g++.dg/ipa/pr110378-1.C: New test.
---
gcc/ipa-param-manipulation.cc | 44 +++++++++++++++++++++---
gcc/ipa-param-manipulation.h | 2 ++
gcc/ipa-sra.cc | 6 ++--
gcc/testsuite/g++.dg/ipa/pr110378-1.C | 48 +++++++++++++++++++++++++++
4 files changed, 94 insertions(+), 6 deletions(-)
create mode 100644 gcc/testsuite/g++.dg/ipa/pr110378-1.C
diff --git a/gcc/ipa-param-manipulation.cc b/gcc/ipa-param-manipulation.cc
index a286af7f5d9..4a185ddbdf4 100644
--- a/gcc/ipa-param-manipulation.cc
+++ b/gcc/ipa-param-manipulation.cc
@@ -1072,6 +1072,20 @@ ipa_param_body_adjustments::carry_over_param (tree t)
return new_parm;
}
+/* If DECL is a gimple register that has a default definition SSA name and that
+ has some uses, return the default definition, otherwise return NULL_TREE.
*/
+
+tree
+ipa_param_body_adjustments::get_ddef_if_exists_and_is_used (tree decl)
+{
+ if (!is_gimple_reg (decl))
+ return NULL_TREE;
+ tree ddef = ssa_default_def (m_id->src_cfun, decl);
+ if (!ddef || has_zero_uses (ddef))
+ return NULL_TREE;
+ return ddef;
+}
+
/* Populate m_dead_stmts given that DEAD_PARAM is going to be removed without
any replacement or splitting. REPL is the replacement VAR_SECL to base any
remaining uses of a removed parameter on. Push all removed SSA names that
@@ -1084,10 +1098,8 @@ ipa_param_body_adjustments::mark_dead_statements (tree
dead_param,
/* Current IPA analyses which remove unused parameters never remove a
non-gimple register ones which have any use except as parameters in other
calls, so we can safely leve them as they are. */
- 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))
+ tree parm_ddef = get_ddef_if_exists_and_is_used (dead_param);
+ if (!parm_ddef)
return;
auto_vec<tree, 4> stack;
@@ -1169,6 +1181,28 @@ ipa_param_body_adjustments::mark_dead_statements (tree
dead_param,
m_dead_ssa_debug_equiv.put (parm_ddef, dp_ddecl);
}
+/* Put all clobbers of of dereference of default definition of PARAM into
+ m_dead_stmts. */
+
+void
+ipa_param_body_adjustments::mark_clobbers_dead (tree param)
+{
+ if (!is_gimple_reg (param))
+ return;
+ tree ddef = get_ddef_if_exists_and_is_used (param);
+ if (!ddef)
+ return;
+
+ imm_use_iterator imm_iter;
+ use_operand_p use_p;
+ FOR_EACH_IMM_USE_FAST (use_p, imm_iter, ddef)
+ {
+ gimple *stmt = USE_STMT (use_p);
+ if (gimple_clobber_p (stmt))
+ m_dead_stmts.add (stmt);
+ }
+}
+
/* 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.
If an SSA argument would be remapped to NULL, the whole operation needs to
@@ -1504,6 +1538,8 @@ ipa_param_body_adjustments::common_initialization (tree
old_fndecl,
that will guide what not to copy to the new body. */
if (!split[i])
mark_dead_statements (m_oparms[i], &ssas_to_process_debug);
+ else
+ mark_clobbers_dead (m_oparms[i]);
if (MAY_HAVE_DEBUG_STMTS
&& is_gimple_reg (m_oparms[i]))
m_reset_debug_decls.safe_push (m_oparms[i]);
diff --git a/gcc/ipa-param-manipulation.h b/gcc/ipa-param-manipulation.h
index 9798eedf0b6..d6a26e9ad36 100644
--- a/gcc/ipa-param-manipulation.h
+++ b/gcc/ipa-param-manipulation.h
@@ -378,7 +378,9 @@ private:
bool modify_call_stmt (gcall **stmt_p, gimple *orig_stmt);
bool modify_cfun_body ();
void reset_debug_stmts ();
+ tree get_ddef_if_exists_and_is_used (tree decl);
void mark_dead_statements (tree dead_param, vec<tree> *debugstack);
+ void mark_clobbers_dead (tree dead_param);
bool prepare_debug_expressions (tree dead_ssa);
/* Declaration of the function that is being transformed. */
diff --git a/gcc/ipa-sra.cc b/gcc/ipa-sra.cc
index c35e03b7abd..edba364f56e 100644
--- a/gcc/ipa-sra.cc
+++ b/gcc/ipa-sra.cc
@@ -898,7 +898,8 @@ isra_track_scalar_value_uses (function *fun, cgraph_node
*node, tree name,
FOR_EACH_IMM_USE_STMT (stmt, imm_iter, name)
{
- if (is_gimple_debug (stmt))
+ if (is_gimple_debug (stmt)
+ || gimple_clobber_p (stmt))
continue;
/* TODO: We could handle at least const builtin functions like arithmetic
@@ -1056,7 +1057,8 @@ ptr_parm_has_nonarg_uses (cgraph_node *node, function
*fun, tree parm,
unsigned uses_ok = 0;
use_operand_p use_p;
- if (is_gimple_debug (stmt))
+ if (is_gimple_debug (stmt)
+ || gimple_clobber_p (stmt))
continue;
if (gimple_assign_single_p (stmt))
diff --git a/gcc/testsuite/g++.dg/ipa/pr110378-1.C
b/gcc/testsuite/g++.dg/ipa/pr110378-1.C
new file mode 100644
index 00000000000..fda7699795a
--- /dev/null
+++ b/gcc/testsuite/g++.dg/ipa/pr110378-1.C
@@ -0,0 +1,48 @@
+/* { dg-do compile } */
+/* { dg-options "-O2 -fdump-ipa-sra -fdump-tree-optimized-slim" } */
+
+/* Test that even though destructors end with clobbering all of *this, it
+ should not prevent IPA-SRA. */
+
+namespace {
+
+ class foo
+ {
+ public:
+ short move_offset_of_a;
+ int *a;
+ foo(int c)
+ {
+ a = new int[c];
+ a[0] = 4;
+ }
+ __attribute__((noinline)) ~foo();
+ int f ()
+ {
+ return a[0] + 1;
+ }
+ };
+
+ volatile int v1 = 4;
+
+ __attribute__((noinline)) foo::~foo()
+ {
+ delete[] a;
+ return;
+ }
+
+
+}
+
+volatile int v2 = 20;
+
+int test (void)
+{
+ foo shouldnotexist(v2);
+ v2 = shouldnotexist.f();
+ return 0;
+}
+
+
+/* { dg-final { scan-ipa-dump "Will split parameter 0" "sra" } } */
+/* { dg-final { scan-tree-dump-not "shouldnotexist" "optimized" } } */
--
2.41.0
