[DAGCombiner] don't reuse the pointer info for merged store

[shchenz]

Summary

The merged store touches memory for other underlying objects, so mapping
the merged store to the first underlying object is not correct. For example
in https://github.com/llvm/llvm-project/issues/60744, the merged store is
not correctly analyzed as dependent with memory operations which are also
part of the merged store.

Fix https://github.com/llvm/llvm-project/issues/60744

[foad]

llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp

19168

I agree that this looks conservatively safe, but I think it will pessimize the case where the MachinePointerInfos had the same underlying pointer.

[pengfei]

llvm/test/CodeGen/X86/merge-store-dependency.ll

14

CHECKT -> CHECK

[shchenz]

still reuse the first store pointer info for cases where narrow stores all point to the same underlying object

[foad]

llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp

729

This should just take one ArrayRef<MemOpLink> argument.

18987

You don't need a set for this. You only need to remember a single Value *.

[shchenz]

address @foad comments

[foad]

llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp

730

You should not need NumStores. ArrayRef knows its own length.

[shchenz]

remove the unnecessary parameter.

[nikic]

llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp

18995

This should use getUnderlyingObject() unless there's strong reason to be aggressive here.

[shchenz]

use getUnderlyingObject()

llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp

18995

Thanks. Looks good to me. No handling for multiple objects even use getUnderlyingObjects() to reduce the complexity.

[shchenz]

gentle ping

[foad]

llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp

18984

Remove NumStores and use range-based for loops like for (const auto &StoreNode : StoreNodes). Maybe also combine the two loops into one?

[shchenz]

address @foad comments

llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp

18984

Make senses Thanks for the good lessons.

[foad]

LGTM.

[uabelho]

Hi,

I'm seeing a miscompile for my out-of-tree target with this patch. I don't have a reproducer I can share and I'm not sure what is happening yet.
It might very well be something we have broken downstream, but I want to give a heads-up in case other people see problems too.

[uabelho]

In D144711#4177887, @uabelho wrote:

Hi,

I'm seeing a miscompile for my out-of-tree target with this patch. I don't have a reproducer I can share and I'm not sure what is happening yet.
It might very well be something we have broken downstream, but I want to give a heads-up in case other people see problems too.

And now I think it's indeed a downstream bug simply exposed by this patch.
Sorry for the noise!

llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp

 #include "llvm/ADT/SmallBitVector.h"
 #include "llvm/ADT/SmallPtrSet.h"
 #include "llvm/ADT/SmallSet.h"
 #include "llvm/ADT/SmallVector.h"
 #include "llvm/ADT/Statistic.h"
 #include "llvm/Analysis/AliasAnalysis.h"
 #include "llvm/Analysis/MemoryLocation.h"
 #include "llvm/Analysis/TargetLibraryInfo.h"
+#include "llvm/Analysis/ValueTracking.h"
 #include "llvm/Analysis/VectorUtils.h"
 #include "llvm/CodeGen/DAGCombine.h"
 #include "llvm/CodeGen/ISDOpcodes.h"
 #include "llvm/CodeGen/MachineFunction.h"
 #include "llvm/CodeGen/MachineMemOperand.h"
 #include "llvm/CodeGen/RuntimeLibcalls.h"
 #include "llvm/CodeGen/SelectionDAG.h"
 #include "llvm/CodeGen/SelectionDAGAddressAnalysis.h"
private:
     /// can be combined into narrow loads.
     bool BackwardsPropagateMask(SDNode *N);
 
     /// Helper function for mergeConsecutiveStores which merges the component
     /// store chains.
     SDValue getMergeStoreChains(SmallVectorImpl<MemOpLink> &StoreNodes,
                                 unsigned NumStores);
 
+    /// Helper function for mergeConsecutiveStores which checks if all the store
+    /// nodes have the same underlying object. We can still reuse the first
+    /// store's pointer info if all the stores are from the same object.
+    bool hasSameUnderlyingObj(ArrayRef<MemOpLink> StoreNodes);
+
     /// This is a helper function for mergeConsecutiveStores. When the source
     /// elements of the consecutive stores are all constants or all extracted
     /// vector elements, try to merge them into one larger store introducing
     /// bitcasts if necessary.  \return True if a merged store was created.
     bool mergeStoresOfConstantsOrVecElts(SmallVectorImpl<MemOpLink> &StoreNodes,
                                          EVT MemVT, unsigned NumStores,
                                          bool IsConstantSrc, bool UseVector,
                                          bool UseTrunc);
for (unsigned i = 0; i < NumStores; ++i) {
     if (Visited.insert(StoreNodes[i].MemNode->getChain().getNode()).second)
       Chains.push_back(StoreNodes[i].MemNode->getChain());
   }
 
   assert(Chains.size() > 0 && "Chain should have generated a chain");
   return DAG.getTokenFactor(StoreDL, Chains);
 }
 
+bool DAGCombiner::hasSameUnderlyingObj(ArrayRef<MemOpLink> StoreNodes) {
+  unsigned NumStores = StoreNodes.size();
+
+  // Pseudo value like stack frame has its own frame index and size, should not
+  // use the first store's frame index for other frames.
+  for (unsigned i = 0; i < NumStores; ++i)
+    if (StoreNodes[i].MemNode->getMemOperand()->getPseudoValue())
+      return false;
+
+  const Value *UnderlyingObj = nullptr;
+  for (unsigned i = 0; i < NumStores; ++i) {
+    if (!StoreNodes[i].MemNode->getMemOperand()->getValue())
+      return false;
+
+    const Value *Obj =
+        getUnderlyingObject(StoreNodes[i].MemNode->getMemOperand()->getValue());
+
+    if (UnderlyingObj && UnderlyingObj != Obj)
+      return false;
+
+    if (!UnderlyingObj)
+      UnderlyingObj = Obj;
+  }
+
+  return true;
+}
+
 bool DAGCombiner::mergeStoresOfConstantsOrVecElts(
     SmallVectorImpl<MemOpLink> &StoreNodes, EVT MemVT, unsigned NumStores,
     bool IsConstantSrc, bool UseVector, bool UseTrunc) {
   // Make sure we have something to merge.
   if (NumStores < 2)
     return false;
 
   assert((!UseTrunc || !UseVector) &&
if (UseVector) {
     }
 
     // Create the new Load and Store operations.
     StoredVal = DAG.getConstant(StoreInt, DL, StoreTy);
   }
 
   LSBaseSDNode *FirstInChain = StoreNodes[0].MemNode;
   SDValue NewChain = getMergeStoreChains(StoreNodes, NumStores);
+  bool CanReusePtrInfo = hasSameUnderlyingObj(StoreNodes);
 
   // make sure we use trunc store if it's necessary to be legal.
+  // When generate the new widen store, if the first store's pointer info can
+  // not be reused, discard the pointer info except the address space because
+  // now the widen store can not be represented by the original pointer info
+  // which is for the narrow memory object.
   SDValue NewStore;
   if (!UseTrunc) {
-    NewStore = DAG.getStore(NewChain, DL, StoredVal, FirstInChain->getBasePtr(),
-                            FirstInChain->getPointerInfo(),
-                            FirstInChain->getAlign(), *Flags, AAInfo);
+    NewStore = DAG.getStore(
+        NewChain, DL, StoredVal, FirstInChain->getBasePtr(),
+        CanReusePtrInfo
+            ? FirstInChain->getPointerInfo()
+            : MachinePointerInfo(FirstInChain->getPointerInfo().getAddrSpace()),
   } else { // Must be realized as a trunc store
     EVT LegalizedStoredValTy =
         TLI.getTypeToTransformTo(*DAG.getContext(), StoredVal.getValueType());
     unsigned LegalizedStoreSize = LegalizedStoredValTy.getSizeInBits();
     ConstantSDNode *C = cast<ConstantSDNode>(StoredVal);
     SDValue ExtendedStoreVal =
         DAG.getConstant(C->getAPIntValue().zextOrTrunc(LegalizedStoreSize), DL,
                         LegalizedStoredValTy);
     NewStore = DAG.getTruncStore(
         NewChain, DL, ExtendedStoreVal, FirstInChain->getBasePtr(),
-        FirstInChain->getPointerInfo(), StoredVal.getValueType() /*TVT*/,
-        FirstInChain->getAlign(), *Flags, AAInfo);
+        CanReusePtrInfo
+            ? FirstInChain->getPointerInfo()
+            : MachinePointerInfo(FirstInChain->getPointerInfo().getAddrSpace()),
+        StoredVal.getValueType() /*TVT*/, FirstInChain->getAlign(), *Flags,
+        AAInfo);
   }
 
   // Replace all merged stores with the new store.
   for (unsigned i = 0; i < NumStores; ++i)
     CombineTo(StoreNodes[i].MemNode, NewStore);
 
   AddToWorklist(NewChain.getNode());
   return true;
while (NumConsecutiveStores >= 2 && LoadNodes.size() >= 2) {
 
     SDLoc LoadDL(LoadNodes[0].MemNode);
     SDLoc StoreDL(StoreNodes[0].MemNode);
 
     // The merged loads are required to have the same incoming chain, so
     // using the first's chain is acceptable.
 
     SDValue NewStoreChain = getMergeStoreChains(StoreNodes, NumElem);
+    bool CanReusePtrInfo = hasSameUnderlyingObj(StoreNodes);
     AddToWorklist(NewStoreChain.getNode());
 
     MachineMemOperand::Flags LdMMOFlags =
         isDereferenceable ? MachineMemOperand::MODereferenceable
                           : MachineMemOperand::MONone;
     if (IsNonTemporalLoad)
       LdMMOFlags |= MachineMemOperand::MONonTemporal;
 
if (UseVectorTy || !DoIntegerTruncate) {
                "Unexpected type for rotate-able load pair");
         SDValue RotAmt =
             DAG.getShiftAmountConstant(LoadWidth / 2, JointMemOpVT, LoadDL);
         // Target can convert to the identical ROTR if it does not have ROTL.
         StoreOp = DAG.getNode(ISD::ROTL, LoadDL, JointMemOpVT, NewLoad, RotAmt);
       }
       NewStore = DAG.getStore(
           NewStoreChain, StoreDL, StoreOp, FirstInChain->getBasePtr(),
-          FirstInChain->getPointerInfo(), FirstStoreAlign, StMMOFlags);
+          CanReusePtrInfo ? FirstInChain->getPointerInfo()
+                          : MachinePointerInfo(FirstStoreAS),
+          FirstStoreAlign, StMMOFlags);
     } else { // This must be the truncstore/extload case
       EVT ExtendedTy =
           TLI.getTypeToTransformTo(*DAG.getContext(), JointMemOpVT);
       NewLoad = DAG.getExtLoad(ISD::EXTLOAD, LoadDL, ExtendedTy,
                                FirstLoad->getChain(), FirstLoad->getBasePtr(),
                                FirstLoad->getPointerInfo(), JointMemOpVT,
                                FirstLoadAlign, LdMMOFlags);
       NewStore = DAG.getTruncStore(
           NewStoreChain, StoreDL, NewLoad, FirstInChain->getBasePtr(),
-          FirstInChain->getPointerInfo(), JointMemOpVT,
-          FirstInChain->getAlign(), FirstInChain->getMemOperand()->getFlags());
+          CanReusePtrInfo ? FirstInChain->getPointerInfo()
+                          : MachinePointerInfo(FirstStoreAS),
+          JointMemOpVT, FirstInChain->getAlign(),
+          FirstInChain->getMemOperand()->getFlags());
     }
 
     // Transfer chain users from old loads to the new load.
     for (unsigned i = 0; i < NumElem; ++i) {
       LoadSDNode *Ld = cast<LoadSDNode>(LoadNodes[i].MemNode);
       DAG.ReplaceAllUsesOfValueWith(SDValue(Ld, 1),
                                     SDValue(NewLoad.getNode(), 1));
     }

llvm/test/CodeGen/AArch64/argument-blocks-array-of-struct.ll

 ; CHECK-NEXT:    sub sp, sp, #96
 ; CHECK-NEXT:    .cfi_def_cfa_offset 96
 ; CHECK-NEXT:    str x30, [sp, #80] // 8-byte Folded Spill
 ; CHECK-NEXT:    .cfi_offset w30, -16
 ; CHECK-NEXT:    adrp x8, in_memory_store
 ; CHECK-NEXT:    add x8, x8, :lo12:in_memory_store
 ; CHECK-NEXT:    ldp q0, q1, [x8]
 ; CHECK-NEXT:    ldp q2, q3, [x8, #32]
-; CHECK-NEXT:    stp q0, q1, [sp]
 ; CHECK-NEXT:    ldr d4, [x8, #64]
+; CHECK-NEXT:    stp q0, q1, [sp]
 ; CHECK-NEXT:    stp q2, q3, [sp, #32]
 ; CHECK-NEXT:    str d4, [sp, #64]
 ; CHECK-NEXT:    bl callee_in_memory
 ; CHECK-NEXT:    ldr x30, [sp, #80] // 8-byte Folded Reload
 ; CHECK-NEXT:    add sp, sp, #96
 ; CHECK-NEXT:    ret
   %1 = load %T_IN_MEMORY, ptr @in_memory_store
   call void @callee_in_memory(%T_IN_MEMORY %1)

llvm/test/CodeGen/X86/merge-store-dependency.ll

 ; RUN: llc < %s -mtriple=i686-unknown-linux-android24 -verify-machineinstrs \
 ; RUN:   -debug-only=machine-scheduler -o - 2>&1 | FileCheck %s
 ; REQUIRES: asserts
 
 ;; MOVUPSmr is a merged store from stack objects %ir.arg1, %ir.arg2, %ir.arg3,
 ;; %ir.arg4.
-;; FIXME: the merged store should have dependency with %ir.arg4.
+;; Check that the merged store has dependency with %ir.arg4.
 
 ; CHECK:       ********** MI Scheduling **********
 ; CHECK-LABEL: f:%bb.0 bb
 ; CHECK:       SU([[ARG4:[0-9]+]]):{{.*}}MOV32rm{{.*}}load (s32) from %ir.arg4
 ; CHECK:       SU([[#WIDEN:]]):{{.*}}MOVUPSmr{{.*}}store (s128) into
 ; CHECK:         Predecessors:
-; CHECK-NOT:       SU([[ARG4]]):{{.*}}Memory
+; CHECK:           SU([[ARG4]]):{{.*}}Memory
 ; CHECK:       SU([[#WIDEN+1]])
 ;
 
 define void @f(ptr %arg, ptr byval(ptr) %arg1, ptr byval(ptr) %arg2, ptr byval(ptr) %arg3, ptr byval(ptr) %arg4) #0 {
 bb:
   %inst = alloca ptr, align 4
   %inst5 = alloca ptr, align 4
   %inst6 = alloca ptr, align 4