pair-fusion: Check for invalid use arrays [PR118320]

As Andrew says in the bugzilla comments, this PR is about a case where
we tried to fuse two stores of x0, one in which x0 was defined and one
in which it was undefined.  merge_access_arrays failed on the conflict,
but the failure wasn't caught.

Normally the hazard detection code would fail if the instructions
had incompatible uses.  However, an undefined use doesn't impose
many restrictions on movements.  I think this is likely to be the
only case where hazard detection isn't enough.

As Andrew notes in bugzilla, it might be possible to allow uses
of defined and undefined values to be merged to the defined value.
But that sounds dangerous in the general case, as an rtl-ssa-level
decision.  We might run the risk of turning conditional UB into
unconditional UB.  And LLVM proves that the definition of "undef"
isn't simple.

gcc/
	PR rtl-optimization/118320
	* pair-fusion.cc (pair_fusion_bb_info::fuse_pair): Commonize
	the merge of input_uses and return early if it fails.

gcc/testsuite/
	PR rtl-optimization/118320
	* g++.dg/torture/pr118320.C: New test.

diff --git a/gcc/pair-fusion.cc b/gcc/pair-fusion.cc
index 602e572ab6c7ede5f7fb0eb6da996eee02e28172..5708d0f3b671b3303cfda0649215d8550e89f545 100644 (file)
--- a/gcc/pair-fusion.cc
+++ b/gcc/pair-fusion.cc
@@ -1730,6 +1730,24 @@ pair_fusion_bb_info::fuse_pair (bool load_p,
       input_uses[i] = remove_note_accesses (attempt, input_uses[i]);
     }
 
+  // Get the uses that the pair instruction would have, and punt if
+  // the unpaired instructions use different definitions of the same
+  // register.  That would normally be caught as a side-effect of
+  // hazard detection below, but this check also deals with cases
+  // in which one use is undefined and the other isn't.
+  auto new_uses = merge_access_arrays (attempt,
+                                      drop_memory_access (input_uses[0]),
+                                      drop_memory_access (input_uses[1]));
+  if (!new_uses.is_valid ())
+    {
+      if (dump_file)
+       fprintf (dump_file,
+                "  load pair: i%d and i%d use different definiitions of"
+                " the same register\n",
+                insns[0]->uid (), insns[1]->uid ());
+      return false;
+    }
+
   // Edge case: if the first insn is a writeback load and the
   // second insn is a non-writeback load which transfers into the base
   // register, then we should drop the writeback altogether as the
@@ -1852,11 +1870,7 @@ pair_fusion_bb_info::fuse_pair (bool load_p,
                                                   input_defs[1]);
       gcc_assert (pair_change->new_defs.is_valid ());
 
-      pair_change->new_uses
-       = merge_access_arrays (attempt,
-                              drop_memory_access (input_uses[0]),
-                              drop_memory_access (input_uses[1]));
-      gcc_assert (pair_change->new_uses.is_valid ());
+      pair_change->new_uses = new_uses;
       set_pair_pat (pair_change);
     }
   else
@@ -1877,9 +1891,7 @@ pair_fusion_bb_info::fuse_pair (bool load_p,
            case Action::CHANGE:
            {
              set_pair_pat (change);
-             change->new_uses = merge_access_arrays (attempt,
-                                                     input_uses[0],
-                                                     input_uses[1]);
+             change->new_uses = new_uses;
              auto d1 = drop_memory_access (input_defs[0]);
              auto d2 = drop_memory_access (input_defs[1]);
              change->new_defs = merge_access_arrays (attempt, d1, d2);
@@ -1907,9 +1919,7 @@ pair_fusion_bb_info::fuse_pair (bool load_p,
              auto new_insn = crtl->ssa->create_insn (attempt, INSN, pair_pat);
              change = make_change (new_insn);
              change->move_range = move_range;
-             change->new_uses = merge_access_arrays (attempt,
-                                                     input_uses[0],
-                                                     input_uses[1]);
+             change->new_uses = new_uses;
              gcc_assert (change->new_uses.is_valid ());
 
              auto d1 = drop_memory_access (input_defs[0]);

diff --git a/gcc/testsuite/g++.dg/torture/pr118320.C b/gcc/testsuite/g++.dg/torture/pr118320.C
new file mode 100644 (file)
index 0000000..228d798
--- /dev/null
+++ b/gcc/testsuite/g++.dg/torture/pr118320.C
@@ -0,0 +1,15 @@
+// { dg-additional-options "-fno-tree-sra" } */
+
+struct T{
+  long f[2];
+};
+void f1(int);
+void g(T&);
+void f1()
+{
+  T a;
+  a.~T(); // To force the clobber
+  a.f[1] = 1;
+  T b = a;
+  g(b);
+}