Revert "[SDAG] Relax conditions under stores of loaded values can be merged"

[dlj]

Summary

This reverts commit a7aa63a594edfea5321d289b05e074e41c167312.

The change fails with ASAN enabled:

ERROR: AddressSanitizer: use-after-poison on address ... at ...
READ of size 2 at ... thread T0

#0 ... in llvm::SDNode::getNumValues() const <snip>/include/llvm/CodeGen/SelectionDAGNodes.h:855:42
#1 ... in llvm::SDNode::hasAnyUseOfValue(unsigned int) const <snip>/lib/CodeGen/SelectionDAG/SelectionDAG.cpp:7270:3
#2 ... in llvm::SDValue::use_empty() const <snip> include/llvm/CodeGen/SelectionDAGNodes.h:1042:17
#3 ... in (anonymous namespace)::DAGCombiner::MergeConsecutiveStores(llvm::StoreSDNode*) <snip>/lib/CodeGen/SelectionDAG/DAGCombiner.cpp:12944:7

include/llvm/CodeGen/ISDOpcodes.h

namespace ISD {
   enum NodeType {
     /// DELETED_NODE - This is an illegal value that is used to catch
     /// errors.  This opcode is not a legal opcode for any node.
     DELETED_NODE,
 
     /// EntryToken - This is the marker used to indicate the start of a region.
     EntryToken,
 
-    /// DummyNode - Temporary node for node replacement. These nodes
-    /// should not persist beyond their introduction.
-    DummyNode,
-
     /// TokenFactor - This node takes multiple tokens as input and produces a
     /// single token result. This is used to represent the fact that the operand
     /// operators are independent of each other.
     TokenFactor,
 
     /// AssertSext, AssertZext - These nodes record if a register contains a
     /// value that has already been zero or sign extended from a narrower type.
     /// These nodes take two operands.  The first is the node that has already

lib/CodeGen/SelectionDAG/DAGCombiner.cpp

bool DAGCombiner::MergeConsecutiveStores(StoreSDNode *St) {
   // Find acceptable loads. Loads need to have the same chain (token factor),
   // must not be zext, volatile, indexed, and they must be consecutive.
   BaseIndexOffset LdBasePtr;
   for (unsigned i = 0; i < NumConsecutiveStores; ++i) {
     StoreSDNode *St  = cast<StoreSDNode>(StoreNodes[i].MemNode);
     LoadSDNode *Ld = dyn_cast<LoadSDNode>(St->getValue());
     if (!Ld) break;
 
+    // Loads must only have one use.
+    if (!Ld->hasNUsesOfValue(1, 0))
+      break;
+
     // The memory operands must not be volatile.
     if (Ld->isVolatile() || Ld->isIndexed())
       break;
 
     // We do not accept ext loads.
     if (Ld->getExtensionType() != ISD::NON_EXTLOAD)
       break;
 
+    // The stored memory type must be the same.
+    if (Ld->getMemoryVT() != MemVT)
+      break;
+
     BaseIndexOffset LdPtr = BaseIndexOffset::match(Ld->getBasePtr(), DAG);
     // If this is not the first ptr that we check.
     if (LdBasePtr.Base.getNode()) {
       // The base ptr must be the same.
       if (!LdPtr.equalBaseIndex(LdBasePtr))
         break;
     } else {
       // Check that all other base pointers are the same as this one.
bool DAGCombiner::MergeConsecutiveStores(StoreSDNode *St) {
 
   SDValue NewStore =
       DAG.getStore(NewStoreChain, StoreDL, NewLoad, FirstInChain->getBasePtr(),
                    FirstInChain->getPointerInfo(), FirstStoreAlign);
 
   // Transfer chain users from old loads to the new load.
   for (unsigned i = 0; i < NumElem; ++i) {
     LoadSDNode *Ld = cast<LoadSDNode>(LoadNodes[i].MemNode);
-    if (SDValue(Ld, 0).hasOneUse()) {
-      // Only the original store used value so just replace chain.
-      DAG.ReplaceAllUsesOfValueWith(SDValue(Ld, 1),
-                                    SDValue(NewLoad.getNode(), 1));
-    } else {
-      // Multiple uses exist. Keep the old load in line with the new
-      // load, i.e. Replace chains using Ld's chain with a
-      // TokenFactor. Create a temporary node to serve as a placer so
-      // we do not replace the reference to original Load's chain in
-      // the TokenFactor.
-      SDValue TokenDummy = DAG.getNode(ISD::DummyNode, SDLoc(Ld), MVT::Other);
-
-      // Replace all references to Load's output chain to TokenDummy
-      CombineTo(Ld, SDValue(Ld, 0), TokenDummy, false);
-      SDValue Token =
-          DAG.getNode(ISD::TokenFactor, SDLoc(Ld), MVT::Other, SDValue(Ld, 1),
-                      SDValue(NewLoad.getNode(), 1));
-      // Replace all uses of TokenDummy from itself to Ld's output chain.
-      CombineTo(TokenDummy.getNode(), Token);
-      assert(TokenDummy.use_empty() && "TokenDummy should be unused");
-      AddToWorklist(Ld);
-    }
   }
 
   // Replace the all stores with the new store.
   for (unsigned i = 0; i < NumElem; ++i)
     CombineTo(StoreNodes[i].MemNode, NewStore);
   return true;
 }
 

test/CodeGen/X86/merge_store_duplicated_loads.ll

 ; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py
 ; RUN: llc %s -mtriple=x86_64-unknown-linux-gnu -mcpu=corei7 -o - | FileCheck %s
 
-; PR32086
+
 target triple = "x86_64-unknown-linux-gnu"
 
 define void @merge_double(double* noalias nocapture %st, double* noalias nocapture readonly %ld) #0 {
 ; CHECK-LABEL: merge_double:
 ; CHECK:       # BB#0:
-; CHECK-NEXT:    movups (%rsi), %xmm0
-; CHECK-NEXT:    movups %xmm0, (%rdi)
-; CHECK-NEXT:    movups %xmm0, 16(%rdi)
+; CHECK-NEXT:    movsd {{.*#+}} xmm0 = mem[0],zero
+; CHECK-NEXT:    movsd {{.*#+}} xmm1 = mem[0],zero
+; CHECK-NEXT:    movsd %xmm0, (%rdi)
+; CHECK-NEXT:    movsd %xmm1, 8(%rdi)
+; CHECK-NEXT:    movsd %xmm0, 16(%rdi)
+; CHECK-NEXT:    movsd %xmm1, 24(%rdi)
 ; CHECK-NEXT:    retq
   %ld_idx1 = getelementptr inbounds double, double* %ld, i64 1
   %ld0 = load double, double* %ld, align 8, !tbaa !2
   %ld1 = load double, double* %ld_idx1, align 8, !tbaa !2
 
   %st_idx1 = getelementptr inbounds double, double* %st, i64 1
   %st_idx2 = getelementptr inbounds double, double* %st, i64 2
   %st_idx3 = getelementptr inbounds double, double* %st, i64 3
 
   store double %ld0, double* %st, align 8, !tbaa !2
   store double %ld1, double* %st_idx1, align 8, !tbaa !2
   store double %ld0, double* %st_idx2, align 8, !tbaa !2
   store double %ld1, double* %st_idx3, align 8, !tbaa !2
   ret void
 }
 
 define void @merge_loadstore_int(i64* noalias nocapture readonly %p, i64* noalias nocapture %q) local_unnamed_addr #0 {
 ; CHECK-LABEL: merge_loadstore_int:
 ; CHECK:       # BB#0: # %entry
-; CHECK-NEXT:    movups (%rdi), %xmm0
-; CHECK-NEXT:    movups %xmm0, (%rsi)
-; CHECK-NEXT:    movups %xmm0, 16(%rsi)
+; CHECK-NEXT:    movq (%rdi), %rax
+; CHECK-NEXT:    movq 8(%rdi), %rcx
+; CHECK-NEXT:    movq %rax, (%rsi)
+; CHECK-NEXT:    movq %rcx, 8(%rsi)
+; CHECK-NEXT:    movq %rax, 16(%rsi)
+; CHECK-NEXT:    movq %rcx, 24(%rsi)
 ; CHECK-NEXT:    retq
 entry:
   %0 = load i64, i64* %p, align 8, !tbaa !1
   %arrayidx1 = getelementptr inbounds i64, i64* %p, i64 1
   %1 = load i64, i64* %arrayidx1, align 8, !tbaa !1
   store i64 %0, i64* %q, align 8, !tbaa !1
   %arrayidx3 = getelementptr inbounds i64, i64* %q, i64 1
   store i64 %1, i64* %arrayidx3, align 8, !tbaa !1
   %arrayidx4 = getelementptr inbounds i64, i64* %q, i64 2
   store i64 %0, i64* %arrayidx4, align 8, !tbaa !1
   %arrayidx5 = getelementptr inbounds i64, i64* %q, i64 3
   store i64 %1, i64* %arrayidx5, align 8, !tbaa !1
   ret void
 }
 
 define i64 @merge_loadstore_int_with_extra_use(i64* noalias nocapture readonly %p, i64* noalias nocapture %q) local_unnamed_addr #0 {
 ; CHECK-LABEL: merge_loadstore_int_with_extra_use:
 ; CHECK:       # BB#0: # %entry
 ; CHECK-NEXT:    movq (%rdi), %rax
-; CHECK-NEXT:    movups (%rdi), %xmm0
-; CHECK-NEXT:    movups %xmm0, (%rsi)
-; CHECK-NEXT:    movups %xmm0, 16(%rsi)
+; CHECK-NEXT:    movq 8(%rdi), %rcx
+; CHECK-NEXT:    movq %rax, (%rsi)
+; CHECK-NEXT:    movq %rcx, 8(%rsi)
+; CHECK-NEXT:    movq %rax, 16(%rsi)
+; CHECK-NEXT:    movq %rcx, 24(%rsi)
 ; CHECK-NEXT:    retq
 entry:
   %0 = load i64, i64* %p, align 8, !tbaa !1
   %arrayidx1 = getelementptr inbounds i64, i64* %p, i64 1
   %1 = load i64, i64* %arrayidx1, align 8, !tbaa !1
   store i64 %0, i64* %q, align 8, !tbaa !1
   %arrayidx3 = getelementptr inbounds i64, i64* %q, i64 1
   store i64 %1, i64* %arrayidx3, align 8, !tbaa !1