[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 478680.
spatel added a comment.

Patch updated:

1. Check one-use of each store that occurs before 'N'.
2. Revert back to the SystemZ asm test because that shows the miscompile.


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,9 +9,10 @@
 @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 
-; a 64-bit store (stg) of r0.
+; 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. The store of r0 can follow
+; the store to 'e' only if it is a 32-bit store (st).
 
 define signext i32 @main() {
 ; CHECK-LABEL: 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
@@ -8150,9 +8150,13 @@
   while (auto *Store = dyn_cast<StoreSDNode>(Chain)) {
     // All stores must be the same size to ensure that we are writing all of the
     // bytes in the wide value.
+    // This store should have exactly one use as a chain operand for another
+    // store in the merging set. If there are other chain uses, then the
+    // transform may not be safe because order of loads/stores outside of this
+    // set may not be preserved.
     // TODO: We could allow multiple sizes by tracking each stored byte.
     if (Store->getMemoryVT() != MemVT || !Store->isSimple() ||
-        Store->isIndexed())
+        Store->isIndexed() || !Store->hasOneUse())
       return SDValue();
     Stores.push_back(Store);
     Chain = Store->getChain();


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D137791.478680.patch
Type: text/x-patch
Size: 2027 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20221129/2f155af3/attachment.bin>