The PR showed two issues with pair-fusion. The first is that the pass
treated stack pointer deallocations as ordinary register updates, and so
might move them earlier than another stack access (through a different
base register) that doesn't alias the pair candidate.
The simplest fix for that seems to be to prevent the stack deallocation
from being moved. This part might (or might not) be a latent source of
wrong code and so worth backporting in some form. (The patch as-is
won't work for GCC 14.)
The second issue only started with r15-6551, which added a memory
write to stack allocations and deallocations. We should use the
existing tombstone mechanism to preserve the associated memory
definition. (Deleting definitions immediately would have quadratic
complexity in the worst case.)
Tested on aarch64-linux-gnu. OK to install?
Richard
gcc/
PR rtl-optimization/118429
* pair-fusion.cc (latest_hazard_before): Add an extra parameter
to say whether the instruction is a load or a store. If the
instruction is not a load or store and has memory side effects,
prevent it from being moved earlier.
(pair_fusion::find_trailing_add): Update call accordingly.
(pair_fusion_bb_info::fuse_pair): If the trailng addition had
a memory side-effect, use a tombstone to preserve it.
gcc/testsuite/
PR rtl-optimization/118429
* gcc.c-torture/compile/pr118429.c: New test.
---
gcc/pair-fusion.cc | 45 ++++++++++++++-----
.../gcc.c-torture/compile/pr118429.c | 7 +++
2 files changed, 40 insertions(+), 12 deletions(-)
create mode 100644 gcc/testsuite/gcc.c-torture/compile/pr118429.c
diff --git a/gcc/pair-fusion.cc b/gcc/pair-fusion.cc
index b6643ca4812..602e572ab6c 100644
--- a/gcc/pair-fusion.cc
+++ b/gcc/pair-fusion.cc
@@ -573,11 +573,13 @@ pair_fusion_bb_info::track_access (insn_info *insn, bool
load_p, rtx mem)
// If IGNORE_INSN is non-NULL, we should further ignore any hazards arising
// from that insn.
//
-// N.B. we ignore any defs/uses of memory here as we deal with that separately,
-// making use of alias disambiguation.
+// IS_LOAD_STORE is true if INSN is one of the loads or stores in the
+// candidate pair. We ignore any defs/uses of memory in such instructions
+// as we deal with that separately, making use of alias disambiguation.
static insn_info *
latest_hazard_before (insn_info *insn, rtx *ignore,
- insn_info *ignore_insn = nullptr)
+ insn_info *ignore_insn = nullptr,
+ bool is_load_store = true)
{
insn_info *result = nullptr;
@@ -588,6 +590,10 @@ latest_hazard_before (insn_info *insn, rtx *ignore,
&& find_reg_note (insn->rtl (), REG_EH_REGION, NULL_RTX))
return insn->prev_nondebug_insn ();
+ if (!is_load_store
+ && accesses_include_memory (insn->defs ()))
+ return insn->prev_nondebug_insn ();
+
// Return true if we registered the hazard.
auto hazard = [&](insn_info *h) -> bool
{
@@ -1238,7 +1244,7 @@ pair_fusion::find_trailing_add (insn_info *insns[2],
insns[0]->uid (), insns[1]->uid ());
};
- insn_info *hazard = latest_hazard_before (cand, nullptr, insns[1]);
+ insn_info *hazard = latest_hazard_before (cand, nullptr, insns[1], false);
if (!hazard || *hazard <= *pair_dst)
{
if (dump_file)
@@ -1633,7 +1639,7 @@ pair_fusion_bb_info::fuse_pair (bool load_p,
insn_info *insns[2] = { first, second };
auto_vec<insn_change *> changes;
- auto_vec<int, 2> tombstone_uids (2);
+ auto_vec<int, 3> tombstone_uids;
rtx pats[2] = {
PATTERN (first->rtl ()),
@@ -1824,6 +1830,16 @@ pair_fusion_bb_info::fuse_pair (bool load_p,
validate_change (rti, ®_NOTES (rti), reg_notes, true);
};
+ // Turn CHANGE into a memory definition tombstone.
+ auto make_tombstone = [&](insn_change *change)
+ {
+ tombstone_uids.quick_push (change->insn ()->uid ());
+ rtx_insn *rti = change->insn ()->rtl ();
+ validate_change (rti, &PATTERN (rti), gen_tombstone (), true);
+ validate_change (rti, ®_NOTES (rti), NULL_RTX, true);
+ change->new_uses = use_array ();
+ };
+
if (load_p)
{
changes.safe_push (make_delete (first));
@@ -1879,11 +1895,7 @@ pair_fusion_bb_info::fuse_pair (bool load_p,
}
case Action::TOMBSTONE:
{
- tombstone_uids.quick_push (change->insn ()->uid ());
- rtx_insn *rti = change->insn ()->rtl ();
- validate_change (rti, &PATTERN (rti), gen_tombstone (), true);
- validate_change (rti, ®_NOTES (rti), NULL_RTX, true);
- change->new_uses = use_array (nullptr, 0);
+ make_tombstone (change);
break;
}
case Action::INSERT:
@@ -1934,7 +1946,17 @@ pair_fusion_bb_info::fuse_pair (bool load_p,
}
if (trailing_add)
- changes.safe_push (make_delete (trailing_add));
+ {
+ if (auto *mem_def = memory_access (trailing_add->defs()))
+ {
+ auto *change = make_change (trailing_add);
+ change->new_defs = insert_access (attempt, mem_def, def_array ());
+ make_tombstone (change);
+ changes.safe_push (change);
+ }
+ else
+ changes.safe_push (make_delete (trailing_add));
+ }
else if ((writeback & 2) && !writeback_effect)
{
// The second insn initially had writeback but now the pair does not,
@@ -1991,7 +2013,6 @@ pair_fusion_bb_info::fuse_pair (bool load_p,
confirm_change_group ();
crtl->ssa->change_insns (changes);
- gcc_checking_assert (tombstone_uids.length () <= 2);
for (auto uid : tombstone_uids)
track_tombstone (uid);
diff --git a/gcc/testsuite/gcc.c-torture/compile/pr118429.c
b/gcc/testsuite/gcc.c-torture/compile/pr118429.c
new file mode 100644
index 00000000000..da7fcf9362d
--- /dev/null
+++ b/gcc/testsuite/gcc.c-torture/compile/pr118429.c
@@ -0,0 +1,7 @@
+void h(long, long, long, long, long, long, long, long, long, long);
+long i(long, long, long, long, long, long, long, long, long, long);
+void f(long a0, long a1, long a2, long a3, long a4, long a5,
+ long a6, long a7, long a8, long a9) {
+ i(a0, a1, a2, a3, a4, a5, a6, a7, a8, a9);
+ h(a0, 1, 1, 1, 1, 1, 1, 1, 0, 0);
+}
--
2.25.1
