[PATCH] D137791: [SDAG] bail out of mergeTruncStores() if there's an unknown store in the chain

[Sanjay Patel via Phabricator via llvm-commits]

spatel created this revision.
spatel added reviewers: jonpa, efriedma, RKSimon, craig.topper, biplmish, dmgreen.
Herald added subscribers: StephenFan, ecnelises, hiraditya, mcrosier.
Herald added a project: All.
spatel requested review of this revision.
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.

This hopefully fixes the miscompile in issue #58883 <https://github.com/llvm/llvm-project/issues/58883>.

I'm not sure if the baseline test actually shows the expected miscompile, but the test diff does demonstrate that we gave up on store merging in that example.

This change should be strictly safe (just adds another clause to avoid the transform), and it does not prohibit any existing valid optimizations based on regression tests. I want to believe that it's also a sufficient fix (possibly overkill), but I'm not sure how to prove that.


https://reviews.llvm.org/D137791

Files:
  llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
  llvm/test/CodeGen/SystemZ/merge-stores.ll


Index: llvm/test/CodeGen/SystemZ/merge-stores.ll
===================================================================
--- llvm/test/CodeGen/SystemZ/merge-stores.ll
+++ llvm/test/CodeGen/SystemZ/merge-stores.ll
@@ -16,8 +16,8 @@
 ; CHECK-NEXT:    srlg %r0, %r2, 32
 ; CHECK-NEXT:    st %r0, 0(%r1)
 ; CHECK-NEXT:    lhi %r0, 0
-; CHECK-NEXT:    stg %r2, 0(%r1)
 ; CHECK-NEXT:    sthrl %r0, e
+; CHECK-NEXT:    st %r2, 4(%r1)
 ; CHECK-NEXT:    strl %r2, d
 ; CHECK-NEXT:    br %r14
   %xsh = lshr i64 %x, 32
Index: llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
===================================================================
--- llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
+++ llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
@@ -8110,6 +8110,19 @@
         Store->isIndexed())
       return SDValue();
     Stores.push_back(Store);
+
+    // If this store is used as a chain operand in a store that is not included
+    // in the set of stores to be merged, then the transform may not be safe.
+    // We must preserve the order of that other store relative to the indivdual
+    // stores in this set.
+    for (SDNode *Use : Store->uses()) {
+      auto *StoreUse = dyn_cast<StoreSDNode>(Use);
+      if (StoreUse && StoreUse->getChain().getNode() == Store &&
+          !is_contained(Stores, StoreUse))
+        return SDValue();
+    }
+
+    // Get the next link in the chain and try to collect another store.
     Chain = Store->getChain();
   }
   // There is no reason to continue if we do not have at least a pair of stores.


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D137791.474566.patch
Type: text/x-patch
Size: 1535 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20221110/bbaffea6/attachment.bin>