[llvm] fc26ab3 - [DAGCombiner] don't use the pointer info for widen store
Chen Zheng via llvm-commits
llvm-commits at lists.llvm.org
Tue Mar 7 17:31:59 PST 2023
Author: Chen Zheng
Date: 2023-03-07T20:31:09-05:00
New Revision: fc26ab36a20a63c5aef5c46e2b7815b5369025a0
URL: https://github.com/llvm/llvm-project/commit/fc26ab36a20a63c5aef5c46e2b7815b5369025a0
DIFF: https://github.com/llvm/llvm-project/commit/fc26ab36a20a63c5aef5c46e2b7815b5369025a0.diff
LOG: [DAGCombiner] don't use the pointer info for widen store
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.
Fixes #60744
Reviewed By: foad
Differential Revision: https://reviews.llvm.org/D144711
Added:
Modified:
llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
llvm/test/CodeGen/AArch64/argument-blocks-array-of-struct.ll
llvm/test/CodeGen/X86/merge-store-dependency.ll
Removed:
################################################################################
diff --git a/llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp b/llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
index 1d3ad49717489..d28597308d763 100644
--- a/llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
+++ b/llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
@@ -30,6 +30,7 @@
#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"
@@ -722,6 +723,11 @@ namespace {
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
@@ -19002,6 +19008,30 @@ SDValue DAGCombiner::getMergeStoreChains(SmallVectorImpl<MemOpLink> &StoreNodes,
return DAG.getTokenFactor(StoreDL, Chains);
}
+bool DAGCombiner::hasSameUnderlyingObj(ArrayRef<MemOpLink> StoreNodes) {
+ const Value *UnderlyingObj = nullptr;
+ for (const auto &MemOp : StoreNodes) {
+ const MachineMemOperand *MMO = MemOp.MemNode->getMemOperand();
+ // Pseudo value like stack frame has its own frame index and size, should
+ // not use the first store's frame index for other frames.
+ if (MMO->getPseudoValue())
+ return false;
+
+ if (!MMO->getValue())
+ return false;
+
+ const Value *Obj = getUnderlyingObject(MMO->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) {
@@ -19153,13 +19183,21 @@ bool DAGCombiner::mergeStoresOfConstantsOrVecElts(
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()),
+ FirstInChain->getAlign(), *Flags, AAInfo);
} else { // Must be realized as a trunc store
EVT LegalizedStoredValTy =
TLI.getTypeToTransformTo(*DAG.getContext(), StoredVal.getValueType());
@@ -19170,8 +19208,11 @@ bool DAGCombiner::mergeStoresOfConstantsOrVecElts(
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.
@@ -19864,6 +19905,7 @@ bool DAGCombiner::tryStoreMergeOfLoads(SmallVectorImpl<MemOpLink> &StoreNodes,
// using the first's chain is acceptable.
SDValue NewStoreChain = getMergeStoreChains(StoreNodes, NumElem);
+ bool CanReusePtrInfo = hasSameUnderlyingObj(StoreNodes);
AddToWorklist(NewStoreChain.getNode());
MachineMemOperand::Flags LdMMOFlags =
@@ -19893,7 +19935,9 @@ bool DAGCombiner::tryStoreMergeOfLoads(SmallVectorImpl<MemOpLink> &StoreNodes,
}
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);
@@ -19903,8 +19947,10 @@ bool DAGCombiner::tryStoreMergeOfLoads(SmallVectorImpl<MemOpLink> &StoreNodes,
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.
diff --git a/llvm/test/CodeGen/AArch64/argument-blocks-array-of-struct.ll b/llvm/test/CodeGen/AArch64/argument-blocks-array-of-struct.ll
index e90f89f359ac7..407a8e2120ccc 100644
--- a/llvm/test/CodeGen/AArch64/argument-blocks-array-of-struct.ll
+++ b/llvm/test/CodeGen/AArch64/argument-blocks-array-of-struct.ll
@@ -503,8 +503,8 @@ define void @argument_in_memory() {
; 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
diff --git a/llvm/test/CodeGen/X86/merge-store-dependency.ll b/llvm/test/CodeGen/X86/merge-store-dependency.ll
index b28456da8bf8c..1e1802b1d50b3 100644
--- a/llvm/test/CodeGen/X86/merge-store-dependency.ll
+++ b/llvm/test/CodeGen/X86/merge-store-dependency.ll
@@ -4,14 +4,14 @@
;; 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]])
;
More information about the llvm-commits
mailing list