From 0c0db7175b65821971c10e9efeefaf71697175a9 Mon Sep 17 00:00:00 2001 From: Bjorn Steinbrink Date: Fri, 23 Feb 2018 10:41:57 +0000 Subject: [PATCH] Mark MergedLoadStoreMotion as not preserving MemDep results Summary: MemDep caches results that signify that a dependence is non-local, and there is currently no way to invalidate such cache entries. Unfortunately, when MLSM sinks a store that can result in a non-local dependence becoming a local one, and then MemDep gives wrong answers. The easiest way out here is to just say that MLSM does indeed not preserve MemDep results. Reviewers: davide, Gerolf Subscribers: llvm-commits Differential Revision: https://reviews.llvm.org/D43177 git-svn-id: https://llvm.org/svn/llvm-project/llvm/trunk@325880 91177308-0d34-0410-b5e6-96231b3b80d8 --- lib/Transforms/Scalar/MergedLoadStoreMotion.cpp | 49 ++++--------------------- test/Transforms/GVN/pr36063.ll | 22 +++++++++++ 2 files changed, 30 insertions(+), 41 deletions(-) create mode 100644 test/Transforms/GVN/pr36063.ll diff --git a/lib/Transforms/Scalar/MergedLoadStoreMotion.cpp b/lib/Transforms/Scalar/MergedLoadStoreMotion.cpp index f2f615cb9b0..058da52dd84 100644 --- a/lib/Transforms/Scalar/MergedLoadStoreMotion.cpp +++ b/lib/Transforms/Scalar/MergedLoadStoreMotion.cpp @@ -80,7 +80,6 @@ #include "llvm/Analysis/CFG.h" #include "llvm/Analysis/GlobalsModRef.h" #include "llvm/Analysis/Loads.h" -#include "llvm/Analysis/MemoryDependenceAnalysis.h" #include "llvm/Analysis/ValueTracking.h" #include "llvm/IR/Metadata.h" #include "llvm/Support/Debug.h" @@ -97,7 +96,6 @@ namespace { // MergedLoadStoreMotion Pass //===----------------------------------------------------------------------===// class MergedLoadStoreMotion { - MemoryDependenceResults *MD = nullptr; AliasAnalysis *AA = nullptr; // The mergeLoad/Store algorithms could have Size0 * Size1 complexity, @@ -107,14 +105,9 @@ class MergedLoadStoreMotion { const int MagicCompileTimeControl = 250; public: - bool run(Function &F, MemoryDependenceResults *MD, AliasAnalysis &AA); + bool run(Function &F, AliasAnalysis &AA); private: - /// - /// \brief Remove instruction from parent and update memory dependence - /// analysis. - /// - void removeInstruction(Instruction *Inst); BasicBlock *getDiamondTail(BasicBlock *BB); bool isDiamondHead(BasicBlock *BB); // Routines for sinking stores @@ -128,22 +121,6 @@ private: } // end anonymous namespace /// -/// \brief Remove instruction from parent and update memory dependence analysis. -/// -void MergedLoadStoreMotion::removeInstruction(Instruction *Inst) { - // Notify the memory dependence analysis. - if (MD) { - MD->removeInstruction(Inst); - if (auto *LI = dyn_cast(Inst)) - MD->invalidateCachedPointerInfo(LI->getPointerOperand()); - if (Inst->getType()->isPtrOrPtrVectorTy()) { - MD->invalidateCachedPointerInfo(Inst); - } - } - Inst->eraseFromParent(); -} - -/// /// \brief Return tail block of a diamond. /// BasicBlock *MergedLoadStoreMotion::getDiamondTail(BasicBlock *BB) { @@ -236,8 +213,6 @@ PHINode *MergedLoadStoreMotion::getPHIOperand(BasicBlock *BB, StoreInst *S0, &BB->front()); NewPN->addIncoming(Opd1, S0->getParent()); NewPN->addIncoming(Opd2, S1->getParent()); - if (MD && NewPN->getType()->isPtrOrPtrVectorTy()) - MD->invalidateCachedPointerInfo(NewPN); return NewPN; } @@ -275,12 +250,12 @@ bool MergedLoadStoreMotion::sinkStore(BasicBlock *BB, StoreInst *S0, // New PHI operand? Use it. if (PHINode *NewPN = getPHIOperand(BB, S0, S1)) SNew->setOperand(0, NewPN); - removeInstruction(S0); - removeInstruction(S1); + S0->eraseFromParent(); + S1->eraseFromParent(); A0->replaceAllUsesWith(ANew); - removeInstruction(A0); + A0->eraseFromParent(); A1->replaceAllUsesWith(ANew); - removeInstruction(A1); + A1->eraseFromParent(); return true; } return false; @@ -344,9 +319,7 @@ bool MergedLoadStoreMotion::mergeStores(BasicBlock *T) { return MergedStores; } -bool MergedLoadStoreMotion::run(Function &F, MemoryDependenceResults *MD, - AliasAnalysis &AA) { - this->MD = MD; +bool MergedLoadStoreMotion::run(Function &F, AliasAnalysis &AA) { this->AA = &AA; bool Changed = false; @@ -382,9 +355,7 @@ public: if (skipFunction(F)) return false; MergedLoadStoreMotion Impl; - auto *MDWP = getAnalysisIfAvailable(); - return Impl.run(F, MDWP ? &MDWP->getMemDep() : nullptr, - getAnalysis().getAAResults()); + return Impl.run(F, getAnalysis().getAAResults()); } private: @@ -392,7 +363,6 @@ private: AU.setPreservesCFG(); AU.addRequired(); AU.addPreserved(); - AU.addPreserved(); } }; @@ -408,7 +378,6 @@ FunctionPass *llvm::createMergedLoadStoreMotionPass() { INITIALIZE_PASS_BEGIN(MergedLoadStoreMotionLegacyPass, "mldst-motion", "MergedLoadStoreMotion", false, false) -INITIALIZE_PASS_DEPENDENCY(MemoryDependenceWrapperPass) INITIALIZE_PASS_DEPENDENCY(AAResultsWrapperPass) INITIALIZE_PASS_END(MergedLoadStoreMotionLegacyPass, "mldst-motion", "MergedLoadStoreMotion", false, false) @@ -416,14 +385,12 @@ INITIALIZE_PASS_END(MergedLoadStoreMotionLegacyPass, "mldst-motion", PreservedAnalyses MergedLoadStoreMotionPass::run(Function &F, FunctionAnalysisManager &AM) { MergedLoadStoreMotion Impl; - auto *MD = AM.getCachedResult(F); auto &AA = AM.getResult(F); - if (!Impl.run(F, MD, AA)) + if (!Impl.run(F, AA)) return PreservedAnalyses::all(); PreservedAnalyses PA; PA.preserveSet(); PA.preserve(); - PA.preserve(); return PA; } diff --git a/test/Transforms/GVN/pr36063.ll b/test/Transforms/GVN/pr36063.ll new file mode 100644 index 00000000000..471e338c0f2 --- /dev/null +++ b/test/Transforms/GVN/pr36063.ll @@ -0,0 +1,22 @@ +; RUN: opt < %s -memcpyopt -mldst-motion -gvn -S | FileCheck %s + +define void @foo(i8* %ret, i1 %x) { + %a = alloca i8 + br i1 %x, label %yes, label %no + +yes: ; preds = %0 + %gepa = getelementptr i8, i8* %a, i64 0 + store i8 5, i8* %gepa + br label %out + +no: ; preds = %0 + %gepb = getelementptr i8, i8* %a, i64 0 + store i8 5, i8* %gepb + br label %out + +out: ; preds = %no, %yes + %tmp = load i8, i8* %a +; CHECK-NOT: undef + store i8 %tmp, i8* %ret + ret void +} -- 2.11.0