While scanning the instructions and upon reaching an instruction that
doesn't satisfy the constraints that we have set, we were removing the
already detected stores, but we were continuing adding stores from that
point onward. This was causing issues when the address ranges from later
stores overlapped with the load's address, leading to partial and wrong
update of the register containing the loaded value.
With this patch, we are skipping the tranformation for stores that operate
on the load's address range, when stores that operate on the same range
have been deleted due to constraint violations.
PR rtl-optimization/119795
gcc/ChangeLog:
* avoid-store-forwarding.cc
(store_forwarding_analyzer::avoid_store_forwarding): Skip
transformations for stores that operate on the same address
range as deleted ones.
gcc/testsuite/ChangeLog:
* gcc.target/i386/pr119795.c: New test.
---
gcc/avoid-store-forwarding.cc | 64 +++++++++++++++++++++---
gcc/testsuite/gcc.target/i386/pr119795.c | 26 ++++++++++
2 files changed, 82 insertions(+), 8 deletions(-)
create mode 100644 gcc/testsuite/gcc.target/i386/pr119795.c
diff --git a/gcc/avoid-store-forwarding.cc b/gcc/avoid-store-forwarding.cc
index 785efd22606f..5d80eb1d428c 100644
--- a/gcc/avoid-store-forwarding.cc
+++ b/gcc/avoid-store-forwarding.cc
@@ -437,9 +437,22 @@ store_forwarding_analyzer::avoid_store_forwarding
(basic_block bb)
return;
auto_vec<store_fwd_info, 8> store_exprs;
+ auto_vec<rtx> store_exprs_del;
rtx_insn *insn;
unsigned int insn_cnt = 0;
+ /* We are iterating over the basic block's instructions detecting store
+ instructions. Upon reaching a load instruction, we check if any of the
+ previously detected stores could result in store forwarding. In that
+ case, we try to reorder the load and store instructions.
+ We skip this transformation when we encounter complex memory operations,
+ instructions that might throw an exception, instruction dependencies,
+ etc. This is done by clearing the vector of detected stores, while
+ keeping the removed stores in another vector. By doing so, we can check
+ if any of the removed stores operated on the load's address range, when
+ reaching a subsequent store that operates on the same address range,
+ as this would lead to incorrect values on the register that keeps the
+ loaded value. */
FOR_BB_INSNS (bb, insn)
{
if (!NONDEBUG_INSN_P (insn))
@@ -452,6 +465,10 @@ store_forwarding_analyzer::avoid_store_forwarding
(basic_block bb)
if (!set || insn_could_throw_p (insn))
{
+ unsigned int i;
+ store_fwd_info *it;
+ FOR_EACH_VEC_ELT (store_exprs, i, it)
+ store_exprs_del.safe_push (it->store_mem);
store_exprs.truncate (0);
continue;
}
@@ -475,6 +492,10 @@ store_forwarding_analyzer::avoid_store_forwarding
(basic_block bb)
|| (load_mem && (!MEM_SIZE_KNOWN_P (load_mem)
|| !MEM_SIZE (load_mem).is_constant ())))
{
+ unsigned int i;
+ store_fwd_info *it;
+ FOR_EACH_VEC_ELT (store_exprs, i, it)
+ store_exprs_del.safe_push (it->store_mem);
store_exprs.truncate (0);
continue;
}
@@ -526,6 +547,7 @@ store_forwarding_analyzer::avoid_store_forwarding
(basic_block bb)
it->remove = true;
removed_count++;
remove_rest = true;
+ store_exprs_del.safe_push (it->store_mem);
}
}
}
@@ -565,23 +587,45 @@ store_forwarding_analyzer::avoid_store_forwarding
(basic_block bb)
it->remove = true;
removed_count++;
remove_rest = true;
+ forwardings.truncate (0);
}
else if (is_store_forwarding (store_mem, load_mem, &off_val))
{
+ unsigned int j;
+ rtx *del_it;
+ bool same_range_as_removed = false;
+
+ /* Check if another store in the load's address range has
+ been deleted due to a constraint violation. In this case
+ we can't forward any other stores that operate in this
+ range, as it would lead to partial update of the register
+ that holds the loaded value. */
+ FOR_EACH_VEC_ELT (store_exprs_del, j, del_it)
+ {
+ rtx del_store_mem = *del_it;
+ same_range_as_removed
+ = is_store_forwarding (del_store_mem, load_mem, NULL);
+ break;
+ }
+
/* Check if moving this store after the load is legal. */
bool write_dep = false;
- for (unsigned int j = store_exprs.length () - 1; j != i; j--)
+ if (!same_range_as_removed)
{
- if (!store_exprs[j].forwarded
- && output_dependence (store_mem,
- store_exprs[j].store_mem))
+ unsigned int j = store_exprs.length () - 1;
+ for (; j != i; j--)
{
- write_dep = true;
- break;
+ if (!store_exprs[j].forwarded
+ && output_dependence (store_mem,
+ store_exprs[j].store_mem))
+ {
+ write_dep = true;
+ break;
+ }
}
}
- if (!write_dep)
+ if (!same_range_as_removed && !write_dep)
{
it->forwarded = true;
it->offset = off_val;
@@ -601,6 +645,7 @@ store_forwarding_analyzer::avoid_store_forwarding
(basic_block bb)
it->remove = true;
removed_count++;
remove_rest = true;
+ forwardings.truncate (0);
}
}
@@ -608,9 +653,12 @@ store_forwarding_analyzer::avoid_store_forwarding
(basic_block bb)
process_store_forwarding (forwardings, insn, load_mem);
}
+ /* Abort in case that we encounter a memory read/write that is not a
+ simple store/load, as we can't make safe assumptions about the
+ side-effects of this. */
if ((writes_mem && !is_simple_store)
|| (reads_mem && !is_simple_load))
- store_exprs.truncate (0);
+ return;
if (removed_count)
{
diff --git a/gcc/testsuite/gcc.target/i386/pr119795.c
b/gcc/testsuite/gcc.target/i386/pr119795.c
new file mode 100644
index 000000000000..03c91cc288d6
--- /dev/null
+++ b/gcc/testsuite/gcc.target/i386/pr119795.c
@@ -0,0 +1,26 @@
+/* { dg-do run } */
+/* { dg-options "-O -fschedule-insns -favoid-store-forwarding" } */
+
+unsigned a, b, c;
+
+void
+foo (_BitInt(2) b2, unsigned _BitInt(255) by, unsigned _BitInt(5) b5,
+ unsigned _BitInt(256) *ret)
+{
+ unsigned _BitInt(255) bx = b2;
+ by += 0x80000000000000000000000000000000wb;
+ __builtin_memmove (&b, &c, 3);
+ unsigned d = b;
+ unsigned e = __builtin_stdc_rotate_right (0x1uwb % b5, a);
+ unsigned _BitInt(256) r = by + bx + d + e;
+ *ret = r;
+}
+
+int
+main ()
+{
+ unsigned _BitInt(256) x;
+ foo (0, -1, 2, &x);
+ if (x != 0x80000000000000000000000000000000wb)
+ __builtin_abort();
+}
\ No newline at end of file
--
2.50.1
