[llvm] 0037e21 - [SDAG] bail out of mergeTruncStores() if there's any other use in the chain

[Sanjay Patel via llvm-commits]

Author: Sanjay Patel
Date: 2022-12-02T10:08:19-05:00
New Revision: 0037e21f28adbeb9f8cac30016521d543561645e

URL: https://github.com/llvm/llvm-project/commit/0037e21f28adbeb9f8cac30016521d543561645e
DIFF: https://github.com/llvm/llvm-project/commit/0037e21f28adbeb9f8cac30016521d543561645e.diff

LOG: [SDAG] bail out of mergeTruncStores() if there's any other use in the chain

This fixes the miscompile in issue #58883.

The test demonstrates 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.

Differential Revision: https://reviews.llvm.org/D137791

Added: 
    

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

Removed: 
    


################################################################################
diff  --git a/llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp b/llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
index 9970ba20610ba..f6d577562e233 100644
--- a/llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
+++ b/llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
@@ -8150,9 +8150,13 @@ SDValue DAGCombiner::mergeTruncStores(StoreSDNode *N) {
   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();

diff  --git a/llvm/test/CodeGen/SystemZ/merge-stores.ll b/llvm/test/CodeGen/SystemZ/merge-stores.ll
index 74732583dd5f1..f09d95cb1f53a 100644
--- a/llvm/test/CodeGen/SystemZ/merge-stores.ll
+++ b/llvm/test/CodeGen/SystemZ/merge-stores.ll
@@ -9,9 +9,10 @@ target datalayout = "E-m:e-i1:8:16-i8:8:16-i64:64-f128:64-v128:64-a:8:16-n32:64"
 @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 @@ define signext i32 @main() {
 ; 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