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

[Sanjay Patel via Phabricator via llvm-commits]

spatel updated this revision to Diff 475003.
spatel added a comment.

Patch updated:
As discussed in #58883, I was looking at the wrong asm. The test is corrected/reduced from before, and I added a test comment to explain the miscompile. (I didn't push the baseline test update yet in case I my reading of the asm is incorrect.)


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D137791/new/

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
@@ -9,8 +9,9 @@
 @f = dso_local local_unnamed_addr global ptr @e, align 8
 @d = dso_local local_unnamed_addr global i32 0, align 4
 
-; FIXME: This shows a miscompile caused by merging truncated 
-; stores if the store of 0 (sthrl) to 'e' happens before 
+; PR58883:
+; This shows a miscompile caused by merging truncated
+; stores if the store of 0 (sthrl) to 'e' happens before
 ; a 64-bit store (stg) of r0.
 
 define signext i32 @main() {
@@ -22,7 +23,7 @@
 ; CHECK-NEXT:    st %r2, 0(%r1)
 ; CHECK-NEXT:    lhi %r2, 0
 ; CHECK-NEXT:    sthrl %r2, e
-; CHECK-NEXT:    stg %r0, 0(%r1)
+; CHECK-NEXT:    st %r0, 4(%r1)
 ; CHECK-NEXT:    lghi %r2, 0
 ; CHECK-NEXT:    strl %r0, d
 ; CHECK-NEXT:    br %r14
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.475003.patch
Type: text/x-patch
Size: 1946 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20221113/64e80626/attachment.bin>