asf: Skip when an instruction doesn't satisfy the constraints [PR119795]

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.

diff --git a/gcc/avoid-store-forwarding.cc b/gcc/avoid-store-forwarding.cc
index 785efd22606f245fa404c27b501b93682a7f5a42..e5ead78d5b80db750d8b7e61c8079615a6cc5275 100644 (file)
--- 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,46 @@ 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);
+                     if (same_range_as_removed)
+                       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 +646,7 @@ store_forwarding_analyzer::avoid_store_forwarding (basic_block bb)
                  it->remove = true;
                  removed_count++;
                  remove_rest = true;
+                 forwardings.truncate (0);
                }
            }
 
@@ -608,9 +654,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 (file)
index 0000000..03c91cc
--- /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